ffang commented on PR #3646: URL: https://github.com/apache/cxf/pull/3646#issuecomment-6475440154
Thanks Colm for working on this, the overload and interface fixes are much needed. A couple of findings on the precedence logic in `authorize()`, [lines 101–108](https://github.com/apache/cxf/blob/1e4dd0715c8e526f64fca384022911af0086674a/rt/security/src/main/java/org/apache/cxf/rt/security/claims/interceptor/ClaimsAuthorizingInterceptor.java#L101-L108). **1. An exact class-level rule prevents the generic interface fallback** ```java interface Repository<T> { @Claim("admin") void update(T value); } @Claim("user") class StringRepository implements Repository<String> { public void update(String value) { } } ``` The interface rule is recorded as `update(Object)`, while the implementation has `update(String)`. The implementation's class-level `user` rule matches the signature exactly, so the name-based fallback is never reached and the interface's method-level `admin` requirement is ignored. A caller with only `user` is accepted, contrary to the stated method-over-class precedence. (`main` behaves the same here, since it never scanned the interface.) **2. A method-level rule from a superclass overrides a more specific class-level rule (a regression compared with `main`)** ```java public static class Base { @Claim("user") public void update() { } } @Claim("admin") public static class AdminSub extends Base { @Override public void update() { } } ``` | Caller of `AdminSub.update()` | `main` | this PR | |---|---|---| | role `user` only | denied | **allowed** | | role `admin` only | allowed | denied | `methodClaimsBySignature` is always preferred over `classClaimsBySignature`, even when the method-level rule comes from a *less specific* class. So the superclass's `@Claim("user")` wins over the subclass's class-level `@Claim("admin")`. On `main`, the subclass is scanned first and its class-level claim is applied to the overriding method. The two cases pull in opposite directions: - (1) needs a method-level rule from an interface to beat an exact class-level match; - (2) needs a more specific class-level rule to still apply when a method-level rule exists further up the hierarchy. Fixing only one of them leaves the other open. One option that covers both is to enforce **all** applicable rules instead of choosing one: - the method-level rule from wherever it is declared, including a generic interface method; - plus the class-level claims of any more specific class in the hierarchy, still dropping a class-level claim that the method-level one overrides (as `isClaimOverridden` does today). That gives `user` + `admin` for (1) and `admin` again for (2), so neither case is weaker than `main`. **3. The name-based fallback is broader than intended** The fallback at [L107](https://github.com/apache/cxf/blob/1e4dd0715c8e526f64fca384022911af0086674a/rt/security/src/main/java/org/apache/cxf/rt/security/claims/interceptor/ClaimsAuthorizingInterceptor.java#L107) is used for any signature without an annotation rule, not only signatures that weren't recorded. Because the implementation's own methods are always recorded, it combines **every** overload with the same name: - an unannotated `delete()` now requires the claims of `delete(String)`; - `StringRepository.update(String)` in the tests requires `admin` **and** `user`, because the unrelated `update()` overload's rule is included too. Both fail closed, so this isn't a security issue, but it may deny legitimate callers. Restricting the fallback to candidates with the same number of parameters and assignable parameter types (`candidate.getParameterTypes()[i].isAssignableFrom(invoked.getParameterTypes()[i])`) would still match the erased `update(T)` while leaving `delete()` and `update()` alone. #3627 (`OAuthScopesFilter`) uses the same design, so the same changes probably apply there too. Freeman -- 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]
