diff --git a/readme.md b/readme.md index 4cf38ed..67aa8f1 100644 --- a/readme.md +++ b/readme.md @@ -42,7 +42,14 @@ To Be Played is split into three repositories: SQL). The access token carries the active role in the `role` claim. **View as**: an admin switches the own login down to USER or AUDITOR and back (`POST /api/auth/view-as`); the choice is stored per login in `refresh_tokens.active_role` and never goes above the stored role, so a demoted admin loses the rights at the next - refresh. Core does not check roles yet. + refresh. Core checks the same claim. +- **Role markers on services**: every service method a controller calls carries `@RequiresRole(Role.X)` or + `@Public`, on the method or on its class (the method wins). A Spring Security method interceptor checks the + active role while a request is served (scheduled jobs and `@Async` work run without a user and are not checked); + a denied call answers `401` without a login, else `403`. `@Public` skips only the role check: login, refresh, + "view as" and the Steam callback prove the caller themselves. The URL rules in `SecurityConfig` stay as the outer + guard. ArchUnit fails the build for an unmarked method, two markers, a controller calling an `@Async` method, and a + service method calling another service's method that needs more. - **Admin users page** (`/api/admin/**`, active role PLATFORM_ADMIN): search and page all users, block and unblock (`users.enabled`; blocking ends every login, the login then answers `403 ACCOUNT_BLOCKED`, but only after the right password), delete (the same flag and nightly cleanup as "Delete my account"). No action on the own account diff --git a/src/main/java/auth_service/configuration/security/MethodSecurityConfig.java b/src/main/java/auth_service/configuration/security/MethodSecurityConfig.java new file mode 100644 index 0000000..eb22b8b --- /dev/null +++ b/src/main/java/auth_service/configuration/security/MethodSecurityConfig.java @@ -0,0 +1,30 @@ +package auth_service.configuration.security; + +import auth_service.shared.security.RequiresRole; +import org.springframework.aop.Advisor; +import org.springframework.aop.Pointcut; +import org.springframework.aop.support.Pointcuts; +import org.springframework.aop.support.annotation.AnnotationMatchingPointcut; +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Role; +import org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor; +import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity; + +/** + * Runs {@link RoleAuthorizationManager} before every method with a role marker, or of a class with one. A method + * that is only {@code @Public} needs no check. + */ +@Configuration +@EnableMethodSecurity(prePostEnabled = false) +public class MethodSecurityConfig { + + @Bean + @Role(BeanDefinition.ROLE_INFRASTRUCTURE) + static Advisor roleAuthorization() { + Pointcut marked = Pointcuts.union(new AnnotationMatchingPointcut(RequiresRole.class, true), + new AnnotationMatchingPointcut(null, RequiresRole.class, true)); + return new AuthorizationManagerBeforeMethodInterceptor(marked, new RoleAuthorizationManager()); + } +} diff --git a/src/main/java/auth_service/configuration/security/RoleAuthorizationManager.java b/src/main/java/auth_service/configuration/security/RoleAuthorizationManager.java new file mode 100644 index 0000000..c0c9604 --- /dev/null +++ b/src/main/java/auth_service/configuration/security/RoleAuthorizationManager.java @@ -0,0 +1,52 @@ +package auth_service.configuration.security; + +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import org.aopalliance.intercept.MethodInvocation; +import org.springframework.aop.support.AopUtils; +import org.springframework.core.annotation.AnnotatedElementUtils; +import org.springframework.security.authorization.AuthorizationDecision; +import org.springframework.security.authorization.AuthorizationManager; +import org.springframework.security.authorization.AuthorizationResult; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.web.context.request.RequestContextHolder; + +import java.lang.reflect.AnnotatedElement; +import java.lang.reflect.Method; +import java.util.function.Supplier; + +class RoleAuthorizationManager implements AuthorizationManager { + + private static final AuthorizationDecision GRANTED = new AuthorizationDecision(true); + private static final AuthorizationDecision DENIED = new AuthorizationDecision(false); + + @Override + public AuthorizationResult authorize(Supplier authentication, MethodInvocation invocation) { + if (RequestContextHolder.getRequestAttributes() == null) { + return GRANTED; + } + Class targetClass = invocation.getThis() != null ? AopUtils.getTargetClass(invocation.getThis()) + : invocation.getMethod().getDeclaringClass(); + Method method = AopUtils.getMostSpecificMethod(invocation.getMethod(), targetClass); + AnnotatedElement marked = isMarked(method) ? method : targetClass; + if (AnnotatedElementUtils.hasAnnotation(marked, Public.class)) { + return GRANTED; + } + RequiresRole required = AnnotatedElementUtils.findMergedAnnotation(marked, RequiresRole.class); + if (required == null) { + return DENIED; + } + return hasAuthority(authentication.get(), "ROLE_" + required.value().name()) ? GRANTED : DENIED; + } + + private static boolean isMarked(Method method) { + return AnnotatedElementUtils.hasAnnotation(method, RequiresRole.class) + || AnnotatedElementUtils.hasAnnotation(method, Public.class); + } + + private static boolean hasAuthority(Authentication authentication, String authority) { + return authentication != null && authentication.getAuthorities().stream() + .map(GrantedAuthority::getAuthority).anyMatch(authority::equals); + } +} diff --git a/src/main/java/auth_service/modules/account/service/AccountService.java b/src/main/java/auth_service/modules/account/service/AccountService.java index 49b82da..6e914bf 100644 --- a/src/main/java/auth_service/modules/account/service/AccountService.java +++ b/src/main/java/auth_service/modules/account/service/AccountService.java @@ -5,6 +5,8 @@ import auth_service.modules.account.exception.TooManyDeleteAttemptsException; import auth_service.modules.account.repo.AccountRepository; import auth_service.shared.account.AccountDeleter; import auth_service.shared.security.AttemptLimiter; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; import lombok.extern.slf4j.Slf4j; import org.springframework.cache.annotation.CacheEvict; import org.springframework.security.crypto.password.PasswordEncoder; @@ -16,6 +18,7 @@ import java.util.List; import java.util.Optional; @Service +@RequiresRole(Role.USER) @Slf4j public class AccountService implements AccountDeleter { @@ -58,6 +61,7 @@ public class AccountService implements AccountDeleter { } @Override + @RequiresRole(Role.PLATFORM_ADMIN) @CacheEvict(value = {"usersByEmail", "publicStats"}, allEntries = true) public boolean deleteByAdmin(long userId) { return flagAndTellCore(userId); diff --git a/src/main/java/auth_service/modules/admin/service/AdminUserService.java b/src/main/java/auth_service/modules/admin/service/AdminUserService.java index a7015bd..a70d599 100644 --- a/src/main/java/auth_service/modules/admin/service/AdminUserService.java +++ b/src/main/java/auth_service/modules/admin/service/AdminUserService.java @@ -7,6 +7,7 @@ import auth_service.modules.admin.model.TargetState; import auth_service.modules.admin.repo.AdminUserRepository; import auth_service.shared.account.AccountDeleter; import auth_service.shared.audit.AdminAuditLog; +import auth_service.shared.security.RequiresRole; import auth_service.shared.security.Role; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -17,6 +18,7 @@ import org.springframework.transaction.annotation.Transactional; import java.time.Instant; @Service +@RequiresRole(Role.PLATFORM_ADMIN) @RequiredArgsConstructor @Slf4j public class AdminUserService { diff --git a/src/main/java/auth_service/modules/login/service/LoginService.java b/src/main/java/auth_service/modules/login/service/LoginService.java index a63ef2b..e7bbd49 100644 --- a/src/main/java/auth_service/modules/login/service/LoginService.java +++ b/src/main/java/auth_service/modules/login/service/LoginService.java @@ -1,5 +1,6 @@ package auth_service.modules.login.service; +import auth_service.shared.security.Public; import auth_service.shared.security.TokenGenerator; import auth_service.modules.login.exception.AccountBlockedException; import auth_service.modules.login.exception.InvalidCredentialsException; @@ -15,6 +16,7 @@ import java.util.Optional; import java.util.UUID; @Service +@Public @Slf4j public class LoginService { private final MinimalUserRepository minimalUserRepository; diff --git a/src/main/java/auth_service/modules/login/service/RefreshTokenService.java b/src/main/java/auth_service/modules/login/service/RefreshTokenService.java index c09d7a9..bccedd9 100644 --- a/src/main/java/auth_service/modules/login/service/RefreshTokenService.java +++ b/src/main/java/auth_service/modules/login/service/RefreshTokenService.java @@ -7,6 +7,7 @@ import auth_service.modules.login.persistence.MinimalUserRepository; import auth_service.modules.login.persistence.RefreshTokenRepository; import auth_service.modules.login.exception.RoleNotAllowedException; import auth_service.shared.audit.AdminAuditLog; +import auth_service.shared.security.Public; import auth_service.shared.security.Role; import auth_service.shared.security.SessionProperties; import auth_service.shared.security.TokenGenerator; @@ -31,6 +32,7 @@ import java.util.UUID; * form a family. */ @Service +@Public @RequiredArgsConstructor @Slf4j public class RefreshTokenService { diff --git a/src/main/java/auth_service/modules/profile/service/PublicProfileService.java b/src/main/java/auth_service/modules/profile/service/PublicProfileService.java index 037c1c2..1cea701 100644 --- a/src/main/java/auth_service/modules/profile/service/PublicProfileService.java +++ b/src/main/java/auth_service/modules/profile/service/PublicProfileService.java @@ -3,12 +3,14 @@ package auth_service.modules.profile.service; import auth_service.modules.profile.model.PublicProfile; import auth_service.modules.profile.repo.PublicProfileRepository; import auth_service.shared.security.ProfileTicketIssuer; +import auth_service.shared.security.Public; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; import java.util.Optional; @Service +@Public @RequiredArgsConstructor public class PublicProfileService { diff --git a/src/main/java/auth_service/modules/registration/service/RegistrationService.java b/src/main/java/auth_service/modules/registration/service/RegistrationService.java index fd39483..8a0a0fe 100644 --- a/src/main/java/auth_service/modules/registration/service/RegistrationService.java +++ b/src/main/java/auth_service/modules/registration/service/RegistrationService.java @@ -5,11 +5,13 @@ import auth_service.modules.registration.exception.InvalidInvitationCodeExceptio import auth_service.modules.registration.exception.DisplayNameAlreadyRegisteredException; import auth_service.modules.registration.model.Registration; import auth_service.modules.registration.persistence.RegisteredUserRepository; +import auth_service.shared.security.Public; import lombok.RequiredArgsConstructor; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.stereotype.Service; @Service +@Public @RequiredArgsConstructor public class RegistrationService { diff --git a/src/main/java/auth_service/modules/stats/service/PublicStatsService.java b/src/main/java/auth_service/modules/stats/service/PublicStatsService.java index bfba981..1e3dbe7 100644 --- a/src/main/java/auth_service/modules/stats/service/PublicStatsService.java +++ b/src/main/java/auth_service/modules/stats/service/PublicStatsService.java @@ -2,11 +2,13 @@ package auth_service.modules.stats.service; import auth_service.modules.stats.model.PublicStats; import auth_service.modules.stats.repo.StatsRepository; +import auth_service.shared.security.Public; import lombok.RequiredArgsConstructor; import org.springframework.cache.annotation.Cacheable; import org.springframework.stereotype.Service; @Service +@Public @RequiredArgsConstructor public class PublicStatsService { diff --git a/src/main/java/auth_service/modules/user/service/SteamLinkService.java b/src/main/java/auth_service/modules/user/service/SteamLinkService.java index 2f9006d..2ca281f 100644 --- a/src/main/java/auth_service/modules/user/service/SteamLinkService.java +++ b/src/main/java/auth_service/modules/user/service/SteamLinkService.java @@ -1,6 +1,9 @@ package auth_service.modules.user.service; import auth_service.modules.user.model.SteamLinkOutcome; +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; import lombok.extern.slf4j.Slf4j; @@ -22,6 +25,7 @@ import java.util.Optional; * attacker could send a victim a Steam login link with the attacker's state and so take the victim's Steam ID. */ @Service +@RequiresRole(Role.USER) @Slf4j public class SteamLinkService { @@ -72,6 +76,7 @@ public class SteamLinkService { * @param cookieState from the binding cookie; null when the browser sent none * @param params all query parameters of Steam's redirect */ + @Public public SteamLinkOutcome complete(String state, String cookieState, Map params) { if ("cancel".equals(params.get("openid.mode"))) { return SteamLinkOutcome.CANCELLED; @@ -111,6 +116,7 @@ public class SteamLinkService { && MessageDigest.isEqual(state.getBytes(StandardCharsets.UTF_8), cookieState.getBytes(StandardCharsets.UTF_8)); } + @Public public String frontendRedirect(SteamLinkOutcome outcome) { return frontendUrl + "/settings?steam=" + outcome.queryValue(); } diff --git a/src/main/java/auth_service/modules/user/service/UserService.java b/src/main/java/auth_service/modules/user/service/UserService.java index 634b5a8..eb7787d 100644 --- a/src/main/java/auth_service/modules/user/service/UserService.java +++ b/src/main/java/auth_service/modules/user/service/UserService.java @@ -8,6 +8,7 @@ import auth_service.modules.user.model.SteamLinkOutcome; import auth_service.modules.user.model.User; import auth_service.modules.user.model.UserSettings; import auth_service.modules.user.persistence.UserRepository; +import auth_service.shared.security.RequiresRole; import auth_service.shared.security.Role; import auth_service.shared.security.TokenGenerator; import lombok.RequiredArgsConstructor; @@ -34,6 +35,7 @@ public class UserService { * @param email The authenticated user's email * @return Optional containing the User model if found */ + @RequiresRole(Role.USER) @Cacheable(value = "usersByEmail", key = "#email") public User getUserByEmail(String email) { return userRepository.findByEmail(email).orElseThrow(() -> new UserNotFoundException("Could not find user with email " + email)); @@ -42,6 +44,7 @@ public class UserService { /** * The Steam ID is not part of the settings: it changes only through {@link SteamLinkService}. */ + @RequiresRole(Role.USER) @Transactional @CacheEvict(value = "usersByEmail", allEntries = true) public void updateUserSettings(long userId, String displayName, UserSettings userSettings) { @@ -54,7 +57,8 @@ public class UserService { /** * Stores a Steam ID that Steam proved. An account that only typed the same ID in loses it; an account that - * proved it keeps it. + * proved it keeps it. No role marker: Steam's callback has no login. A controller that calls it fails + * the ArchitectureTest. */ @Transactional @CacheEvict(value = "usersByEmail", allEntries = true) @@ -76,6 +80,7 @@ public class UserService { /** * @return a new access token without the steamId claim */ + @RequiresRole(Role.USER) @Transactional @CacheEvict(value = "usersByEmail", allEntries = true) public String unlinkSteamId(long userId, String email, Role role) { @@ -84,6 +89,7 @@ public class UserService { return tokenGenerator.generateToken(String.valueOf(userId), email, null, role); } + @RequiresRole(Role.USER) @Transactional @CacheEvict(value = "usersByEmail", allEntries = true) public void updateAppearance(long userId, Appearance appearance) { @@ -93,6 +99,7 @@ public class UserService { /** * Stores each group once; the request may repeat one. */ + @RequiresRole(Role.USER) @Transactional @CacheEvict(value = "usersByEmail", allEntries = true) public void updateProfileStats(long userId, List groups) { diff --git a/src/main/java/auth_service/shared/security/Public.java b/src/main/java/auth_service/shared/security/Public.java new file mode 100644 index 0000000..71280b7 --- /dev/null +++ b/src/main/java/auth_service/shared/security/Public.java @@ -0,0 +1,17 @@ +package auth_service.shared.security; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Callable without a login, e.g. the landing stats. Only the role check is skipped: login, refresh, "view as" and + * the Steam callback prove the caller themselves (password, refresh cookie, state cookie). See {@link RequiresRole}. + */ +@Documented +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface Public { +} diff --git a/src/main/java/auth_service/shared/security/RequiresRole.java b/src/main/java/auth_service/shared/security/RequiresRole.java new file mode 100644 index 0000000..63d6a16 --- /dev/null +++ b/src/main/java/auth_service/shared/security/RequiresRole.java @@ -0,0 +1,20 @@ +package auth_service.shared.security; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * The lowest active role that may call this service method, or every method of this class. A method marker + * replaces the class marker. Every service method a controller calls carries {@link RequiresRole} or + * {@link Public} (ArchitectureTest). Checked only while an HTTP request is served: scheduled jobs and + * {@code @Async} work run without a user. + */ +@Documented +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface RequiresRole { + Role value(); +} diff --git a/src/test/java/auth_service/architecture/ArchitectureTest.java b/src/test/java/auth_service/architecture/ArchitectureTest.java index f7456be..0591207 100644 --- a/src/test/java/auth_service/architecture/ArchitectureTest.java +++ b/src/test/java/auth_service/architecture/ArchitectureTest.java @@ -119,4 +119,17 @@ class ArchitectureTest extends AbstractArchitectureTest { .orShould().beAnnotatedWith(org.springframework.web.bind.annotation.ControllerAdvice.class) .orShould().beAnnotatedWith(org.springframework.web.bind.annotation.RestControllerAdvice.class) .because("Classes in 'api' package should be registered as Spring @RestController or @ControllerAdvice or @RestControllerAdvice beans."); + + // =================================================================================== + // 6. ROLE MARKERS (see RoleAnnotationRules) + // =================================================================================== + + @ArchTest + static final ArchRule controllers_call_only_marked_service_methods = RoleAnnotationRules.controllers_call_only_marked_service_methods; + + @ArchTest + static final ArchRule one_role_marker_per_element = RoleAnnotationRules.one_role_marker_per_element; + + @ArchTest + static final ArchRule services_never_call_a_method_that_needs_more = RoleAnnotationRules.services_never_call_a_method_that_needs_more; } diff --git a/src/test/java/auth_service/architecture/RoleAnnotationRules.java b/src/test/java/auth_service/architecture/RoleAnnotationRules.java new file mode 100644 index 0000000..b0063c0 --- /dev/null +++ b/src/test/java/auth_service/architecture/RoleAnnotationRules.java @@ -0,0 +1,192 @@ +package auth_service.architecture; + +import com.tngtech.archunit.base.DescribedPredicate; +import com.tngtech.archunit.core.domain.JavaClass; +import com.tngtech.archunit.core.domain.JavaCodeUnit; +import com.tngtech.archunit.core.domain.JavaCodeUnitAccess; +import com.tngtech.archunit.core.domain.JavaMethod; +import com.tngtech.archunit.core.domain.JavaModifier; +import com.tngtech.archunit.core.domain.properties.HasAnnotations; +import com.tngtech.archunit.lang.ArchCondition; +import com.tngtech.archunit.lang.ArchRule; +import com.tngtech.archunit.lang.ConditionEvents; +import com.tngtech.archunit.lang.SimpleConditionEvent; +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; +import org.springframework.scheduling.annotation.Async; + +import java.lang.annotation.Annotation; +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; + +/** + * The rules for {@link RequiresRole} and {@link Public}. Core has the same rules plus its {@code Internal} marker. + */ +final class RoleAnnotationRules { + + private static final List> MARKERS = List.of(RequiresRole.class, Public.class); + + private static final String APP_PACKAGE = "auth_service."; + + private RoleAnnotationRules() { + } + + /** + * What a marked method needs from its caller. {@code role} is set only for {@link RequiresRole}. + */ + private record Need(Class marker, Role role) { + + boolean isMetBy(Need caller) { + if (marker == Public.class) { + return true; + } + return caller.marker == RequiresRole.class && caller.role.atLeast(role); + } + + @Override + public String toString() { + return marker == RequiresRole.class ? "@RequiresRole(" + role + ")" : "@" + marker.getSimpleName(); + } + } + + private static long markerCount(HasAnnotations element) { + return MARKERS.stream().filter(element::isAnnotatedWith).count(); + } + + private static Optional need(HasAnnotations element) { + for (Class marker : MARKERS) { + if (element.isAnnotatedWith(marker)) { + Role role = marker == RequiresRole.class + ? element.getAnnotationOfType(RequiresRole.class).value() : null; + return Optional.of(new Need(marker, role)); + } + } + return Optional.empty(); + } + + private static Optional effectiveNeed(JavaMethod method) { + Optional own = need(method); + return own.isPresent() ? own : need(method.getOwner()); + } + + private record MethodAccess(JavaCodeUnitAccess access, JavaMethod target) { + + @Override + public String toString() { + return target.getOwner().getSimpleName() + "." + target.getName() + " called in " + + access.getSourceCodeLocation(); + } + } + + /** + * Calls and method references ({@code service::method}) to non-static methods: a proxy never sees a static call, + * so a marker there would promise a check that does not run. + */ + private static List methodAccesses(JavaCodeUnit from) { + return Stream.>concat(from.getMethodCallsFromSelf().stream(), + from.getMethodReferencesFromSelf().stream()) + .flatMap(access -> access.getTarget().resolveMember().stream() + .filter(JavaMethod.class::isInstance).map(JavaMethod.class::cast) + .filter(method -> !method.getModifiers().contains(JavaModifier.STATIC)) + .map(method -> new MethodAccess(access, method))) + .toList(); + } + + private static boolean isService(JavaClass javaClass) { + return javaClass.getSimpleName().endsWith("Service") && javaClass.getPackageName().contains(".service"); + } + + private static boolean isAsync(JavaMethod method) { + return method.isAnnotatedWith(Async.class) || method.getOwner().isAnnotatedWith(Async.class); + } + + private static final DescribedPredicate SERVICE_CLASSES = + DescribedPredicate.describe("are services", RoleAnnotationRules::isService); + + /** + * Every service method a controller calls is marked, and is not {@code @Async}: there the check would never run. + */ + static final ArchRule controllers_call_only_marked_service_methods = classes() + .that().resideInAPackage("..api..") + .should(new ArchCondition<>("call only service methods that carry a role marker and are not @Async") { + @Override + public void check(JavaClass controller, ConditionEvents events) { + for (JavaCodeUnit codeUnit : controller.getCodeUnits()) { + for (MethodAccess access : methodAccesses(codeUnit)) { + if (!isService(access.target().getOwner())) { + continue; + } + if (effectiveNeed(access.target()).isEmpty()) { + events.add(SimpleConditionEvent.violated(access.access(), "no role marker on " + access)); + } + if (isAsync(access.target())) { + events.add(SimpleConditionEvent.violated(access.access(), "@Async method " + access)); + } + } + } + } + }) + .because("a new endpoint must not forget the role check"); + + static final ArchRule one_role_marker_per_element = classes() + .should(new ArchCondition<>("carry at most one role marker on the class and on each method") { + @Override + public void check(JavaClass javaClass, ConditionEvents events) { + if (markerCount(javaClass) > 1) { + events.add(SimpleConditionEvent.violated(javaClass, javaClass.getName() + " has two role markers")); + } + for (JavaMethod method : javaClass.getMethods()) { + if (markerCount(method) > 1) { + events.add(SimpleConditionEvent.violated(method, method.getFullName() + " has two role markers")); + } + } + } + }) + .because("the marker must say one thing"); + + /** + * Starts at every method the proxy checks (not private, not static) and follows every call that is not checked + * again: self calls and calls of unmarked methods. Calls through an interface (a port) end the walk: the + * implementation is not known here. + */ + static final ArchRule services_never_call_a_method_that_needs_more = classes() + .that(SERVICE_CLASSES) + .should(new ArchCondition<>("never call a service method that needs more than the caller") { + @Override + public void check(JavaClass service, ConditionEvents events) { + for (JavaMethod entry : service.getMethods()) { + Optional caller = effectiveNeed(entry); + if (caller.isEmpty() || entry.getModifiers().contains(JavaModifier.PRIVATE) + || entry.getModifiers().contains(JavaModifier.STATIC)) { + continue; + } + Set seen = new HashSet<>(List.of(entry)); + Deque toVisit = new ArrayDeque<>(seen); + while (!toVisit.isEmpty()) { + for (MethodAccess access : methodAccesses(toVisit.pop())) { + JavaMethod target = access.target(); + Optional callee = target.getOwner().equals(service) ? Optional.empty() + : effectiveNeed(target); + if (callee.isEmpty()) { + if (target.getOwner().getPackageName().startsWith(APP_PACKAGE) && seen.add(target)) { + toVisit.push(target); + } + } else if (!callee.get().isMetBy(caller.get())) { + events.add(SimpleConditionEvent.violated(access.access(), entry.getName() + " (" + + caller.get() + ") calls " + access + ", which needs " + callee.get())); + } + } + } + } + } + }) + .because("the inner check would refuse a caller the outer check let in"); +} diff --git a/src/test/java/auth_service/architecture/RoleAnnotationRulesTest.java b/src/test/java/auth_service/architecture/RoleAnnotationRulesTest.java new file mode 100644 index 0000000..c4fa947 --- /dev/null +++ b/src/test/java/auth_service/architecture/RoleAnnotationRulesTest.java @@ -0,0 +1,100 @@ +package auth_service.architecture; + +import com.tngtech.archunit.core.domain.JavaClasses; +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.lang.ArchRule; +import com.tngtech.archunit.lang.EvaluationResult; +import auth_service.architecture.fixture.api.BadController; +import auth_service.architecture.fixture.api.GoodController; +import auth_service.architecture.fixture.api.MethodReferenceController; +import auth_service.architecture.fixture.service.ForwardingService; +import auth_service.architecture.fixture.service.MarkedService; +import auth_service.architecture.fixture.service.PublicCallerService; +import auth_service.architecture.fixture.service.PublicViaUnmarkedCallerService; +import auth_service.architecture.fixture.service.PublicHelperCallerService; +import auth_service.architecture.fixture.service.PublicMethodReferenceCallerService; +import auth_service.architecture.fixture.service.TwoMarkersService; +import auth_service.architecture.fixture.service.UnmarkedService; +import auth_service.architecture.fixture.service.UserCallerService; +import auth_service.shared.test.AbstractArchitectureTest; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +class RoleAnnotationRulesTest extends AbstractArchitectureTest { + + private static List violations(ArchRule rule, Class... classes) { + JavaClasses imported = new ClassFileImporter().importClasses(classes); + EvaluationResult result = rule.evaluate(imported); + return result.getFailureReport().getDetails(); + } + + @Test + void controllerRule_PassesMarkedMethodsTheMethodMarkerAndStaticHelpers() { + assertThat(violations(RoleAnnotationRules.controllers_call_only_marked_service_methods, + GoodController.class, MarkedService.class, UnmarkedService.class)).isEmpty(); + } + + @Test + void controllerRule_FindsAnUnmarkedAndAnAsyncMethod() { + List found = violations(RoleAnnotationRules.controllers_call_only_marked_service_methods, + BadController.class, MarkedService.class, UnmarkedService.class); + + assertThat(found).hasSize(2); + assertThat(found).anyMatch(line -> line.contains("no role marker on UnmarkedService.bare")); + assertThat(found).anyMatch(line -> line.contains("@Async method MarkedService.later")); + } + + @Test + void markerRule_FindsTwoMarkersOnOneMethod() { + assertThat(violations(RoleAnnotationRules.one_role_marker_per_element, TwoMarkersService.class)) + .singleElement().asString().contains("both"); + } + + @Test + void escalationRule_FindsAPublicMethodCallingAUserMethod() { + List found = violations(RoleAnnotationRules.services_never_call_a_method_that_needs_more, + PublicCallerService.class, MarkedService.class); + + assertThat(found).singleElement().asString() + .contains("callsUser (@Public)", "MarkedService.plain", "needs @RequiresRole(USER)"); + } + + @Test + void escalationRule_FindsAUserMethodCallingAnAuditorMethod() { + assertThat(violations(RoleAnnotationRules.services_never_call_a_method_that_needs_more, + UserCallerService.class, MarkedService.class)) + .singleElement().asString().contains("needs @RequiresRole(AUDITOR)"); + } + + @Test + void controllerRule_FindsAnUnmarkedMethodCalledByMethodReference() { + assertThat(violations(RoleAnnotationRules.controllers_call_only_marked_service_methods, + MethodReferenceController.class, UnmarkedService.class)) + .anyMatch(line -> line.contains("no role marker on UnmarkedService.bare")); + } + + @Test + void escalationRule_FindsAPublicMethodCallingAUserMethodThroughAPrivateHelper() { + assertThat(violations(RoleAnnotationRules.services_never_call_a_method_that_needs_more, + PublicHelperCallerService.class, MarkedService.class)) + .anyMatch(line -> line.contains("MarkedService.plain") && line.contains("needs @RequiresRole(USER)")); + } + + @Test + void escalationRule_FindsAPublicMethodCallingAUserMethodByMethodReference() { + assertThat(violations(RoleAnnotationRules.services_never_call_a_method_that_needs_more, + PublicMethodReferenceCallerService.class, MarkedService.class)) + .anyMatch(line -> line.contains("MarkedService.plain") && line.contains("needs @RequiresRole(USER)")); + } + + @Test + void escalationRule_FindsAPublicMethodReachingAUserMethodThroughAnUnmarkedService() { + assertThat(violations(RoleAnnotationRules.services_never_call_a_method_that_needs_more, + PublicViaUnmarkedCallerService.class, ForwardingService.class, MarkedService.class)) + .singleElement().asString() + .contains("callsThroughAnUnmarkedService (@Public)", "MarkedService.plain", "needs @RequiresRole(USER)"); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/api/BadController.java b/src/test/java/auth_service/architecture/fixture/api/BadController.java new file mode 100644 index 0000000..e9eb3d8 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/api/BadController.java @@ -0,0 +1,15 @@ +package auth_service.architecture.fixture.api; + +import auth_service.architecture.fixture.service.MarkedService; +import auth_service.architecture.fixture.service.UnmarkedService; + +public class BadController { + + private final UnmarkedService unmarkedService = new UnmarkedService(); + private final MarkedService markedService = new MarkedService(); + + public void handle() { + unmarkedService.bare(); + markedService.later(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/api/GoodController.java b/src/test/java/auth_service/architecture/fixture/api/GoodController.java new file mode 100644 index 0000000..fc609ab --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/api/GoodController.java @@ -0,0 +1,15 @@ +package auth_service.architecture.fixture.api; + +import auth_service.architecture.fixture.service.MarkedService; +import auth_service.architecture.fixture.service.UnmarkedService; + +public class GoodController { + + private final MarkedService markedService = new MarkedService(); + + public void handle() { + markedService.plain(); + markedService.open(); + UnmarkedService.helper(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/api/MethodReferenceController.java b/src/test/java/auth_service/architecture/fixture/api/MethodReferenceController.java new file mode 100644 index 0000000..08a998a --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/api/MethodReferenceController.java @@ -0,0 +1,13 @@ +package auth_service.architecture.fixture.api; + +import auth_service.architecture.fixture.service.UnmarkedService; + +public class MethodReferenceController { + + private final UnmarkedService unmarkedService = new UnmarkedService(); + + public void handle() { + Runnable call = unmarkedService::bare; + call.run(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/ForwardingService.java b/src/test/java/auth_service/architecture/fixture/service/ForwardingService.java new file mode 100644 index 0000000..511b075 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/ForwardingService.java @@ -0,0 +1,10 @@ +package auth_service.architecture.fixture.service; + +public class ForwardingService { + + private final MarkedService markedService = new MarkedService(); + + public void forward() { + markedService.plain(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/MarkedService.java b/src/test/java/auth_service/architecture/fixture/service/MarkedService.java new file mode 100644 index 0000000..dc13f90 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/MarkedService.java @@ -0,0 +1,25 @@ +package auth_service.architecture.fixture.service; + +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; +import org.springframework.scheduling.annotation.Async; + +@RequiresRole(Role.USER) +public class MarkedService { + + public void plain() { + } + + @Public + public void open() { + } + + @RequiresRole(Role.AUDITOR) + public void audit() { + } + + @Async + public void later() { + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/PublicCallerService.java b/src/test/java/auth_service/architecture/fixture/service/PublicCallerService.java new file mode 100644 index 0000000..9d09d73 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/PublicCallerService.java @@ -0,0 +1,17 @@ +package auth_service.architecture.fixture.service; + +import auth_service.shared.security.Public; + +@Public +public class PublicCallerService { + + private final MarkedService markedService = new MarkedService(); + + public void callsOpen() { + markedService.open(); + } + + public void callsUser() { + markedService.plain(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/PublicHelperCallerService.java b/src/test/java/auth_service/architecture/fixture/service/PublicHelperCallerService.java new file mode 100644 index 0000000..3f53307 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/PublicHelperCallerService.java @@ -0,0 +1,20 @@ +package auth_service.architecture.fixture.service; + +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; + +@RequiresRole(Role.USER) +public class PublicHelperCallerService { + + private final MarkedService markedService = new MarkedService(); + + @Public + public void open() { + helper(); + } + + private void helper() { + markedService.plain(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/PublicMethodReferenceCallerService.java b/src/test/java/auth_service/architecture/fixture/service/PublicMethodReferenceCallerService.java new file mode 100644 index 0000000..310c9c3 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/PublicMethodReferenceCallerService.java @@ -0,0 +1,17 @@ +package auth_service.architecture.fixture.service; + +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; + +@RequiresRole(Role.USER) +public class PublicMethodReferenceCallerService { + + private final MarkedService markedService = new MarkedService(); + + @Public + public void open() { + Runnable call = markedService::plain; + call.run(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/PublicViaUnmarkedCallerService.java b/src/test/java/auth_service/architecture/fixture/service/PublicViaUnmarkedCallerService.java new file mode 100644 index 0000000..4061664 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/PublicViaUnmarkedCallerService.java @@ -0,0 +1,13 @@ +package auth_service.architecture.fixture.service; + +import auth_service.shared.security.Public; + +@Public +public class PublicViaUnmarkedCallerService { + + private final ForwardingService forwardingService = new ForwardingService(); + + public void callsThroughAnUnmarkedService() { + forwardingService.forward(); + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/TwoMarkersService.java b/src/test/java/auth_service/architecture/fixture/service/TwoMarkersService.java new file mode 100644 index 0000000..9d5d68f --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/TwoMarkersService.java @@ -0,0 +1,13 @@ +package auth_service.architecture.fixture.service; + +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; + +public class TwoMarkersService { + + @Public + @RequiresRole(Role.USER) + public void both() { + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/UnmarkedService.java b/src/test/java/auth_service/architecture/fixture/service/UnmarkedService.java new file mode 100644 index 0000000..f04e7eb --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/UnmarkedService.java @@ -0,0 +1,11 @@ +package auth_service.architecture.fixture.service; + +public class UnmarkedService { + + public void bare() { + } + + public static boolean helper() { + return true; + } +} diff --git a/src/test/java/auth_service/architecture/fixture/service/UserCallerService.java b/src/test/java/auth_service/architecture/fixture/service/UserCallerService.java new file mode 100644 index 0000000..6a98eb8 --- /dev/null +++ b/src/test/java/auth_service/architecture/fixture/service/UserCallerService.java @@ -0,0 +1,14 @@ +package auth_service.architecture.fixture.service; + +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; + +@RequiresRole(Role.USER) +public class UserCallerService { + + private final MarkedService markedService = new MarkedService(); + + public void callsAuditor() { + markedService.audit(); + } +} diff --git a/src/test/java/auth_service/configuration/security/RoleAuthorizationManagerTest.java b/src/test/java/auth_service/configuration/security/RoleAuthorizationManagerTest.java new file mode 100644 index 0000000..80192a3 --- /dev/null +++ b/src/test/java/auth_service/configuration/security/RoleAuthorizationManagerTest.java @@ -0,0 +1,100 @@ +package auth_service.configuration.security; + +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; +import auth_service.shared.test.AbstractUnitTest; +import org.aopalliance.intercept.MethodInvocation; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.security.authentication.AnonymousAuthenticationToken; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.web.context.request.RequestContextHolder; +import org.springframework.web.context.request.ServletRequestAttributes; + +import java.util.Arrays; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; + +class RoleAuthorizationManagerTest extends AbstractUnitTest { + + private final RoleAuthorizationManager manager = new RoleAuthorizationManager(); + + @RequiresRole(Role.USER) + static class UserMarked { + public void plain() { + } + + @RequiresRole(Role.AUDITOR) + public void audit() { + } + + @Public + public void open() { + } + } + + @AfterEach + void clearRequest() { + RequestContextHolder.resetRequestAttributes(); + } + + private static void inRequest() { + RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(new MockHttpServletRequest())); + } + + // The authorities JwtAuthenticationFilter grants: the role and every role below it. + private static Authentication login(Role role) { + return new UsernamePasswordAuthenticationToken("42", null, Arrays.stream(Role.values()).filter(role::atLeast) + .map(each -> new SimpleGrantedAuthority("ROLE_" + each.name())).toList()); + } + + private static Authentication anonymous() { + return new AnonymousAuthenticationToken("key", "anonymousUser", List.of(new SimpleGrantedAuthority("ROLE_ANONYMOUS"))); + } + + private boolean granted(Authentication authentication, Object target, String method) throws Exception { + MethodInvocation invocation = mock(MethodInvocation.class); + // Lenient: without a request the manager does not look at the invocation. + lenient().when(invocation.getThis()).thenReturn(target); + lenient().when(invocation.getMethod()).thenReturn(target.getClass().getMethod(method)); + return manager.authorize(() -> authentication, invocation).isGranted(); + } + + @Test + void classMarker_CoversTheMethodsWithoutOwnMarker() throws Exception { + inRequest(); + + assertTrue(granted(login(Role.USER), new UserMarked(), "plain")); + assertFalse(granted(anonymous(), new UserMarked(), "plain")); + } + + @Test + void methodMarker_ReplacesTheClassMarker() throws Exception { + inRequest(); + + assertFalse(granted(login(Role.USER), new UserMarked(), "audit")); + assertTrue(granted(anonymous(), new UserMarked(), "open")); + } + + @Test + void higherRoles_PassLowerChecks() throws Exception { + inRequest(); + + assertTrue(granted(login(Role.AUDITOR), new UserMarked(), "audit")); + assertTrue(granted(login(Role.PLATFORM_ADMIN), new UserMarked(), "audit")); + assertTrue(granted(login(Role.PLATFORM_ADMIN), new UserMarked(), "plain")); + } + + @Test + void withoutARequest_EverythingRuns() throws Exception { + assertTrue(granted(null, new UserMarked(), "audit")); + } +} diff --git a/src/test/java/auth_service/configuration/security/RoleCheckFlowTest.java b/src/test/java/auth_service/configuration/security/RoleCheckFlowTest.java new file mode 100644 index 0000000..e5cdfdd --- /dev/null +++ b/src/test/java/auth_service/configuration/security/RoleCheckFlowTest.java @@ -0,0 +1,131 @@ +package auth_service.configuration.security; + +import auth_service.shared.security.Public; +import auth_service.shared.security.RequiresRole; +import auth_service.shared.security.Role; +import auth_service.shared.security.TokenGenerator; +import auth_service.shared.api.AuthCookies; +import auth_service.shared.test.AbstractIntegrationTest; +import jakarta.servlet.http.Cookie; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.context.annotation.Import; +import org.springframework.stereotype.Service; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.ResultActions; +import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RestController; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +/** + * The role markers through the real filter chain, on a probe that only this test sees. + */ +@AutoConfigureMockMvc +@Import({RoleCheckFlowTest.ProbeService.class, RoleCheckFlowTest.ProbeController.class}) +class RoleCheckFlowTest extends AbstractIntegrationTest { + + @Service + @RequiresRole(Role.USER) + static class ProbeService { + public String user() { + return "user"; + } + + @RequiresRole(Role.AUDITOR) + public String auditor() { + return "auditor"; + } + + @Public + public String open() { + return "open"; + } + } + + @RestController + static class ProbeController { + private final ProbeService probeService; + + ProbeController(ProbeService probeService) { + this.probeService = probeService; + } + + @GetMapping("/api/role-probe/user") + String user() { + return probeService.user(); + } + + @GetMapping("/api/role-probe/auditor") + String auditor() { + return probeService.auditor(); + } + + // Authenticated URL: the marker opens nothing the URL rules close. + @GetMapping("/api/role-probe/open") + String open() { + return probeService.open(); + } + + // Public URL: only the marker stops an anonymous caller. + @GetMapping("/api/public/role-probe/user") + String publicUser() { + return probeService.user(); + } + + @GetMapping("/api/public/role-probe/open") + String publicOpen() { + return probeService.open(); + } + } + + @Autowired + private MockMvc mockMvc; + + @Autowired + private ProbeService probeService; + + @Autowired + private TokenGenerator tokenGenerator; + + private String token(Role role) { + return tokenGenerator.generateToken("4711", "probe@example.com", null, role); + } + + private ResultActions call(String path, Role role) throws Exception { + MockHttpServletRequestBuilder request = get("/api/role-probe/" + path); + if (role != null) { + request.cookie(new Cookie(AuthCookies.ACCESS_COOKIE, token(role))); + } + return mockMvc.perform(request); + } + + @Test + void user_PassesUserAndIsRefusedAuditor() throws Exception { + call("user", Role.USER).andExpect(status().isOk()); + call("open", Role.USER).andExpect(status().isOk()); + call("auditor", Role.USER).andExpect(status().isForbidden()); + } + + @Test + void higherRoles_PassTheLowerChecks() throws Exception { + call("auditor", Role.AUDITOR).andExpect(status().isOk()); + call("auditor", Role.PLATFORM_ADMIN).andExpect(status().isOk()); + call("user", Role.PLATFORM_ADMIN).andExpect(status().isOk()); + } + + @Test + void noLogin_OnAPublicUrl_IsStoppedByTheMarker() throws Exception { + mockMvc.perform(get("/api/public/role-probe/user")).andExpect(status().isUnauthorized()); + mockMvc.perform(get("/api/public/role-probe/open")).andExpect(status().isOk()); + } + + @Test + void outsideARequest_TheServiceRuns() { + assertEquals("auditor", probeService.auditor()); + } +} diff --git a/src/test/java/auth_service/modules/account/service/AccountServiceRoleTest.java b/src/test/java/auth_service/modules/account/service/AccountServiceRoleTest.java new file mode 100644 index 0000000..3567403 --- /dev/null +++ b/src/test/java/auth_service/modules/account/service/AccountServiceRoleTest.java @@ -0,0 +1,39 @@ +package auth_service.modules.account.service; + +import auth_service.shared.account.AccountDeleter; +import auth_service.shared.test.AbstractIntegrationTest; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.authorization.AuthorizationDeniedException; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.web.context.request.RequestContextHolder; +import org.springframework.web.context.request.ServletRequestAttributes; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +class AccountServiceRoleTest extends AbstractIntegrationTest { + + @Autowired + private AccountDeleter accountDeleter; + + @AfterEach + void clear() { + RequestContextHolder.resetRequestAttributes(); + SecurityContextHolder.clearContext(); + } + + @Test + void deleteByAdmin_RefusesAUserLogin() { + RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(new MockHttpServletRequest())); + SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken("42", null, + List.of(new SimpleGrantedAuthority("ROLE_USER")))); + + assertThrows(AuthorizationDeniedException.class, () -> accountDeleter.deleteByAdmin(999_999L)); + } +}