From 3e99709035531330eeaf5e494982fdfa97d927d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 13 Nov 2018 09:54:28 +0100 Subject: [PATCH] Replace method interceptor with request filter --- .../main/java/sonia/scm/ScmServletModule.java | 19 ------ .../scm/security/SecurityInterceptor.java | 29 --------- .../scm/security/SecurityRequestFilter.java | 46 ++++++++++++++ .../security/SecurityRequestFilterTest.java | 62 +++++++++++++++++++ 4 files changed, 108 insertions(+), 48 deletions(-) delete mode 100644 scm-webapp/src/main/java/sonia/scm/security/SecurityInterceptor.java create mode 100644 scm-webapp/src/main/java/sonia/scm/security/SecurityRequestFilter.java create mode 100644 scm-webapp/src/test/java/sonia/scm/security/SecurityRequestFilterTest.java diff --git a/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java b/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java index 0d324137ce..90764a7e00 100644 --- a/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java +++ b/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java @@ -45,7 +45,6 @@ import sonia.scm.api.rest.ObjectMapperProvider; import sonia.scm.cache.CacheManager; import sonia.scm.cache.GuavaCacheManager; import sonia.scm.config.ScmConfiguration; -import sonia.scm.debug.DebugResource; import sonia.scm.event.ScmEventBus; import sonia.scm.group.DefaultGroupManager; import sonia.scm.group.GroupDAO; @@ -88,7 +87,6 @@ import sonia.scm.security.DefaultKeyGenerator; import sonia.scm.security.DefaultSecuritySystem; import sonia.scm.security.KeyGenerator; import sonia.scm.security.LoginAttemptHandler; -import sonia.scm.security.SecurityInterceptor; import sonia.scm.security.SecuritySystem; import sonia.scm.store.BlobStoreFactory; import sonia.scm.store.ConfigurationEntryStoreFactory; @@ -120,16 +118,7 @@ import sonia.scm.web.security.DefaultAdministrationContext; import javax.net.ssl.SSLContext; import javax.servlet.ServletContext; -import javax.ws.rs.DELETE; -import javax.ws.rs.GET; -import javax.ws.rs.HEAD; -import javax.ws.rs.OPTIONS; -import javax.ws.rs.POST; -import javax.ws.rs.PUT; -import static com.google.inject.matcher.Matchers.annotatedWith; -import static com.google.inject.matcher.Matchers.not; -import static com.google.inject.matcher.Matchers.subclassesOf; import static sonia.scm.api.v2.resources.ScmPathInfo.REST_API_PATH; /** @@ -330,14 +319,6 @@ public class ScmServletModule extends ServletModule // bind(LastModifiedUpdateListener.class); bind(PushStateDispatcher.class).toProvider(PushStateDispatcherProvider.class); - bindInterceptor(not(subclassesOf(DebugResource.class)), - annotatedWith(GET.class) - .or(annotatedWith(POST.class)) - .or(annotatedWith(HEAD.class)) - .or(annotatedWith(PUT.class)) - .or(annotatedWith(DELETE.class)) - .or(annotatedWith(OPTIONS.class)), - new SecurityInterceptor()); } diff --git a/scm-webapp/src/main/java/sonia/scm/security/SecurityInterceptor.java b/scm-webapp/src/main/java/sonia/scm/security/SecurityInterceptor.java deleted file mode 100644 index e055590bba..0000000000 --- a/scm-webapp/src/main/java/sonia/scm/security/SecurityInterceptor.java +++ /dev/null @@ -1,29 +0,0 @@ -package sonia.scm.security; - -import org.aopalliance.intercept.MethodInterceptor; -import org.aopalliance.intercept.MethodInvocation; -import org.apache.shiro.SecurityUtils; -import org.apache.shiro.authc.AuthenticationException; -import org.apache.shiro.subject.Subject; - -public class SecurityInterceptor implements MethodInterceptor { - - @Override - public Object invoke(MethodInvocation methodInvocation) throws Throwable { - if (hasPermission() || anonymousAccessIsAllowed(methodInvocation)) { - return methodInvocation.proceed(); - } else { - throw new AuthenticationException(); - } - } - - private boolean anonymousAccessIsAllowed(MethodInvocation methodInvocation) { - return methodInvocation.getMethod().isAnnotationPresent(AllowAnonymousAccess.class) - || methodInvocation.getMethod().getDeclaringClass().isAnnotationPresent(AllowAnonymousAccess.class); - } - - private boolean hasPermission() { - Subject subject = SecurityUtils.getSubject(); - return subject.isAuthenticated() || subject.isRemembered(); - } -} diff --git a/scm-webapp/src/main/java/sonia/scm/security/SecurityRequestFilter.java b/scm-webapp/src/main/java/sonia/scm/security/SecurityRequestFilter.java new file mode 100644 index 0000000000..6c3723935e --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/security/SecurityRequestFilter.java @@ -0,0 +1,46 @@ +package sonia.scm.security; + +import org.apache.shiro.SecurityUtils; +import org.apache.shiro.authc.AuthenticationException; +import org.apache.shiro.subject.Subject; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.container.ContainerRequestFilter; +import javax.ws.rs.container.ResourceInfo; +import javax.ws.rs.core.Context; +import javax.ws.rs.ext.Provider; +import java.lang.reflect.Method; + +@Provider +public class SecurityRequestFilter implements ContainerRequestFilter { + + private static final Logger LOG = LoggerFactory.getLogger(SecurityRequestFilter.class); + + @Context + private ResourceInfo resourceInfo; + + @Override + public void filter(ContainerRequestContext requestContext) { + Method resourceMethod = resourceInfo.getResourceMethod(); + LOG.info("jax-rs method {}", resourceMethod.getName()); + if (hasPermission() || anonymousAccessIsAllowed(resourceMethod)) { + LOG.debug("allowed unauthenticated request to method {}", resourceMethod); + // nothing further to do + } else { + LOG.debug("blocked unauthenticated request to method {}", resourceMethod); + throw new AuthenticationException(); + } + } + + private boolean anonymousAccessIsAllowed(Method method) { + return method.isAnnotationPresent(AllowAnonymousAccess.class) + || method.getDeclaringClass().isAnnotationPresent(AllowAnonymousAccess.class); + } + + private boolean hasPermission() { + Subject subject = SecurityUtils.getSubject(); + return subject.isAuthenticated() || subject.isRemembered(); + } +} diff --git a/scm-webapp/src/test/java/sonia/scm/security/SecurityRequestFilterTest.java b/scm-webapp/src/test/java/sonia/scm/security/SecurityRequestFilterTest.java new file mode 100644 index 0000000000..663198dff2 --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/security/SecurityRequestFilterTest.java @@ -0,0 +1,62 @@ +package sonia.scm.security; + +import com.github.sdorra.shiro.ShiroRule; +import com.github.sdorra.shiro.SubjectAware; +import org.apache.shiro.authc.AuthenticationException; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.container.ResourceInfo; + +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) +@SubjectAware(configuration = "classpath:sonia/scm/shiro-001.ini") +public class SecurityRequestFilterTest { + + @Rule + public ShiroRule shiroRule = new ShiroRule(); + + @Mock + private ResourceInfo resourceInfo; + @Mock + private ContainerRequestContext context; + @InjectMocks + private SecurityRequestFilter securityRequestFilter; + + @Test + public void shouldAllowUnauthenticatedAccessForAnnotatedMethod() throws NoSuchMethodException { + when(resourceInfo.getResourceMethod()).thenReturn(SecurityTestClass.class.getMethod("anonymousAccessAllowed")); + + securityRequestFilter.filter(context); + } + + @Test(expected = AuthenticationException.class) + public void shouldRejectUnauthenticatedAccessForAnnotatedMethod() throws NoSuchMethodException { + when(resourceInfo.getResourceMethod()).thenReturn(SecurityTestClass.class.getMethod("loginRequired")); + + securityRequestFilter.filter(context); + } + + @Test + @SubjectAware(username = "trillian", password = "secret") + public void shouldAllowAuthenticatedAccessForMethodWithoutAnnotation() throws NoSuchMethodException { + when(resourceInfo.getResourceMethod()).thenReturn(SecurityTestClass.class.getMethod("loginRequired")); + + securityRequestFilter.filter(context); + } + + private static class SecurityTestClass { + @AllowAnonymousAccess + public void anonymousAccessAllowed() { + } + + public void loginRequired() { + } + } +}