Skip to content
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

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
*.dump binary
*.xcas binary
*.xmi binary
*.gif binary
*.pptx binary
*.zip binary
# next is probably the default
*.pdf binary
*.ser binary
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,13 @@
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;
import org.springframework.context.ConfigurableApplicationContext;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.DynamicPropertyRegistry;
import org.springframework.test.context.DynamicPropertySource;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import org.springframework.util.FileSystemUtils;

class InceptionHsqldbIntegrationTest
Expand All @@ -55,6 +58,8 @@ static void tearDownClass()
}

@Nested
@ContextConfiguration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Why) did you have to add these annotations here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a side effect of a refactor, I didn't changed that class - will revert that

@ExtendWith(SpringExtension.class)
class SpringApplcationContext
extends InceptionIntegrationTest_ImplBase
{
Expand All @@ -66,9 +71,11 @@ static void configureProperties(DynamicPropertyRegistry registry)
// a non-temporary folder on Windows because open files cannot be deleted.
STATIC_TEST_FOLDER.mkdirs();
registry.add("inception.home", () -> STATIC_TEST_FOLDER);

}
else {
registry.add("inception.home", () -> tempDir.toString());

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
import de.tudarmstadt.ukp.clarin.webanno.security.OverridableUserDetailsManager;
import de.tudarmstadt.ukp.clarin.webanno.security.UserDao;
import de.tudarmstadt.ukp.clarin.webanno.security.model.User;
import de.tudarmstadt.ukp.clarin.webanno.security.preauth.PreAuthUtils;

public class OAuth2AdapterImpl
implements OAuth2Adapter
Expand Down Expand Up @@ -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));
userRepository.update(u);

return u;
}

Expand All @@ -161,7 +164,8 @@ private User materializeUser(OAuth2User user, String username, String realm)
u.setPassword(UserDao.EMPTY_PASSWORD);
u.setEnabled(true);
u.setRealm(realm);
u.setRoles(PreAuthUtils.getPreAuthenticationNewUserRoles(u));

u.setRoles(OAuth2Utils.getOAuth2UserRoles(u, user));

String email = user.getAttribute("email");
if (email != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/*
* Licensed to the Technische Universität Darmstadt under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The Technische Universität Darmstadt
* licenses this file to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License.
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package de.tudarmstadt.ukp.inception.security.oauth;

import java.util.HashSet;
import java.util.List;
import java.util.Set;

import org.apache.commons.lang3.StringUtils;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.context.annotation.Configuration;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.oauth2.core.user.OAuth2User;

import de.tudarmstadt.ukp.clarin.webanno.security.model.Role;
import de.tudarmstadt.ukp.clarin.webanno.security.model.User;

@Configuration
@ConfigurationProperties(prefix = "security.oauth.roles")
public class OAuth2Utils {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?


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;
}


public static Set<Role> getOAuth2UserRoles(User aUser, OAuth2User user)
throws AccessDeniedException
{
Set<Role> roles = new HashSet<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use var when possible.


if (!OAUTH2_ROLES_ENABLED) {
roles.add(Role.ROLE_USER);
return roles;
}

List<String> oauth2groups = user.getAttribute(OAUTH2_ROLES_CLAIM);

if (oauth2groups == null || oauth2groups.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just return the empty list here.

throw new AccessDeniedException("OAuth2 roles mapping is enabled, but user ["
+ aUser.getUsername() + "] doesn't have any roles, or the corresponding claim is empty");
}

oauth2groups.forEach(group -> matchOauth2groupToRole(group, roles));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (roles.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this check is necessary at all since the application should only set users in that have ROLE_USER. If it is necessary, then it should IMHO be at a higher level, not at the level where the groups are extracted from the claim.

throw new AccessDeniedException("User ["
+ aUser.getUsername() + "] doesn't belong to any role");
}

return roles;
}

private static void matchOauth2groupToRole(String oauth2group, Set<Role> userRoles) {

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)) {
userRoles.add(Role.ROLE_REMOTE);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/*
* Licensed to the Technische Universität Darmstadt under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The Technische Universität Darmstadt
* licenses this file to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License.
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package de.tudarmstadt.ukp.inception.security.oauth;

import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.ArrayList;
import java.util.Set;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.oauth2.core.user.OAuth2User;
import org.springframework.test.annotation.DirtiesContext;
import org.springframework.test.annotation.DirtiesContext.ClassMode;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.TestPropertySource;
import org.springframework.test.context.junit.jupiter.SpringExtension;

import de.tudarmstadt.ukp.clarin.webanno.security.model.Role;
import de.tudarmstadt.ukp.clarin.webanno.security.model.User;


@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes = OAuth2Utils.class)
@DirtiesContext(classMode = ClassMode.AFTER_CLASS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@TestPropertySource(
properties = """
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use @DynamicPropertySource to configure the properties programmatically, that allows you to use constants and this way facilitate refactoring.

security.oauth.roles.enabled=true
security.oauth.roles.claim=groups
security.oauth.roles.admin=/INCEPTION_ADMIN
security.oauth.roles.user=/INCEPTION_USER
security.oauth.roles.project-creator=/INCEPTION_PROJECT_CREATOR
security.oauth.roles.remote=/INCEPTION_REMOTE
""")
public class OAuth2UtilsTest
{

String USERNAME = "ThatGuy";
String OAUTH2_GROUP_USER = "/INCEPTION_USER";
String OAUTH2_GROUP_PROJECT_CREATOR = "/INCEPTION_PROJECT_CREATOR";
String OAUTH2_GROUP_ADMIN = "/INCEPTION_ADMIN";
String OAUTH2_GROUP_REMOTE = "/INCEPTION_REMOTE";

User testUser;
OAuth2User testRetievedOAuth2User;

@BeforeEach
void setup() {
testUser = new User();
testUser.setUsername(USERNAME);

testRetievedOAuth2User = mock(OAuth2User.class);
}

@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));
}

@Test
void thatUserRoleIsGivenIfMatchingGroupFound()
{
ArrayList<String> userOAuth2Groups = new ArrayList<>();
userOAuth2Groups.add(OAUTH2_GROUP_USER);
when(testRetievedOAuth2User.getAttribute(anyString())).thenReturn(userOAuth2Groups);

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

assertTrue(userRoles.contains(Role.ROLE_USER));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use AssertJ asserts instead of Unit asserts.

}

@Test
void thatProjectCreatorRoleIsGivenIfMatchingGroupFound()
{
ArrayList<String> userOAuth2Groups = new ArrayList<>();
userOAuth2Groups.add(OAUTH2_GROUP_PROJECT_CREATOR);
when(testRetievedOAuth2User.getAttribute(anyString())).thenReturn(userOAuth2Groups);

Set<Role> userRoles = OAuth2Utils.getOAuth2UserRoles(testUser, testRetievedOAuth2User);
assertTrue(userRoles.contains(Role.ROLE_PROJECT_CREATOR));
}

@Test
void thatRemoteRoleIsGivenIfMatchingGroupFound()
{
ArrayList<String> userOAuth2Groups = new ArrayList<>();
userOAuth2Groups.add(OAUTH2_GROUP_REMOTE);
when(testRetievedOAuth2User.getAttribute(anyString())).thenReturn(userOAuth2Groups);

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

assertTrue(userRoles.contains(Role.ROLE_REMOTE));
}

@Test
void thatUnauthorizedExceptionIsThrownIfNoRoleIsMapped() {

ArrayList<String> userOAuth2Groups = new ArrayList<>();
when(testRetievedOAuth2User.getAttribute(anyString())).thenReturn(userOAuth2Groups);

try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OAuth2Utils.getOAuth2UserRoles(testUser, testRetievedOAuth2User);
} catch (AccessDeniedException ade) {
System.out.println(ade.getClass().getSimpleName() + " was thrown");
return;
}

fail("Expected Exception wasn't catched");
}
}
Loading