oscerd commented on PR #27185:
URL: https://github.com/apache/camel/pull/27185#issuecomment-5947527766
Addressed in `cf1430da3e4d` — and first, an apology for the delay on these
three: I replied to both inline threads last round and did not read the review
*body* with the same care, so points 2, 3 and 4 sat unanswered. They were not
out of scope, I simply missed them. Checking the body separately from the
threads is a habit I will keep.
**2. `OpenFgaSecurityPolicy` had no setters.** Not deliberate, so I wired
them rather than documenting the gap. Worth saying why it was worse than it
looked: the policy builds its own `OpenFgaConfiguration`, and that object
already *had* both fields — so the options were reachable by the code and by
nobody configuring it. `setContextualTuples` and `setConditionContext` now
delegate like every other policy setter, and
`theGuardCanCarryContextualTuplesAndAConditionContext` asserts the resolved
tuple and the context arrive on the real `ClientCheckRequest`. There is no
security argument for withholding them: on a policy the value comes from
whoever configured the route, exactly as on an endpoint, which is the whole
basis of the trust story.
**3. No `batchCheck` test.** Two added. The one that earns its place is
`batchCheckSendsTheContextWithEveryItemOfTheBatch` — the batch fans out into
one check per object, so the context has to be on *every* item; on the first
alone the remaining objects would be judged against different facts and the
filter would be quietly wrong rather than visibly broken.
`batchCheckDeniesEverythingWhenAContextualTupleDoesNotResolve` covers the deny
branch you named, and it is **revert-checked**: replacing that branch with
drop-and-proceed fails it.
**4. Commas.** You reasoned it fails loudly at startup. I probed it rather
than agree, because there is an edge case your analysis does not obviously
cover: a comma inside an expression usually pushes the part count off three,
but `user:${bean:ids.pick(1,2)},member` splits into *exactly three* parts — so
in principle it could have compiled a wrong tuple silently. It does not. The
first part fails with `missing } to close the function`, and it fails at
endpoint resolution, earlier even than start. Documented in a NOTE with that
exact message and the advice to pre-compute into an exchange property, and
`refusesAContextualTupleWhoseExpressionContainsAComma` pins the message so a
future Simple change cannot quietly turn this into a silent mis-parse.
Rebased onto current main (44 commits of drift), full reactor `BUILD
SUCCESS`, regen limited to the catalog doc mirror, 110 unit + 13 integration
tests green.
Unchanged from my last comment: the only approval here is AI-generated, so
this is not merge-eligible and I am not merging it. The five threads on #27195
and the two here stay open deliberately — as the author I should not be
resolving my own review conversations.
_Claude Code on behalf of oscerd_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
