#4941 - Mapping OIDC groups to INCEpTION's internal roles #4982

kzgrzendek · 2024-08-07T18:15:28Z

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

Run a local Keycloak, with a dedicated user and 4 groups associated to that user :

INCEPTION_ADMIN
INCEPTION_USER
INCEPTION_PROJECT_CREATOR
INCEPTION_REMOTE

Add the following lines to your settings.properties file, inside your INCEpTION home directory :

# OAuth2 groups mapping settings
oauth2-groups.enabled=true
oauth2-groups.admin=/INCEPTION_ADMIN
oauth2-groups.user=/INCEPTION_USER
oauth2-groups.project-creator=/INCEPTION_PROJECT_CREATOR
oauth2-groups.remote=/INCEPTION_REMOTE

Build and run an INCEpTION instance from this branch
Try to login via OAuth2, and observe that the newly created user is bind to all the INCEpTION roles, as mapped in the settings.properties file. You can play with the mapping/groups to test the behavior.

Note : :

The roles are mapped at the user's creation and at every connections
If the feature is enabled and the user isn't in any group mapped to INCEpTION roles, and AccessDeniedException will be throw.
The OAuth2AdaptaterImpl class was vefore calling to the PreAuthUtil class, whereas it's specified in the documentation that the External PreAuth feature isn't compatible with OAuth2 authentication. I made a dedicated OAuth2Utils class that contains a method that maps the OAuth2 groups if the mapping feature is enabled, or just adds the USER INCEpTION role to the user if the mapping feature is disabled.
Corresponding properties have been added with a native SpringBoot @Configuration annotation.

Automatic testing

The test class OAuth2UtilsTest contains a set of unit tests

PR includes unit tests

Documentation

I can do the documentation as well if the feature is validated.

PR updates documentation

I'm of course open to feedback and willing to modify the implementation if needed ;)

…efore cleaning and refactoring the feature

Updating branch with latest devs from main

Updated with latest main commits

… a dedicated file

reckart

Thanks for the suggestion! I hope you find my feedback useful.

reckart · 2024-08-07T20:08:35Z

...nception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2Utils.java

+
+@Configuration
+@ConfigurationProperties(prefix = "oauth2-groups")
+public class OAuth2Utils {


Utils classes in INCEpTION are typically final classes containing only static methods. Configuration classes are called XxxPropertiesImpl, e.g. KnowledgeBasePropertiesImpl and come with a respective interface, e.g. KnowledgeBaseProperties. This class/interface separation is necessary in cases where the properties are injected into Wicket code.

@reckart how would you recommend to proceed? Creating the interface and implementation class for the properties, injecting the PropertiesImpl class in OAuth2AdapterImpl's constructor and moving the mapping logic in a private method of that class?

reckart · 2024-08-07T20:09:27Z

...nception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2Utils.java

+@ConfigurationProperties(prefix = "oauth2-groups")
+public class OAuth2Utils {
+
+    private static boolean OAUTH2_GROUPS_ENABLED;


Fields should be non-static and use camelCase.

reckart · 2024-08-07T20:09:54Z

...nception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2Utils.java

+    }
+
+
+    public static Set<Role> getOAuth2UserRoles(User aUser, ArrayList<String> oauth2groups)


Arguments should use interfaces for collections. e.g. List instead of ArrayList.

reckart · 2024-08-07T20:10:33Z

...nception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2Utils.java

+    public static Set<Role> getOAuth2UserRoles(User aUser, ArrayList<String> oauth2groups)
+        throws AccessDeniedException
+    {
+        Set<Role> roles = new HashSet<>();


Use var when possible.

reckart · 2024-08-07T20:11:48Z

...nception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2Utils.java

+                + aUser.getUsername() + "] doesn't have any groups, or the corresponding claim is empty");
+        }
+
+        oauth2groups.forEach(group -> matchOauth2groupToRole(group, roles));


Functional programming (without side-effects) would be easier to understand. Can we use map instead of forEach here?

reckart · 2024-08-07T20:27:48Z

...tion-security/src/test/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2UtilsTest.java

+
+        Set<Role> userRoles = OAuth2Utils.getOAuth2UserRoles(testUser, userOAuth2Groups);
+
+        assertTrue(userRoles.contains(Role.ROLE_USER));


Please use AssertJ asserts instead of Unit asserts.

reckart · 2024-08-07T20:29:39Z

...tion-security/src/test/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2UtilsTest.java

+
+        ArrayList<String> userOAuth2Groups = new ArrayList<>();
+
+        try {


assertThatExceptionOfType(...).isThrownBy(() -> { ... })

reckart · 2024-08-07T20:30:38Z

...tion-security/src/test/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2UtilsTest.java

+
+@ExtendWith(SpringExtension.class)
+@ContextConfiguration(classes = OAuth2Utils.class)
+@DirtiesContext(classMode = ClassMode.AFTER_CLASS)


Instead of making this a spring integration test, I'd tend towards it being a plain junit test that sets up the necessary beans as POJOs. It seems a bit of overkill to start up an entire spring context for this.

reckart · 2024-08-07T20:33:45Z

...nception-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2Utils.java

+import de.tudarmstadt.ukp.clarin.webanno.security.model.User;
+
+@Configuration
+@ConfigurationProperties(prefix = "oauth2-groups")


security.oauth2.roles...? Other INCEpTION security properties live under security.... Mind, INCEpTION has no groups, only roles. It is incidental that you call these labels groups on your side. So from the INCEpTION perspective, we are mapping roles, not groups.

security.oauth.roles.enabled=true security.oauth.roles.claim=groups security.oauth.roles.mapping.ROLE_USER=ad-user security.oauth.roles.mapping.ROLE_ADMIN=ad-admin security.oauth.roles.mapping.ROLE_PROJECT_CREATOR=ad-project-creator security.oauth.roles.mapping....=...

reckart · 2024-08-07T20:34:42Z

...on-security/src/main/java/de/tudarmstadt/ukp/inception/security/oauth/OAuth2AdapterImpl.java

@@ -148,6 +147,10 @@ private User fetchOrMaterializeUser(OAuth2UserRequest userRequest, OAuth2User us
        if (u != null) {
            denyAccessToDeactivatedUsers(u);
            denyAccessOfRealmsDoNotMatch(realm, u);
+
+            u.setRoles(OAuth2Utils.getOAuth2UserRoles(u, user.getAttribute("groups")));


The claim should be configurable via the properties as well since this is not a well-defined name. Others might e.g. use a roles claim instead.

reckart · 2024-08-07T20:40:41Z

Btw. the commit message convention for INCEpTION goes like this:

#ISSUE-NUMBER - Title of the issue

- change 1
- change 2
- change 3

…roles

- OAuth2 Role Claim is now congigurable - Updated config prefix for the feature to 'security.oauth.roles'

- Updated branch with latest commits from `main`

reckart · 2024-09-07T09:08:35Z

The PR build fails but I have no idea why. I have opened a support discussion: https://github.com/orgs/community/discussions/138018

Kévin ZGRZENDEK added 9 commits July 31, 2024 22:05

Updated CRLF

8038bc1

Draft WIP

375efa2

Implemented feature. Existing UT done, but new ones must be written b…

3664f5c

…efore cleaning and refactoring the feature

Fixed gitattributes file and reupload badly encoded files

1fa5702

Merge branch 'main' into feat/map-oauth2-groups-to-roles

c19f375

Updating branch with latest devs from main

Wrote first basic UT

86f2266

Refactored UT and code with native spring boot properties injection

a036546

Merge branch 'main' into feat/map-oauth2-groups-to-roles

370bba9

Updated with latest main commits

Replaced test ptoperties injection with inline properties rather than…

644e435

… a dedicated file

kzgrzendek mentioned this pull request Aug 7, 2024

Mapping OIDC groups to INCEpTION's internal roles #4941

Open

reckart reviewed Aug 7, 2024

View reviewed changes

reckart changed the title ~~Feat/map oauth2 groups to roles~~ #4941 - Mapping OIDC groups to INCEpTION's internal roles Aug 7, 2024

reckart assigned kzgrzendek Aug 7, 2024

reckart added the ⭐️ Enhancement New feature or request label Aug 7, 2024

reckart added this to the 34.0 milestone Aug 7, 2024

kzgrzendek and others added 5 commits August 22, 2024 08:51

Merge branch 'inception-project:main' into feat/map-oauth2-groups-to-…

f86f656

…roles

4941 - Mapping OIDC groups to INCEpTION's internal roles

7787bc1

- OAuth2 Role Claim is now congigurable - Updated config prefix for the feature to 'security.oauth.roles'

4941 - Mapping OIDC groups to INCEpTION's internal roles

d45480a

- Updated branch with latest commits from `main`

Merge branch 'main' into feat/map-oauth2-groups-to-roles

c505b1a

Updated with latest main version

3cb8a01

reckart modified the milestones: 34.0, 35.0 Sep 7, 2024

Merge branch 'main' into feat/map-oauth2-groups-to-roles

f2b41dc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4941 - Mapping OIDC groups to INCEpTION's internal roles #4982

#4941 - Mapping OIDC groups to INCEpTION's internal roles #4982

kzgrzendek commented Aug 7, 2024

reckart left a comment

reckart Aug 7, 2024

kzgrzendek Aug 22, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart Aug 7, 2024

reckart commented Aug 7, 2024 •

edited

Loading

reckart commented Sep 7, 2024

		}


		public static Set<Role> getOAuth2UserRoles(User aUser, ArrayList<String> oauth2groups)


		Set<Role> userRoles = OAuth2Utils.getOAuth2UserRoles(testUser, userOAuth2Groups);

		assertTrue(userRoles.contains(Role.ROLE_USER));


		ArrayList<String> userOAuth2Groups = new ArrayList<>();

		try {

#4941 - Mapping OIDC groups to INCEpTION's internal roles #4982

Are you sure you want to change the base?

#4941 - Mapping OIDC groups to INCEpTION's internal roles #4982

Conversation

kzgrzendek commented Aug 7, 2024

reckart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckart commented Aug 7, 2024 • edited Loading

reckart commented Sep 7, 2024

reckart commented Aug 7, 2024 •

edited

Loading