Bugfix for impersonating institution-admin

OpenConext · Sep 28, 2023 · 261d69f · 261d69f
1 parent eee1787
commit 261d69f
Show file tree

Hide file tree

Showing 7 changed files with 86 additions and 12 deletions.
diff --git a/server/src/main/java/access/model/User.java b/server/src/main/java/access/model/User.java
@@ -180,11 +180,15 @@ public void updateAttributes(Map<String, Object> attributes) {
         this.givenName = (String) attributes.get("given_name");
         this.familyName = (String) attributes.get("family_name");
         this.email = (String) attributes.get("email");
+        this.lastActivity = Instant.now();
+        this.updateRemoteAttributes(attributes);
+    }
+
+    @JsonIgnore
+    public void updateRemoteAttributes(Map<String, Object> attributes) {
         this.institutionAdmin = (boolean) attributes.getOrDefault(INSTITUTION_ADMIN, false);
         this.organizationGUID = (String) attributes.get(ORGANIZATION_GUID);
         this.applications = (List<Map<String, Object>>) attributes.getOrDefault(APPLICATIONS, Collections.emptyList());
         this.institution = (Map<String, Object>) attributes.getOrDefault(INSTITUTION, Collections.emptyMap());
-        this.lastActivity = Instant.now();
     }
-
 }
diff --git a/server/src/main/java/access/security/CustomOidcUserService.java b/server/src/main/java/access/security/CustomOidcUserService.java
@@ -61,7 +61,6 @@ public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2Authenticatio
         optionalUser.ifPresent(user -> {
             user.updateAttributes(newClaims);
             userRepository.save(user);
-
         });
         return oidcUser;
 

diff --git a/server/src/main/java/access/security/InstitutionAdmin.java b/server/src/main/java/access/security/InstitutionAdmin.java
@@ -17,6 +17,9 @@ public class InstitutionAdmin {
     public static final String APPLICATIONS = "APPLICATIONS";
     public static final String INSTITUTION = "INSTITUTION";
 
+    private InstitutionAdmin() {
+    }
+
     public static boolean isInstitutionAdmin(Map<String, Object> attributes, String requiredEntitlement) {
         if (attributes.containsKey("eduperson_entitlement")) {
             List<String> entitlements = (List<String>) attributes.get("eduperson_entitlement");

diff --git a/server/src/main/java/access/security/UserHandlerMethodArgumentResolver.java b/server/src/main/java/access/security/UserHandlerMethodArgumentResolver.java
@@ -4,7 +4,6 @@
 import access.manage.Manage;
 import access.model.User;
 import access.repository.UserRepository;
-import jakarta.servlet.http.HttpSession;
 import org.springframework.core.MethodParameter;
 import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken;
 import org.springframework.security.oauth2.server.resource.authentication.BearerTokenAuthentication;
@@ -40,6 +39,7 @@ public boolean supportsParameter(MethodParameter methodParameter) {
         return methodParameter.getParameterType().equals(User.class);
     }
 
+    @SuppressWarnings("unchecked")
     public User resolveArgument(MethodParameter methodParameter,
                                 ModelAndViewContainer mavContainer,
                                 NativeWebRequest webRequest,
@@ -86,10 +86,15 @@ public User resolveArgument(MethodParameter methodParameter,
             return new User(attributes);
         }
         return optionalUser.map(user -> {
-            if (validImpersonation.get() && user.isInstitutionAdmin() && StringUtils.hasText(user.getOrganizationGUID())) {
+            if (user.isInstitutionAdmin() && StringUtils.hasText(user.getOrganizationGUID())) {
                 String organizationGUID = user.getOrganizationGUID();
-                user.setApplications(manage.providersByInstitutionalGUID(organizationGUID));
-                user.setInstitution(manage.identityProviderByInstitutionalGUID(organizationGUID).orElse(Collections.emptyMap()));
+                if (validImpersonation.get()) {
+                    //The overhead is justified when super_user is impersonating institutionAdmin
+                    user.setApplications(manage.providersByInstitutionalGUID(organizationGUID));
+                    user.setInstitution(manage.identityProviderByInstitutionalGUID(organizationGUID).orElse(Collections.emptyMap()));
+                } else {
+                    user.updateRemoteAttributes(attributes);
+                }
             }
             return user;
         }).orElseThrow(UserRestrictionException::new);

diff --git a/server/src/test/java/access/AbstractTest.java b/server/src/test/java/access/AbstractTest.java
@@ -53,7 +53,6 @@
 import java.util.*;
 import java.util.function.Consumer;
 import java.util.function.UnaryOperator;
-import java.util.stream.Stream;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.*;
 import static io.restassured.RestAssured.given;
@@ -162,10 +161,19 @@ protected String opaqueAccessToken(String sub, String responseJsonFileName, Stri
     }
 
     protected AccessCookieFilter openIDConnectFlow(String path, String sub) throws Exception {
-        return this.openIDConnectFlow(path, sub, s -> {}, m -> m);
+        return this.openIDConnectFlow(path, sub, s -> {
+        }, m -> m);
     }
 
-    protected AccessCookieFilter openIDConnectFlow(String path, String sub, Consumer<String> authorizationConsumer,
+    protected AccessCookieFilter openIDConnectFlow(String path,
+                                                   String sub,
+                                                   UnaryOperator<Map<String, Object>> userInfoEnhancer) throws Exception {
+        return this.openIDConnectFlow(path, sub, s -> {
+        }, userInfoEnhancer);
+    }
+
+    protected AccessCookieFilter openIDConnectFlow(String path, String sub,
+                                                   Consumer<String> authorizationConsumer,
                                                    UnaryOperator<Map<String, Object>> userInfoEnhancer) throws Exception {
         CookieFilter cookieFilter = new CookieFilter();
         Headers headers = given()

diff --git a/server/src/test/java/access/api/EnvironmentControllerTest.java b/server/src/test/java/access/api/EnvironmentControllerTest.java
@@ -0,0 +1,29 @@
+package access.api;
+
+import access.AbstractTest;
+import access.model.Authority;
+import access.model.Invitation;
+import io.restassured.http.ContentType;
+import org.apache.commons.io.IOUtils;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+
+import static io.restassured.RestAssured.given;
+import static org.junit.jupiter.api.Assertions.*;
+
+class EnvironmentControllerTest extends AbstractTest {
+
+    @Test
+    void disclaimer() throws IOException {
+        InputStream inputStream = given()
+                .when()
+                .accept("text/css")
+                .get("/api/v1/disclaimer")
+                .asInputStream();
+        String css = IOUtils.toString(inputStream, Charset.defaultCharset());
+        assertEquals(css, "body::after {background: red;content: \"LOCAL\";}");
+    }
+}
diff --git a/server/src/test/java/access/api/RoleControllerTest.java b/server/src/test/java/access/api/RoleControllerTest.java
@@ -14,8 +14,7 @@
 import java.util.Map;
 import java.util.UUID;
 
-import static access.Seed.MANAGE_SUB;
-import static access.Seed.SUPER_SUB;
+import static access.Seed.*;
 import static com.github.tomakehurst.wiremock.client.WireMock.*;
 import static io.restassured.RestAssured.given;
 import static org.junit.jupiter.api.Assertions.*;
@@ -169,6 +168,33 @@ void rolesByApplication() throws Exception {
         assertEquals("Wiki", roles.get(0).getName());
     }
 
+    @Test
+    void rolesByApplicationInstitutionAdmin() throws Exception {
+        super.stubForManageProviderByEntityID(EntityType.SAML20_SP, "https://wiki");
+        super.stubForManageProviderByEntityID(EntityType.OIDC10_RP, "https://wiki");
+        super.stubForManageProviderByOrganisationGUID(ORGANISATION_GUID);
+
+        AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/me", INSTITUTION_ADMIN,
+                 m -> {
+                    m.put("eduperson_entitlement",
+                            List.of(
+                                    "urn:mace:surfnet.nl:surfnet.nl:sab:role:SURFconextverantwoordelijke",
+                                    "urn:mace:surfnet.nl:surfnet.nl:sab:organizationGUID:" + ORGANISATION_GUID
+                            ));
+                    return m;
+                });
+        List<Role> roles = given()
+                .when()
+                .filter(accessCookieFilter.cookieFilter())
+                .accept(ContentType.JSON)
+                .header(accessCookieFilter.csrfToken().getHeaderName(), accessCookieFilter.csrfToken().getToken())
+                .contentType(ContentType.JSON)
+                .get("/api/v1/roles")
+                .as(new TypeRef<>() {
+                });
+        assertEquals(4, roles.size());
+    }
+
     @Test
     void rolesByApplicationSuperUser() throws Exception {
         AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/login", SUPER_SUB);