From 215bba39bc93c516c9b4a6f35bfced821ddd101b Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Thu, 18 Jun 2026 17:27:22 +0100 Subject: [PATCH] Give SaaS users their own team and harden the user list endpoint (#6717) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description of Changes Previously, new SaaS users were placed on a shared Default team and then migrated to their own. A race (or a failed migration, or an anonymous→registered upgrade) could leave them stuck on that shared team, where unrelated users could see each other Instead they now get their own personal team during creation so unrelated users no longer collide on one team. SaaS-only (@Profile("saas")); self-host's Default behaviour is untouched. Also happens during call to avoid uncaught users Scope GET /api/v1/user/users. Anonymous callers get 403; a caller on a system team (Default/Internal) gets only themselves, not the team's members. --- ## Checklist ### General - [ ] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [ ] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [ ] I have performed a self-review of my own code - [ ] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### Translations (if applicable) - [ ] I ran [`scripts/counter_translation.py`](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/docs/counter_translation.md) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have run `task check` to verify linters, typechecks, and tests pass - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md#7-testing) for more details. --------- Co-authored-by: EthanHealy01 <80844253+EthanHealy01@users.noreply.github.com> --- .../controller/api/UserController.java | 33 +++++++++--- .../controller/api/UserControllerTest.java | 51 ++++++++++++++++++- .../SupabaseAuthenticationFilter.java | 11 ++-- .../saas/service/SaasTeamService.java | 10 ++++ .../saas/service/SaasUserAccountService.java | 3 ++ .../SupabaseAuthenticationFilterTest.java | 8 ++- .../service/SaasUserAccountServiceTest.java | 4 ++ 7 files changed, 101 insertions(+), 19 deletions(-) diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/UserController.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/UserController.java index 509de7e811..d8500e475f 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/UserController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/UserController.java @@ -978,28 +978,33 @@ public class UserController { } } - // Lists enabled users for the signing user picker, scoped by storage.signing.userListScope: - // 'org' (default) = whole instance, anything else = caller's team only (fail-closed). + // Lists enabled users for the signing picker; 'org' scope = instance-wide, else caller's team. @GetMapping("/users") public ResponseEntity> listUsers(Principal principal) { if (principal == null) { return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); } + Optional callerOpt = userService.findByUsernameIgnoreCase(principal.getName()); + + // Anonymous (SaaS) accounts must never enumerate users, in any scope or team. + if (callerOpt.map(UserController::isAnonymousUser).orElse(false)) { + return ResponseEntity.status(HttpStatus.FORBIDDEN).build(); + } + // Fail-closed: only literal "org" opens the whole instance; anything else scopes to team. String scope = applicationProperties.getStorage().getSigning().getUserListScope(); boolean teamScoped = !"org".equalsIgnoreCase(scope == null ? "" : scope.trim()); List source; if (teamScoped) { - Optional callerOpt = userService.findByUsernameIgnoreCase(principal.getName()); - if (callerOpt.isEmpty() || callerOpt.get().getTeam() == null) { - // No team: return only the caller rather than leak the org. + Team callerTeam = callerOpt.map(User::getTeam).orElse(null); + if (callerTeam == null || isSystemTeam(callerTeam)) { + // No team or a shared system team: return only the caller, not the team's members. source = callerOpt.map(List::of).orElse(List.of()); } else { - // KNOWN LIMITATION: scopes the team via the single User.team FK - correct while - // acceptInvitation() collapses users to one team; revisit if multi-team enabled. - source = userRepository.findAllByTeamId(callerOpt.get().getTeam().getId()); + // Scopes via the single User.team FK; revisit if multi-team membership is added. + source = userRepository.findAllByTeamId(callerTeam.getId()); } } else { source = userRepository.findAll(); @@ -1011,6 +1016,18 @@ public class UserController { return ResponseEntity.ok(users); } + // SaaS anonymous accounts, which must not enumerate users. + private static boolean isAnonymousUser(User user) { + return AuthenticationType.ANONYMOUS.name().equalsIgnoreCase(user.getAuthenticationType()); + } + + // System teams (Default/Internal) are not enumerable through the signing picker. + private static boolean isSystemTeam(Team team) { + String name = team.getName(); + return TeamService.DEFAULT_TEAM_NAME.equalsIgnoreCase(name) + || TeamService.INTERNAL_TEAM_NAME.equalsIgnoreCase(name); + } + private UserSummaryDTO toUserSummaryDTO(User user) { return new UserSummaryDTO( user.getId(), diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/UserControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/UserControllerTest.java index 61e4bf17ce..7bcc0da9f4 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/UserControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/UserControllerTest.java @@ -2,7 +2,6 @@ package stirling.software.proprietary.security.controller.api; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -28,6 +27,7 @@ import org.springframework.test.web.servlet.setup.MockMvcBuilders; import stirling.software.common.model.ApplicationProperties; import stirling.software.proprietary.model.Team; import stirling.software.proprietary.security.database.repository.UserRepository; +import stirling.software.proprietary.security.model.AuthenticationType; import stirling.software.proprietary.security.model.User; import stirling.software.proprietary.security.model.api.user.UsernameAndPass; import stirling.software.proprietary.security.repository.TeamRepository; @@ -195,8 +195,22 @@ class UserControllerTest { .andExpect(jsonPath("$[0].username").value("a@alpha.com")) .andExpect(jsonPath("$[1].username").value("b@alpha.com")); + // Caller is resolved (for the anonymous-gate) but org scope still uses findAll, not team. + verify(userRepository, never()).findAllByTeamId(any()); + } + + @Test + void listUsersForbiddenForAnonymousCaller() throws Exception { + // Anonymous SaaS accounts must never enumerate users, regardless of scope. + User anon = user(1L, "anon_abc", true, team(1L, TeamService.DEFAULT_TEAM_NAME)); + anon.setAuthenticationType(AuthenticationType.ANONYMOUS); + when(userService.findByUsernameIgnoreCase("anon_abc")).thenReturn(Optional.of(anon)); + + mockMvc.perform(get("/api/v1/user/users").principal(auth("anon_abc"))) + .andExpect(status().isForbidden()); + + verify(userRepository, never()).findAll(); verify(userRepository, never()).findAllByTeamId(any()); - verify(userService, never()).findByUsernameIgnoreCase(anyString()); } @Test @@ -262,6 +276,39 @@ class UserControllerTest { verify(userRepository, never()).findAll(); } + @Test + void listUsersTeamScopeOnDefaultTeamReturnsSelfOnly() throws Exception { + // A caller on a shared system team must not enumerate its members. + applicationProperties.getStorage().getSigning().setUserListScope("team"); + Team defaultTeam = team(1L, TeamService.DEFAULT_TEAM_NAME); + User caller = user(1L, "new@saas.com", true, defaultTeam); + when(userService.findByUsernameIgnoreCase("new@saas.com")).thenReturn(Optional.of(caller)); + + mockMvc.perform(get("/api/v1/user/users").principal(auth("new@saas.com"))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[0].username").value("new@saas.com")); + + verify(userRepository, never()).findAllByTeamId(any()); + verify(userRepository, never()).findAll(); + } + + @Test + void listUsersTeamScopeOnInternalTeamReturnsSelfOnly() throws Exception { + applicationProperties.getStorage().getSigning().setUserListScope("team"); + Team internalTeam = team(2L, TeamService.INTERNAL_TEAM_NAME); + User caller = user(1L, "svc@saas.com", true, internalTeam); + when(userService.findByUsernameIgnoreCase("svc@saas.com")).thenReturn(Optional.of(caller)); + + mockMvc.perform(get("/api/v1/user/users").principal(auth("svc@saas.com"))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[0].username").value("svc@saas.com")); + + verify(userRepository, never()).findAllByTeamId(any()); + verify(userRepository, never()).findAll(); + } + @Test void listUsersFailsClosedOnUnrecognisedScope() throws Exception { // Any non-"org" value must restrict to the caller's team, not leak the instance. diff --git a/app/saas/src/main/java/stirling/software/saas/security/SupabaseAuthenticationFilter.java b/app/saas/src/main/java/stirling/software/saas/security/SupabaseAuthenticationFilter.java index f1b2681e69..aa678f7cbf 100644 --- a/app/saas/src/main/java/stirling/software/saas/security/SupabaseAuthenticationFilter.java +++ b/app/saas/src/main/java/stirling/software/saas/security/SupabaseAuthenticationFilter.java @@ -262,7 +262,10 @@ public class SupabaseAuthenticationFilter extends OncePerRequestFilter { user.setUsername(supabaseUser.getEmail()); } try { - return userService.saveUser(user); + User saved = userService.saveUser(user); + // Give the account its own team rather than the shared Default team. + saved.setTeam(saasTeamService.ensurePersonalTeam(saved)); + return saved; } catch (DataIntegrityViolationException e) { log.warn( "Email collision upgrading anonymous user {} to {}: {}", @@ -344,7 +347,8 @@ public class SupabaseAuthenticationFilter extends OncePerRequestFilter { newUser.setEnabled(true); newUser.setFirstLogin(true); newUser.setRoleName(roleId); - newUser.setTeam(teamService.getOrCreateDefaultTeam()); + // No shared Default team; a per-user personal team is assigned after save (team_id + // nullable). newUser.setAuthenticationType(authenticationType); newUser.setSupabaseId(supabaseId); newUser.addAuthority(new Authority(roleId, newUser)); @@ -379,8 +383,7 @@ public class SupabaseAuthenticationFilter extends OncePerRequestFilter { // Only the DB-race winner runs first-time init; the losers skip it. if (weCreatedThisUser) { try { - saasTeamService.createPersonalTeam(savedUser); - savedUser = userService.findBySupabaseId(supabaseId).orElse(savedUser); + savedUser.setTeam(saasTeamService.ensurePersonalTeam(savedUser)); } catch (Exception e) { log.warn( "Failed to create personal team for new user {} ({}): {}", diff --git a/app/saas/src/main/java/stirling/software/saas/service/SaasTeamService.java b/app/saas/src/main/java/stirling/software/saas/service/SaasTeamService.java index 7c77c6a0e9..e54aa00df2 100644 --- a/app/saas/src/main/java/stirling/software/saas/service/SaasTeamService.java +++ b/app/saas/src/main/java/stirling/software/saas/service/SaasTeamService.java @@ -50,6 +50,16 @@ public class SaasTeamService { public static final String DEFAULT_TEAM_NAME = "Default"; public static final String INTERNAL_TEAM_NAME = "Internal"; + /** Returns the user's personal team, creating one if they have none. Idempotent. */ + @Transactional + public Team ensurePersonalTeam(User user) { + Team existing = user.getTeam(); + if (existing != null && saasTeamExtensionService.isPersonal(existing)) { + return existing; + } + return createPersonalTeam(user); + } + /** * Create personal team for new user during signup or migrate existing user from Default team * diff --git a/app/saas/src/main/java/stirling/software/saas/service/SaasUserAccountService.java b/app/saas/src/main/java/stirling/software/saas/service/SaasUserAccountService.java index c4b0ad769d..51ad9cab17 100644 --- a/app/saas/src/main/java/stirling/software/saas/service/SaasUserAccountService.java +++ b/app/saas/src/main/java/stirling/software/saas/service/SaasUserAccountService.java @@ -31,6 +31,7 @@ public class SaasUserAccountService { private final SupabaseUserService supabaseUserService; private final SaasUserExtensionService saasUserExtensionService; private final SaasTeamExtensionService saasTeamExtensionService; + private final SaasTeamService saasTeamService; /** * Resolve a local {@link User} from a Supabase UUID string. Throws if the ID format is invalid @@ -173,6 +174,8 @@ public class SaasUserAccountService { user.setUsername(email); } user = userService.saveUser(user); + // Give the upgraded user their own team rather than the shared Default team. + user.setTeam(saasTeamService.ensurePersonalTeam(user)); log.info( "Upgraded anonymous user {} to {} ({})", user.getId(), diff --git a/app/saas/src/test/java/stirling/software/saas/security/SupabaseAuthenticationFilterTest.java b/app/saas/src/test/java/stirling/software/saas/security/SupabaseAuthenticationFilterTest.java index 0069b9742e..e906655edc 100644 --- a/app/saas/src/test/java/stirling/software/saas/security/SupabaseAuthenticationFilterTest.java +++ b/app/saas/src/test/java/stirling/software/saas/security/SupabaseAuthenticationFilterTest.java @@ -30,7 +30,6 @@ import org.springframework.security.oauth2.jwt.Jwt; import org.springframework.security.oauth2.jwt.JwtDecoder; import org.springframework.security.oauth2.jwt.JwtException; -import stirling.software.proprietary.model.Team; import stirling.software.proprietary.security.model.ApiKeyAuthenticationToken; import stirling.software.proprietary.security.model.AuthenticationType; import stirling.software.proprietary.security.model.User; @@ -155,7 +154,6 @@ class SupabaseAuthenticationFilterTest { when(supabaseUserService.getUser(supabaseId)) .thenReturn(supabaseUserMatching(supabaseId, "bob@example.com", false)); when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty()); - when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team()); when(userService.saveUser(any())).thenAnswer(inv -> inv.getArgument(0)); request.setRequestURI("/api/v1/something"); @@ -166,6 +164,9 @@ class SupabaseAuthenticationFilterTest { verify(userService, times(1)).saveUser(any(User.class)); verify(supabaseUserService).createSupabaseUser(supabaseId, "bob@example.com", false); + // New users get their own personal team, never the shared Default team. + verify(saasTeamService).ensurePersonalTeam(any(User.class)); + verify(teamService, never()).getOrCreateDefaultTeam(); assertThat(SecurityContextHolder.getContext().getAuthentication()) .isInstanceOf(EnhancedJwtAuthenticationToken.class); } @@ -179,7 +180,6 @@ class SupabaseAuthenticationFilterTest { when(supabaseUserService.getUser(supabaseId)) .thenReturn(supabaseUserMatching(supabaseId, "carol@example.com", false)); when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty()); - when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team()); when(userService.saveUser(any(User.class))) .thenAnswer( inv -> { @@ -208,7 +208,6 @@ class SupabaseAuthenticationFilterTest { when(supabaseUserService.getUser(supabaseId)) .thenReturn(supabaseUserMatching(supabaseId, "dave@example.com", false)); when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty()); - when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team()); when(userService.saveUser(any(User.class))) .thenAnswer( inv -> { @@ -237,7 +236,6 @@ class SupabaseAuthenticationFilterTest { when(supabaseUserService.getUser(supabaseId)) .thenReturn(supabaseUserMatching(supabaseId, "eve@example.com", false)); when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty()); - when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team()); when(userService.saveUser(any(User.class))) .thenAnswer( inv -> { diff --git a/app/saas/src/test/java/stirling/software/saas/service/SaasUserAccountServiceTest.java b/app/saas/src/test/java/stirling/software/saas/service/SaasUserAccountServiceTest.java index 03efa06ab6..5bf0a33e4c 100644 --- a/app/saas/src/test/java/stirling/software/saas/service/SaasUserAccountServiceTest.java +++ b/app/saas/src/test/java/stirling/software/saas/service/SaasUserAccountServiceTest.java @@ -51,6 +51,7 @@ class SaasUserAccountServiceTest { @Mock private SupabaseUserService supabaseUserService; @Mock private SaasUserExtensionService saasUserExtensionService; @Mock private SaasTeamExtensionService saasTeamExtensionService; + @Mock private SaasTeamService saasTeamService; @InjectMocks private SaasUserAccountService service; @@ -355,6 +356,8 @@ class SaasUserAccountServiceTest { assertThat(u.getEmail()).isEqualTo("alice@example.com"); assertThat(u.getUsername()).isEqualTo("alice@example.com"); verify(userService).saveUser(u); + // Upgrading from anon gives the user their own team. + verify(saasTeamService).ensurePersonalTeam(u); } @Test @@ -455,6 +458,7 @@ class SaasUserAccountServiceTest { assertThat(u.getUsername()).isEqualTo("existing@example.com"); assertThat(u.getAuthenticationType()).isEqualTo("web"); verify(userService, never()).saveUser(any()); + verify(saasTeamService, never()).ensurePersonalTeam(any()); } @Test