Skip to content

Commit 215bba3

Browse files
Give SaaS users their own team and harden the user list endpoint (Stirling-Tools#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>
1 parent 900b66b commit 215bba3

7 files changed

Lines changed: 101 additions & 19 deletions

File tree

app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/UserController.java

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -978,28 +978,33 @@ public ResponseEntity<?> completeInitialSetup() {
978978
}
979979
}
980980

981-
// Lists enabled users for the signing user picker, scoped by storage.signing.userListScope:
982-
// 'org' (default) = whole instance, anything else = caller's team only (fail-closed).
981+
// Lists enabled users for the signing picker; 'org' scope = instance-wide, else caller's team.
983982
@GetMapping("/users")
984983
public ResponseEntity<List<UserSummaryDTO>> listUsers(Principal principal) {
985984
if (principal == null) {
986985
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
987986
}
988987

988+
Optional<User> callerOpt = userService.findByUsernameIgnoreCase(principal.getName());
989+
990+
// Anonymous (SaaS) accounts must never enumerate users, in any scope or team.
991+
if (callerOpt.map(UserController::isAnonymousUser).orElse(false)) {
992+
return ResponseEntity.status(HttpStatus.FORBIDDEN).build();
993+
}
994+
989995
// Fail-closed: only literal "org" opens the whole instance; anything else scopes to team.
990996
String scope = applicationProperties.getStorage().getSigning().getUserListScope();
991997
boolean teamScoped = !"org".equalsIgnoreCase(scope == null ? "" : scope.trim());
992998

993999
List<User> source;
9941000
if (teamScoped) {
995-
Optional<User> callerOpt = userService.findByUsernameIgnoreCase(principal.getName());
996-
if (callerOpt.isEmpty() || callerOpt.get().getTeam() == null) {
997-
// No team: return only the caller rather than leak the org.
1001+
Team callerTeam = callerOpt.map(User::getTeam).orElse(null);
1002+
if (callerTeam == null || isSystemTeam(callerTeam)) {
1003+
// No team or a shared system team: return only the caller, not the team's members.
9981004
source = callerOpt.map(List::of).orElse(List.of());
9991005
} else {
1000-
// KNOWN LIMITATION: scopes the team via the single User.team FK - correct while
1001-
// acceptInvitation() collapses users to one team; revisit if multi-team enabled.
1002-
source = userRepository.findAllByTeamId(callerOpt.get().getTeam().getId());
1006+
// Scopes via the single User.team FK; revisit if multi-team membership is added.
1007+
source = userRepository.findAllByTeamId(callerTeam.getId());
10031008
}
10041009
} else {
10051010
source = userRepository.findAll();
@@ -1011,6 +1016,18 @@ public ResponseEntity<List<UserSummaryDTO>> listUsers(Principal principal) {
10111016
return ResponseEntity.ok(users);
10121017
}
10131018

1019+
// SaaS anonymous accounts, which must not enumerate users.
1020+
private static boolean isAnonymousUser(User user) {
1021+
return AuthenticationType.ANONYMOUS.name().equalsIgnoreCase(user.getAuthenticationType());
1022+
}
1023+
1024+
// System teams (Default/Internal) are not enumerable through the signing picker.
1025+
private static boolean isSystemTeam(Team team) {
1026+
String name = team.getName();
1027+
return TeamService.DEFAULT_TEAM_NAME.equalsIgnoreCase(name)
1028+
|| TeamService.INTERNAL_TEAM_NAME.equalsIgnoreCase(name);
1029+
}
1030+
10141031
private UserSummaryDTO toUserSummaryDTO(User user) {
10151032
return new UserSummaryDTO(
10161033
user.getId(),

app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/UserControllerTest.java

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
44
import static org.mockito.ArgumentMatchers.any;
5-
import static org.mockito.ArgumentMatchers.anyString;
65
import static org.mockito.Mockito.never;
76
import static org.mockito.Mockito.verify;
87
import static org.mockito.Mockito.when;
@@ -28,6 +27,7 @@
2827
import stirling.software.common.model.ApplicationProperties;
2928
import stirling.software.proprietary.model.Team;
3029
import stirling.software.proprietary.security.database.repository.UserRepository;
30+
import stirling.software.proprietary.security.model.AuthenticationType;
3131
import stirling.software.proprietary.security.model.User;
3232
import stirling.software.proprietary.security.model.api.user.UsernameAndPass;
3333
import stirling.software.proprietary.security.repository.TeamRepository;
@@ -195,8 +195,22 @@ void listUsersDefaultScopeIsOrgWide() throws Exception {
195195
.andExpect(jsonPath("$[0].username").value("a@alpha.com"))
196196
.andExpect(jsonPath("$[1].username").value("b@alpha.com"));
197197

198+
// Caller is resolved (for the anonymous-gate) but org scope still uses findAll, not team.
199+
verify(userRepository, never()).findAllByTeamId(any());
200+
}
201+
202+
@Test
203+
void listUsersForbiddenForAnonymousCaller() throws Exception {
204+
// Anonymous SaaS accounts must never enumerate users, regardless of scope.
205+
User anon = user(1L, "anon_abc", true, team(1L, TeamService.DEFAULT_TEAM_NAME));
206+
anon.setAuthenticationType(AuthenticationType.ANONYMOUS);
207+
when(userService.findByUsernameIgnoreCase("anon_abc")).thenReturn(Optional.of(anon));
208+
209+
mockMvc.perform(get("/api/v1/user/users").principal(auth("anon_abc")))
210+
.andExpect(status().isForbidden());
211+
212+
verify(userRepository, never()).findAll();
198213
verify(userRepository, never()).findAllByTeamId(any());
199-
verify(userService, never()).findByUsernameIgnoreCase(anyString());
200214
}
201215

202216
@Test
@@ -262,6 +276,39 @@ void listUsersTeamScopeWithNullTeamReturnsSelfOnly() throws Exception {
262276
verify(userRepository, never()).findAll();
263277
}
264278

279+
@Test
280+
void listUsersTeamScopeOnDefaultTeamReturnsSelfOnly() throws Exception {
281+
// A caller on a shared system team must not enumerate its members.
282+
applicationProperties.getStorage().getSigning().setUserListScope("team");
283+
Team defaultTeam = team(1L, TeamService.DEFAULT_TEAM_NAME);
284+
User caller = user(1L, "new@saas.com", true, defaultTeam);
285+
when(userService.findByUsernameIgnoreCase("new@saas.com")).thenReturn(Optional.of(caller));
286+
287+
mockMvc.perform(get("/api/v1/user/users").principal(auth("new@saas.com")))
288+
.andExpect(status().isOk())
289+
.andExpect(jsonPath("$.length()").value(1))
290+
.andExpect(jsonPath("$[0].username").value("new@saas.com"));
291+
292+
verify(userRepository, never()).findAllByTeamId(any());
293+
verify(userRepository, never()).findAll();
294+
}
295+
296+
@Test
297+
void listUsersTeamScopeOnInternalTeamReturnsSelfOnly() throws Exception {
298+
applicationProperties.getStorage().getSigning().setUserListScope("team");
299+
Team internalTeam = team(2L, TeamService.INTERNAL_TEAM_NAME);
300+
User caller = user(1L, "svc@saas.com", true, internalTeam);
301+
when(userService.findByUsernameIgnoreCase("svc@saas.com")).thenReturn(Optional.of(caller));
302+
303+
mockMvc.perform(get("/api/v1/user/users").principal(auth("svc@saas.com")))
304+
.andExpect(status().isOk())
305+
.andExpect(jsonPath("$.length()").value(1))
306+
.andExpect(jsonPath("$[0].username").value("svc@saas.com"));
307+
308+
verify(userRepository, never()).findAllByTeamId(any());
309+
verify(userRepository, never()).findAll();
310+
}
311+
265312
@Test
266313
void listUsersFailsClosedOnUnrecognisedScope() throws Exception {
267314
// Any non-"org" value must restrict to the caller's team, not leak the instance.

app/saas/src/main/java/stirling/software/saas/security/SupabaseAuthenticationFilter.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,10 @@ protected User upgradeAnonymousUser(User user, SupabaseUser supabaseUser, Jwt jw
262262
user.setUsername(supabaseUser.getEmail());
263263
}
264264
try {
265-
return userService.saveUser(user);
265+
User saved = userService.saveUser(user);
266+
// Give the account its own team rather than the shared Default team.
267+
saved.setTeam(saasTeamService.ensurePersonalTeam(saved));
268+
return saved;
266269
} catch (DataIntegrityViolationException e) {
267270
log.warn(
268271
"Email collision upgrading anonymous user {} to {}: {}",
@@ -344,7 +347,8 @@ protected User createUser(
344347
newUser.setEnabled(true);
345348
newUser.setFirstLogin(true);
346349
newUser.setRoleName(roleId);
347-
newUser.setTeam(teamService.getOrCreateDefaultTeam());
350+
// No shared Default team; a per-user personal team is assigned after save (team_id
351+
// nullable).
348352
newUser.setAuthenticationType(authenticationType);
349353
newUser.setSupabaseId(supabaseId);
350354
newUser.addAuthority(new Authority(roleId, newUser));
@@ -379,8 +383,7 @@ protected User createUser(
379383
// Only the DB-race winner runs first-time init; the losers skip it.
380384
if (weCreatedThisUser) {
381385
try {
382-
saasTeamService.createPersonalTeam(savedUser);
383-
savedUser = userService.findBySupabaseId(supabaseId).orElse(savedUser);
386+
savedUser.setTeam(saasTeamService.ensurePersonalTeam(savedUser));
384387
} catch (Exception e) {
385388
log.warn(
386389
"Failed to create personal team for new user {} ({}): {}",

app/saas/src/main/java/stirling/software/saas/service/SaasTeamService.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,16 @@ public class SaasTeamService {
5050
public static final String DEFAULT_TEAM_NAME = "Default";
5151
public static final String INTERNAL_TEAM_NAME = "Internal";
5252

53+
/** Returns the user's personal team, creating one if they have none. Idempotent. */
54+
@Transactional
55+
public Team ensurePersonalTeam(User user) {
56+
Team existing = user.getTeam();
57+
if (existing != null && saasTeamExtensionService.isPersonal(existing)) {
58+
return existing;
59+
}
60+
return createPersonalTeam(user);
61+
}
62+
5363
/**
5464
* Create personal team for new user during signup or migrate existing user from Default team
5565
*

app/saas/src/main/java/stirling/software/saas/service/SaasUserAccountService.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ public class SaasUserAccountService {
3131
private final SupabaseUserService supabaseUserService;
3232
private final SaasUserExtensionService saasUserExtensionService;
3333
private final SaasTeamExtensionService saasTeamExtensionService;
34+
private final SaasTeamService saasTeamService;
3435

3536
/**
3637
* Resolve a local {@link User} from a Supabase UUID string. Throws if the ID format is invalid
@@ -173,6 +174,8 @@ public User synchronizeUserUpgrade(SupabaseUser supabaseUser, String email, Stri
173174
user.setUsername(email);
174175
}
175176
user = userService.saveUser(user);
177+
// Give the upgraded user their own team rather than the shared Default team.
178+
user.setTeam(saasTeamService.ensurePersonalTeam(user));
176179
log.info(
177180
"Upgraded anonymous user {} to {} ({})",
178181
user.getId(),

app/saas/src/test/java/stirling/software/saas/security/SupabaseAuthenticationFilterTest.java

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import org.springframework.security.oauth2.jwt.JwtDecoder;
3131
import org.springframework.security.oauth2.jwt.JwtException;
3232

33-
import stirling.software.proprietary.model.Team;
3433
import stirling.software.proprietary.security.model.ApiKeyAuthenticationToken;
3534
import stirling.software.proprietary.security.model.AuthenticationType;
3635
import stirling.software.proprietary.security.model.User;
@@ -155,7 +154,6 @@ void validJwtForNewUserTriggersUserCreation() throws Exception {
155154
when(supabaseUserService.getUser(supabaseId))
156155
.thenReturn(supabaseUserMatching(supabaseId, "bob@example.com", false));
157156
when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty());
158-
when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team());
159157
when(userService.saveUser(any())).thenAnswer(inv -> inv.getArgument(0));
160158

161159
request.setRequestURI("/api/v1/something");
@@ -166,6 +164,9 @@ void validJwtForNewUserTriggersUserCreation() throws Exception {
166164

167165
verify(userService, times(1)).saveUser(any(User.class));
168166
verify(supabaseUserService).createSupabaseUser(supabaseId, "bob@example.com", false);
167+
// New users get their own personal team, never the shared Default team.
168+
verify(saasTeamService).ensurePersonalTeam(any(User.class));
169+
verify(teamService, never()).getOrCreateDefaultTeam();
169170
assertThat(SecurityContextHolder.getContext().getAuthentication())
170171
.isInstanceOf(EnhancedJwtAuthenticationToken.class);
171172
}
@@ -179,7 +180,6 @@ void appleProviderClassifiedAsOauth2NotWeb() throws Exception {
179180
when(supabaseUserService.getUser(supabaseId))
180181
.thenReturn(supabaseUserMatching(supabaseId, "carol@example.com", false));
181182
when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty());
182-
when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team());
183183
when(userService.saveUser(any(User.class)))
184184
.thenAnswer(
185185
inv -> {
@@ -208,7 +208,6 @@ void azureProviderClassifiedAsOauth2NotWeb() throws Exception {
208208
when(supabaseUserService.getUser(supabaseId))
209209
.thenReturn(supabaseUserMatching(supabaseId, "dave@example.com", false));
210210
when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty());
211-
when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team());
212211
when(userService.saveUser(any(User.class)))
213212
.thenAnswer(
214213
inv -> {
@@ -237,7 +236,6 @@ void emailProviderClassifiedAsWeb() throws Exception {
237236
when(supabaseUserService.getUser(supabaseId))
238237
.thenReturn(supabaseUserMatching(supabaseId, "eve@example.com", false));
239238
when(userService.findBySupabaseId(supabaseId)).thenReturn(Optional.empty());
240-
when(teamService.getOrCreateDefaultTeam()).thenReturn(new Team());
241239
when(userService.saveUser(any(User.class)))
242240
.thenAnswer(
243241
inv -> {

app/saas/src/test/java/stirling/software/saas/service/SaasUserAccountServiceTest.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ class SaasUserAccountServiceTest {
5151
@Mock private SupabaseUserService supabaseUserService;
5252
@Mock private SaasUserExtensionService saasUserExtensionService;
5353
@Mock private SaasTeamExtensionService saasTeamExtensionService;
54+
@Mock private SaasTeamService saasTeamService;
5455

5556
@InjectMocks private SaasUserAccountService service;
5657

@@ -355,6 +356,8 @@ void anonymousLocalUser_promotedToWeb() {
355356
assertThat(u.getEmail()).isEqualTo("alice@example.com");
356357
assertThat(u.getUsername()).isEqualTo("alice@example.com");
357358
verify(userService).saveUser(u);
359+
// Upgrading from anon gives the user their own team.
360+
verify(saasTeamService).ensurePersonalTeam(u);
358361
}
359362

360363
@Test
@@ -455,6 +458,7 @@ void nonAnonymousLocalUser_untouched() {
455458
assertThat(u.getUsername()).isEqualTo("existing@example.com");
456459
assertThat(u.getAuthenticationType()).isEqualTo("web");
457460
verify(userService, never()).saveUser(any());
461+
verify(saasTeamService, never()).ensurePersonalTeam(any());
458462
}
459463

460464
@Test

0 commit comments

Comments
 (0)