mirror of
https://github.com/Stirling-Tools/Stirling-PDF.git
synced 2026-09-03 05:10:16 +03:00
Give SaaS users their own team and harden the user list endpoint (#6717)
# 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>
This commit is contained in:
Anthony Stirling
co-authored by
EthanHealy01
EthanHealy01
parent
900b66b030
commit
215bba39bc
+25
-8
@@ -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<List<UserSummaryDTO>> listUsers(Principal principal) {
|
||||
if (principal == null) {
|
||||
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
|
||||
}
|
||||
|
||||
Optional<User> 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<User> source;
|
||||
if (teamScoped) {
|
||||
Optional<User> 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(),
|
||||
|
||||
+49
-2
@@ -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.
|
||||
|
||||
+7
-4
@@ -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 {} ({}): {}",
|
||||
|
||||
@@ -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
|
||||
*
|
||||
|
||||
@@ -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(),
|
||||
|
||||
+3
-5
@@ -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 -> {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user