ffang commented on PR #3651: URL: https://github.com/apache/cxf/pull/3651#issuecomment-6498811381
Hi Colm, Thanks for following up on this. The "every applicable rule" approach fixes the generic-interface and subclass cases from #3627. I found two combinations that now return 403 where `main` allowed the call: **1. An inherited class-level `@Scopes` is not replaced by a subclass method's `@Scopes`** In [`hasMethodScopes()`](https://github.com/apache/cxf/blob/a87cc108d726feb25026244546e65305d89aeeda/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java#L235-L237), a method-level `@Scopes` only replaces the class-level one when both are declared in the same class. Because `@Scopes` is `@Inherited`, the same effective class annotation gives different results depending on whether the subclass redeclares it: ```java @Scopes("read") public static class Base { public void op() { } } // inherits @Scopes("read") public static class Sub extends Base { @Scopes("admin") public void foo() { } } // redeclares the same @Scopes("read") @Scopes("read") public static class Sub2 extends Base { @Scopes("admin") public void foo() { } } ``` | Caller with only `admin` | `main` | this PR | |---|---|---| | `Sub.foo()` | allowed | **forbidden** (needs `read` + `admin`) | | `Sub2.foo()` | allowed | allowed | This contradicts the stated rule that a method's own `@Scopes` replaces the class-wide one. It also affects the common pattern of a base resource class with a class-level `@Scopes` and subclasses that annotate their own methods. **2. Bridge resolution links unrelated overloads** [`recordBridgedSignature()`](https://github.com/apache/cxf/blob/a87cc108d726feb25026244546e65305d89aeeda/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthScopesFilter.java#L178-L187) treats every same-named method whose parameter types are assignable to the bridge's as the bridge target: ```java public interface Repository<T> { @Scopes("admin") void update(T value); } public static class StringRepository implements Repository<String> { @Override public void update(String value) { } @Scopes("read") public void update(Integer value) { } } ``` The compiler-generated `update(Object)` bridge delegates only to `update(String)`, but `update(Integer)` is linked to it too because `Object` accepts `Integer`: | `update(Integer)`, caller with | `main` | this PR | |---|---|---| | `read` | allowed | **forbidden** | | `read` + `admin` | allowed | allowed | So the unrelated `Integer` overload now also requires the interface's `admin` scope. Both cases fail closed, so nothing is exposed. But they are behaviour changes compared with `main` that would break existing applications, so it would be good to cover both with regression tests. Best Regards 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]
