feat(sso): sso mapping teant scoped (#6370)#6371
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the SSO group-mapping flow to support tenant-scoped group lookup/auto-creation by resolving an effective tenantId (mapping-level tenantId overrides provider-level openaev.provider.<registrationId>.tenant_id). It also adjusts call sites and adds regression tests for “multiple IdP groups map to the same OpenAEV group” behavior and removal semantics.
Changes:
- Extend
UserMappingService.mapCurrentUserWithGroup(...)to acceptregistrationIdand resolve an effective tenant for group lookup / auto-create / user tenant attachment. - Update
SecurityServiceto call the newUserMappingServicesignature. - Add new
UserMappingServiceTestcoverage for multiple mappings targeting the same user group and for removal behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
openaev-api/src/main/java/io/openaev/service/UserMappingService.java |
Adds tenant-aware group lookup/creation and improves mapping/removal behavior (but current implementation has tenant-scope correctness issues). |
openaev-api/src/main/java/io/openaev/config/security/SecurityService.java |
Updates call to the new mapCurrentUserWithGroup method signature. |
openaev-api/src/test/java/io/openaev/sso/UserMappingServiceTest.java |
Updates existing tests for new signature and adds new tests for multi-mapping and removal scenarios. |
Comments suppressed due to low confidence (2)
openaev-api/src/main/java/io/openaev/service/UserMappingService.java:84
- issue (blocking): The existing-group check deduplicates by group name, but groups are dual-scope and names may collide across tenants/platform. This can lead to skipping a needed add or adding duplicates incorrectly. Prefer using entity equality (id-based equals/hashCode) via userGroups.contains(group) instead of comparing names.
List<Group> userGroups = user.getUnscopedGroups();
List<Group> existing =
userGroups.stream()
.filter(userG -> userG.getName().equals(groupOptional.get().getName()))
.toList();
if (existing.isEmpty()) {
userGroups.add(groupOptional.get());
user.setGroups(userGroups);
}
openaev-api/src/main/java/io/openaev/service/UserMappingService.java:123
- issue (blocking): The removal logic and the "anyMappingMatchesForSameGroup" check are based only on userGroup name, ignoring tenant scoping. With dual-scope groups, this can remove (or keep) the wrong group when the same name exists in multiple tenants/platform, and it can also prevent removal because a different-tenant mapping matched. Include the effective tenantId in both the "matches" computation and the group-to-remove predicate.
boolean anyMappingMatchesForSameGroup =
groupMappings.stream()
.filter(m -> m.getUserGroup().equals(mapping.getUserGroup())) // do we have several mapping with same group
.anyMatch(m -> groupsFromToken.contains(m.getIdpGroup()));
if (!anyMappingMatchesForSameGroup
&& user.getUnscopedGroups().stream()
.anyMatch(groupOfUser -> groupOfUser.getName().equals(mapping.getUserGroup()))) {
// It means the user was removed from the group in the identity provider -> we remove it
// from its current groups
List<Group> userGroups = user.getUnscopedGroups();
userGroups.removeIf(group -> group.getName().equals(mapping.getUserGroup()));
user.setGroups(userGroups);
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (2.99%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6371 +/- ##
============================================
+ Coverage 45.14% 45.22% +0.07%
- Complexity 7755 7786 +31
============================================
Files 2332 2333 +1
Lines 64652 64729 +77
Branches 8554 8571 +17
============================================
+ Hits 29188 29272 +84
+ Misses 33610 33590 -20
- Partials 1854 1867 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
| attachTenantFromGroupMapping(mapping, user); | ||
| attachTenantToUser(tenantId, user); |
There was a problem hiding this comment.
I think we have a bigger problem here, tenant ID in groups_management should not be present. It's a mistake..
There was a problem hiding this comment.
removed reading tenant ID from groups_management in 8c8717f
|
@copilot resolve the merge conflicts in this pull request |
Resolved the merge conflict in |
…openaev into issue-6370/sso-mapping
Proposed changes
This pull request refactors and enhances the SSO group mapping logic in
UserMappingService, improving configurability, maintainability, and test coverage. The main changes include extracting provider property reading logic into a new helper class, supporting group scoping (tenant/platform) via configuration, and adding comprehensive unit tests.SSO Group Mapping Improvements:
UserMappingServiceto use a newReadPropertiesHelperfor reading provider configuration0)user_scopeproperty per provider. The mapping now handles auto-creation of groups in the correct scope and removes groups when users are removed from the IDP.Testing Instructions
Related issues
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...