ffang commented on PR #3687:
URL: https://github.com/apache/cxf/pull/3687#issuecomment-6686919942
Thanks @coheigea , failing closed is the right call. I checked that an empty
rule really does deny every user, anonymous ones included.
One case now breaks a valid configuration, though: an annotation type that
isn't public. An application's own annotation is often declared
package-private, for example:
```java
package com.example.app;
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE, ElementType.METHOD })
@interface AppRoles {
String[] value();
}
public class AppService {
@AppRoles("admin")
public void update() {
}
}
```
`AppRoles` passes the new `setAnnotationClassName` check, because it is an
annotation with a `String[]` `value()`. But `getRoles` calls `value()` from
`org.apache.cxf.interceptor.security`, and that call throws:
```
java.lang.IllegalAccessException: class
org.apache.cxf.interceptor.security.SecureAnnotationsInterceptor
cannot access a member of interface com.example.app.AppRoles
```
So `update()` is now denied to everyone, including `admin`.
<details>
<summary>Test reproducing it (it has to be in a different package from the
interceptor)</summary>
```java
package org.apache.cxf.interceptor.security.app;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.security.Principal;
import org.apache.cxf.interceptor.security.AccessDeniedException;
import org.apache.cxf.interceptor.security.SecureAnnotationsInterceptor;
import org.apache.cxf.message.ExchangeImpl;
import org.apache.cxf.message.Message;
import org.apache.cxf.message.MessageImpl;
import org.apache.cxf.security.SecurityContext;
import org.junit.Test;
import static org.junit.Assert.assertThrows;
public class PackagePrivateAnnotationTest {
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE, ElementType.METHOD })
@interface AppRoles {
String[] value();
}
public static class AppService {
@AppRoles("admin")
public void update() {
}
}
@Test
public void testPackagePrivateAnnotation() throws Exception {
SecureAnnotationsInterceptor in = new
SecureAnnotationsInterceptor();
in.setAnnotationClassName(AppRoles.class.getName());
in.setSecuredObject(new AppService());
in.handleMessage(createMessage("admin"));
assertThrows(AccessDeniedException.class, () ->
in.handleMessage(createMessage("user")));
}
private static Message createMessage(String role) throws Exception {
Message message = new MessageImpl();
message.setExchange(new ExchangeImpl());
message.put("org.apache.cxf.resource.method",
AppService.class.getMethod("update"));
message.put(SecurityContext.class, new SecurityContext() {
@Override
public Principal getUserPrincipal() {
return () -> "bob";
}
message.put("org.apache.cxf.resource.method",
AppService.class.getMethod("update"));
message.put(SecurityContext.class, new SecurityContext() {
@Override
public Principal getUserPrincipal() {
return () -> "bob";
}
@Override
public boolean isUserInRole(String r) {
return role.equals(r);
}
});
return message;
}
}
```
</details>
Results:
- **This PR:** fails on the first `handleMessage`, because `admin` is denied
(`AccessDeniedException: Unauthorized`).
- **main:** fails on the `assertThrows`, because `user` is allowed.
- **With the change below:** passes.
```java
Object roles =
ReflectionUtil.setAccessible(ann.annotationType().getMethod("value")).invoke(ann);
```
This still fails closed. If access can't be granted, for example when the
annotation is in a module that doesn't open its package, `setAccessible`
throws. The existing `catch (Exception ex)` then denies the method.
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]
