-
Notifications
You must be signed in to change notification settings - Fork 165
#4941 - Mapping OIDC groups to INCEpTION's internal roles #5453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…efore cleaning and refactoring the feature
Updating branch with latest devs from main
Updated with latest main commits
… a dedicated file
- OAuth2 Role Claim is now congigurable - Updated config prefix for the feature to 'security.oauth.roles'
- Updated branch with latest commits from `main`
- Try figuring out why the PR build does not work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements OAuth2 group-to-role mapping functionality for INCEpTION, allowing OAuth2/OIDC groups from identity providers to be mapped to INCEpTION's internal roles (ADMIN, USER, PROJECT_CREATOR, REMOTE). The feature is configurable via settings.properties and can be enabled/disabled. When disabled, users default to the USER role as before.
Key changes:
- Introduced new
OAuth2Utilsclass to handle OAuth2 group-to-role mapping with configurable properties - Modified
OAuth2AdapterImplto use the new role mapping logic instead of PreAuthUtils - Added comprehensive unit tests for the role mapping functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
inception/inception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2Utils.java |
New configuration properties class handling OAuth2 group-to-role mapping logic |
inception/inception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2AdapterImpl.java |
Updated to use OAuth2Utils for role assignment during user creation and login |
inception/inception-security/src/test/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2UtilsTest.java |
New test class with unit tests for role mapping scenarios |
.github/workflows/maven.yml |
Added Git debug trace configuration (appears to be leftover debugging) |
.gitattributes |
Added binary file type specifications for gif, pptx, and zip files |
inception/inception-app-webapp/src/test/java/de/tudarmstadt/ukp/inception/db/InceptionHsqldbIntegrationTest.java |
Minor formatting changes with trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (StringUtils.equals(oauth2group, OAUTH2_ADMIN_ROLE)) { | ||
| userRoles.add(Role.ROLE_ADMIN); | ||
| } | ||
|
|
||
| if (StringUtils.equals(oauth2group, OAUTH2_USER_ROLE)) { | ||
| userRoles.add(Role.ROLE_USER); | ||
| } | ||
|
|
||
| if (StringUtils.equals(oauth2group, OAUTH2_PROJECT_CREATOR_ROLE)) { | ||
| userRoles.add(Role.ROLE_PROJECT_CREATOR); | ||
| } | ||
|
|
||
| if (StringUtils.equals(oauth2group, OAUTH2_REMOTE_ROLE)) { |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matchOauth2groupToRole method uses multiple independent if statements, which means it will only match the first configured role even if empty strings are configured for other roles. Since StringUtils.equals returns true when both arguments are empty strings, an empty configuration value (e.g., security.oauth.roles.admin=) will match and add a role incorrectly.
Consider adding null/empty checks before the StringUtils.equals calls:
if (StringUtils.isNotBlank(OAUTH2_ADMIN_ROLE) && StringUtils.equals(oauth2group, OAUTH2_ADMIN_ROLE)) {
userRoles.add(Role.ROLE_ADMIN);
}| if (StringUtils.equals(oauth2group, OAUTH2_ADMIN_ROLE)) { | |
| userRoles.add(Role.ROLE_ADMIN); | |
| } | |
| if (StringUtils.equals(oauth2group, OAUTH2_USER_ROLE)) { | |
| userRoles.add(Role.ROLE_USER); | |
| } | |
| if (StringUtils.equals(oauth2group, OAUTH2_PROJECT_CREATOR_ROLE)) { | |
| userRoles.add(Role.ROLE_PROJECT_CREATOR); | |
| } | |
| if (StringUtils.equals(oauth2group, OAUTH2_REMOTE_ROLE)) { | |
| if (StringUtils.isNotBlank(OAUTH2_ADMIN_ROLE) && StringUtils.equals(oauth2group, OAUTH2_ADMIN_ROLE)) { | |
| userRoles.add(Role.ROLE_ADMIN); | |
| } | |
| if (StringUtils.isNotBlank(OAUTH2_USER_ROLE) && StringUtils.equals(oauth2group, OAUTH2_USER_ROLE)) { | |
| userRoles.add(Role.ROLE_USER); | |
| } | |
| if (StringUtils.isNotBlank(OAUTH2_PROJECT_CREATOR_ROLE) && StringUtils.equals(oauth2group, OAUTH2_PROJECT_CREATOR_ROLE)) { | |
| userRoles.add(Role.ROLE_PROJECT_CREATOR); | |
| } | |
| if (StringUtils.isNotBlank(OAUTH2_REMOTE_ROLE) && StringUtils.equals(oauth2group, OAUTH2_REMOTE_ROLE)) { |
| try { | ||
| OAuth2Utils.getOAuth2UserRoles(testUser, testRetievedOAuth2User); | ||
| } catch (AccessDeniedException ade) { | ||
| System.out.println(ade.getClass().getSimpleName() + " was thrown"); |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove debug print statement from test. Use proper logging or assertions instead of System.out.println.
| System.out.println(ade.getClass().getSimpleName() + " was thrown"); |
| env: | ||
| GIT_TRACE: 1 | ||
| GIT_CURL_VERBOSE: 1 |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] These debug environment variables (GIT_TRACE and GIT_CURL_VERBOSE) appear to be leftover debugging configuration and should be removed before merging. They increase log verbosity unnecessarily in the CI pipeline.
| env: | |
| GIT_TRACE: 1 | |
| GIT_CURL_VERBOSE: 1 |
| @Test | ||
| void thatAdminRoleIsGivenIfMatchingGroupFound() | ||
| { | ||
| ArrayList<String> userOAuth2Groups = new ArrayList<>(); | ||
| userOAuth2Groups.add(OAUTH2_GROUP_ADMIN); | ||
| when(testRetievedOAuth2User.getAttribute(anyString())).thenReturn(userOAuth2Groups); | ||
|
|
||
| Set<Role> userRoles = OAuth2Utils.getOAuth2UserRoles(testUser, testRetievedOAuth2User); | ||
|
|
||
| assertTrue(userRoles.contains(Role.ROLE_ADMIN)); | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests don't cover the scenario where security.oauth.roles.enabled=false (the default behavior). Add a test to verify that when role mapping is disabled, users are assigned the default ROLE_USER role regardless of their OAuth2 groups.
| } | ||
|
|
||
| @Test | ||
| void thatUnauthorizedExceptionIsThrownIfNoRoleIsMapped() { |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name thatUnauthorizedExceptionIsThrownIfNoRoleIsMapped is inaccurate. The code throws AccessDeniedException, not an "Unauthorized" exception. Consider renaming to thatAccessDeniedExceptionIsThrownIfNoRoleIsMapped or thatExceptionIsThrownWhenUserHasNoGroups.
| void thatUnauthorizedExceptionIsThrownIfNoRoleIsMapped() { | |
| void thatAccessDeniedExceptionIsThrownIfNoRoleIsMapped() { |
|
|
||
| @Configuration | ||
| @ConfigurationProperties(prefix = "security.oauth.roles") | ||
| public class OAuth2Utils { |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class name OAuth2Utils suggests a utility class with static helper methods, but it's actually a Spring configuration properties class. Consider renaming to follow the codebase convention of appending PropertiesImpl (e.g., OAuth2RolesPropertiesImpl) and having it implement an interface (e.g., OAuth2RolesProperties), as seen with LoginPropertiesImpl, SecurityPropertiesImpl, etc.
| try { | ||
| OAuth2Utils.getOAuth2UserRoles(testUser, testRetievedOAuth2User); | ||
| } catch (AccessDeniedException ade) { | ||
| System.out.println(ade.getClass().getSimpleName() + " was thrown"); | ||
| return; | ||
| } | ||
|
|
||
| fail("Expected Exception wasn't catched"); | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a try-catch block with fail(), use JUnit 5's assertThrows for cleaner exception testing:
assertThrows(AccessDeniedException.class, () ->
OAuth2Utils.getOAuth2UserRoles(testUser, testRetievedOAuth2User)
);This is more idiomatic and removes the need for the return statement and fail call.
|
|
||
| if (oauth2groups == null || oauth2groups.isEmpty()) { | ||
| throw new AccessDeniedException("OAuth2 roles mapping is enabled, but user [" | ||
| + aUser.getUsername() + "] doesn't have any roles, or the corresponding claim is empty"); |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is ambiguous. It states "doesn't have any roles" but what it actually means is "doesn't have the groups claim or the claim is empty". Consider making it clearer:
throw new AccessDeniedException("OAuth2 roles mapping is enabled, but user ["
+ aUser.getUsername() + "] has no groups in the '" + OAUTH2_ROLES_CLAIM + "' claim");| + aUser.getUsername() + "] doesn't have any roles, or the corresponding claim is empty"); | |
| + aUser.getUsername() + "] does not have the required claim '" + OAUTH2_ROLES_CLAIM | |
| + "', or the claim is empty"); |
| @Test | ||
| void thatAdminRoleIsGivenIfMatchingGroupFound() | ||
| { | ||
| ArrayList<String> userOAuth2Groups = new ArrayList<>(); | ||
| userOAuth2Groups.add(OAUTH2_GROUP_ADMIN); | ||
| when(testRetievedOAuth2User.getAttribute(anyString())).thenReturn(userOAuth2Groups); | ||
|
|
||
| Set<Role> userRoles = OAuth2Utils.getOAuth2UserRoles(testUser, testRetievedOAuth2User); | ||
|
|
||
| assertTrue(userRoles.contains(Role.ROLE_ADMIN)); | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test to verify that a user can have multiple roles when they belong to multiple OAuth2 groups (e.g., both ROLE_USER and ROLE_PROJECT_CREATOR). This is an important use case that should be explicitly tested.
| @Configuration | ||
| @ConfigurationProperties(prefix = "security.oauth.roles") | ||
| public class OAuth2Utils { | ||
|
|
||
| private static boolean OAUTH2_ROLES_ENABLED; | ||
| private static String OAUTH2_ROLES_CLAIM; | ||
| private static String OAUTH2_ADMIN_ROLE; | ||
| private static String OAUTH2_USER_ROLE; | ||
| private static String OAUTH2_PROJECT_CREATOR_ROLE; | ||
| private static String OAUTH2_REMOTE_ROLE; | ||
|
|
||
| @Value("${security.oauth.roles.enabled:false}") | ||
| public void setOAuth2RolesEnabled(boolean oAuth2RolesEnabled) | ||
| { | ||
| OAUTH2_ROLES_ENABLED = oAuth2RolesEnabled; | ||
| } | ||
|
|
||
| @Value("${security.oauth.roles.claim:groups}") | ||
| public void setOAuth2RolesClaim(String oAuth2RolesClaim) | ||
| { | ||
| OAUTH2_ROLES_CLAIM = oAuth2RolesClaim; | ||
| } | ||
|
|
||
| @Value("${security.oauth.roles.admin:}") | ||
| public void setAdminRole(String adminRole) | ||
| { | ||
| OAUTH2_ADMIN_ROLE = adminRole; | ||
| } | ||
|
|
||
| @Value("${security.oauth.roles.user:}") | ||
| public void setUserRole(String userRole) | ||
| { | ||
| OAUTH2_USER_ROLE = userRole; | ||
| } | ||
|
|
||
| @Value("${security.oauth.roles.project-creator:}") | ||
| public void setProjectCreatorRole(String projectCreatorRole) | ||
| { | ||
| OAUTH2_PROJECT_CREATOR_ROLE = projectCreatorRole; | ||
| } | ||
|
|
||
| @Value("${security.oauth.roles.remote:}") | ||
| public void setRemoteRole(String remoteRole) | ||
| { | ||
| OAUTH2_REMOTE_ROLE = remoteRole; | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OAuth2Utils class has several architectural issues:
-
Anti-pattern: Using static fields with Spring configuration: The class uses both
@Configurationand@ConfigurationPropertiesannotations with static fields. Static fields are set via instance methods (setters), which works but is not thread-safe and violates Spring best practices. Configuration properties classes should use instance fields. -
Redundant annotations: Both
@Valueand@ConfigurationPropertiesare used together, which is unnecessary. When using@ConfigurationProperties(prefix = "security.oauth.roles"), Spring automatically binds properties to instance fields via setters without needing@Valueannotations. -
Missing from SecurityAutoConfiguration: The class is not registered in
SecurityAutoConfiguration.java's@EnableConfigurationPropertieslist, which means it may not be properly initialized as a Spring configuration properties bean.
Recommended approach:
- Remove the
@Configurationannotation (not needed for properties classes) - Change all static fields to instance fields
- Remove all
@Valueannotations (redundant with@ConfigurationProperties) - Add
OAuth2Utils.classto@EnableConfigurationPropertiesinSecurityAutoConfiguration.java - Inject
OAuth2Utilsas a dependency inOAuth2AdapterImplrather than calling static methods - Follow the pattern used by other properties classes like
LoginPropertiesImplorSecurityPropertiesImpl
* main: (31 commits) #5788 - Keyboard shortcuts to set granularity [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release inception-39.1 #5781 - INCEpTION 39.0 fails with PostgreSQL #5783 - Optimized rendering for HTML and XML documents No issue: Try not to use devtools URL as redirection target after login No issue: Avoid unnecessary override warning when original policy and new policy at the same #5777 - Allow fetching resource metadata from the backend #5777 - Allow fetching resource metadata from the backend [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release inception-39.0 #5726 - Recent activity across projects on project overview page #5610 - Upgrade dependencies No issue: Reduce log spamming in browser by stomp client [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release inception-39.0-beta-2 #5775 - Cleaning up No issue. Set version to 40.0-SNAPSHOT #5733 - Ability to export all annotations for given users or documents #5610 - Upgrade dependencies (39.0) ...
* main: (32 commits) #5809 - Ability to set the default grouping mode in the project settings #5793 - Upgrade dependencies [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release inception-40.0-beta-1 [maven-release-plugin] rollback the release of inception-40.0-beta-1 [maven-release-plugin] prepare release inception-40.0-beta-1 #5793 - Upgrade dependencies [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release inception-40.0-beta-1 #5773 - Upgrade to Spring 4 #5773 - Upgrade to Spring 4 #5773 - Upgrade to Spring 4 #5773 - Upgrade to Spring 4 #5773 - Upgrade to Spring 4 #5773 - Upgrade to Spring 4 #5773 - Upgrade to Spring 4 #251 - Option to focus active learning on current document #4699 - Ability to export all layers as JSON #4872 - Customizable key-bindings #5429 - Configurable default feature values ...
What's in the PR
This PR contains the feature allowing to map OAuth2 groups with INCEpTION roles, as described in the corresopnding feaure request
How to test manually
INCEPTION_ADMININCEPTION_USERINCEPTION_PROJECT_CREATORINCEPTION_REMOTEsettings.propertiesfile, inside your INCEpTION home directory :settings.propertiesfile. You can play with the mapping/groups to test the behavior.Note : :
OAuth2AdaptaterImplclass was vefore calling to thePreAuthUtilclass, whereas it's specified in the documentation that theExternal PreAuthfeature isn't compatible with OAuth2 authentication. I made a dedicatedOAuth2Utilsclass that contains a method that maps the OAuth2 groups if the mapping feature is enabled, or just adds theUSERINCEpTION role to the user if the mapping feature is disabled.@Configurationannotation.Automatic testing
OAuth2UtilsTestcontains a set of unit testsDocumentation
I'm of course open to feedback and willing to modify the implementation if needed ;)
Supersedes #4982 because GH actions somehow don't work on that one.