Fixes #320

OpenConext · Oct 10, 2024 · 0f8c9f1 · 0f8c9f1
1 parent 99dbc60
commit 0f8c9f1
Show file tree

Hide file tree

Showing 13 changed files with 232 additions and 17 deletions.
diff --git a/server/src/main/java/access/api/RoleController.java b/server/src/main/java/access/api/RoleController.java
@@ -190,7 +190,8 @@ public ResponseEntity<Role> updateRole(@Validated @RequestBody Role role,
             //API user with Basic Authentication
             RemoteUserPermissions.assertScopeAccess(remoteUser, Scope.sp_dashboard);
         }
-        LOG.debug(String.format("Update role '%s' by user %s", role.getName(), user.getEduPersonPrincipalName()));
+        String userName = user != null ? user.getEduPersonPrincipalName() : remoteUser.getName();
+        LOG.debug(String.format("Update role '%s' by user %s", role.getName(), userName));
 
         return saveOrUpdate(role, user, remoteUser);
     }

diff --git a/server/src/main/java/access/security/RemoteUser.java b/server/src/main/java/access/security/RemoteUser.java
@@ -1,6 +1,7 @@
 package access.security;
 
 import access.model.Provisionable;
+import lombok.AllArgsConstructor;
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.Setter;
@@ -9,18 +10,20 @@
 import org.springframework.security.core.authority.SimpleGrantedAuthority;
 import org.springframework.security.core.userdetails.UserDetails;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.stream.Collectors;
 
 @Getter
 @Setter
 @NoArgsConstructor
+@AllArgsConstructor
 public class RemoteUser implements UserDetails, CredentialsContainer, Provisionable {
 
     private String username;
     private String password;
-    private List<Scope> scopes;
+    private List<Scope> scopes = new ArrayList<>();
 
     public RemoteUser(RemoteUser remoteUser) {
         this.username = remoteUser.username;

diff --git a/server/src/main/java/access/security/UserPermissions.java b/server/src/main/java/access/security/UserPermissions.java
@@ -103,6 +103,9 @@ public static void assertRoleAccess(User user, Role accessRole, Authority author
         if (user.isSuperUser()) {
             return;
         }
+        if (accessRole == null) {
+            throw new UserRestrictionException();
+        }
         if (user.isInstitutionAdmin() && mayInviteByInstitutionAdmin(user.getApplications(), accessRole.applicationIdentifiers())) {
             return;
         }

diff --git a/server/src/main/java/access/teams/TeamsController.java b/server/src/main/java/access/teams/TeamsController.java
@@ -156,6 +156,9 @@ private void provision(Role role, Membership membership) {
     }
 
     protected static Authority mapAuthority(access.teams.Role role) {
+        if (role == null) {
+            throw new InvalidInputException("Null membership role");
+        }
         switch (role) {
             case MEMBER -> {
                 return Authority.GUEST;
@@ -166,9 +169,7 @@ protected static Authority mapAuthority(access.teams.Role role) {
             case ADMIN, OWNER -> {
                 return Authority.MANAGER;
             }
-            default -> {
-                throw new InvalidInputException("Unknown membership role: " + role);
-            }
         }
+        throw new InvalidInputException("Unknown membership role: " + role);
     }
 }
diff --git a/server/src/test/java/access/aggregation/AttributeAggregatorControllerTest.java b/server/src/test/java/access/aggregation/AttributeAggregatorControllerTest.java
@@ -10,6 +10,7 @@
 import java.util.List;
 import java.util.Map;
 
+import static access.manage.EntityType.SAML20_SP;
 import static io.restassured.RestAssured.given;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -18,7 +19,7 @@ class AttributeAggregatorControllerTest extends AbstractTest {
 
     @Test
     void getGroupMemberships() throws JsonProcessingException {
-        stubForManageProviderByEntityID(EntityType.SAML20_SP, "https://research");
+        stubForManageProviderByEntityID(SAML20_SP, "https://research");
         List<Map<String, String>> roles = given()
                 .when()
                 .auth().preemptive().basic("aa", "secret")
@@ -50,7 +51,7 @@ void getGroupMembershipsManageUnavailable() {
 
     @Test
     void getGroupMembershipsGuestIncluded() throws JsonProcessingException {
-        stubForManageProviderByEntityID(EntityType.SAML20_SP, "https://wiki");
+        stubForManageProviderByEntityID(SAML20_SP, "https://wiki");
         List<Map<String, String>> roles = given()
                 .when()
                 .auth().preemptive().basic("aa", "secret")
@@ -67,7 +68,7 @@ void getGroupMembershipsGuestIncluded() throws JsonProcessingException {
 
     @Test
     void getGroupMembershipsNonExistingUser() throws JsonProcessingException {
-        stubForManageProviderByEntityID(EntityType.SAML20_SP, "https://research");
+        stubForManageProviderByEntityID(SAML20_SP, "https://research");
         List<Map<String, String>> roles = given()
                 .when()
                 .auth().preemptive().basic("aa", "secret")
@@ -83,7 +84,7 @@ void getGroupMembershipsNonExistingUser() throws JsonProcessingException {
 
     @Test
     void getGroupMembershipsNonExistingProvider() throws JsonProcessingException {
-        stubForManageProviderByEntityID(EntityType.SAML20_SP, "nope");
+        stubForManageProviderByEntityID(SAML20_SP, "nope");
         stubForManageProviderByEntityID(EntityType.OIDC10_RP, "nope");
         List<Map<String, String>> roles = given()
                 .when()
@@ -97,4 +98,19 @@ void getGroupMembershipsNonExistingProvider() throws JsonProcessingException {
                 });
         assertEquals(0, roles.size());
     }
+
+    @Test
+    void manageDown() {
+        List<Map<String, String>> roles = given()
+                .when()
+                .auth().preemptive().basic("aa", "secret")
+                .accept(ContentType.JSON)
+                .contentType(ContentType.JSON)
+                .pathParam("sub", GUEST_SUB)
+                .queryParam("SPentityID", "")
+                .get("/api/external/v1/aa/{sub}")
+                .as(new TypeRef<>() {
+                });
+        assertEquals(0, roles.size());
+    }
 }
diff --git a/server/src/test/java/access/api/InvitationControllerTest.java b/server/src/test/java/access/api/InvitationControllerTest.java
@@ -568,4 +568,61 @@ void eduIDRequiredLoginOnlyForGuests() throws Exception {
                 m -> m);
     }
 
+    @Test
+    void newInvitationInvalidEmail() throws Exception {
+        AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/login", INVITER_SUB);
+
+        List<Long> roleIdentifiers = roleRepository.search("Calendar",1)
+                .stream()
+                .map(Role::getId)
+                .toList();
+        InvitationRequest invitationRequest = new InvitationRequest(
+                Authority.GUEST,
+                "Message",
+                Language.en,
+                true,
+                false,
+                false,
+                false,
+                List.of("nope"),
+                roleIdentifiers,
+                Instant.now().plus(365, ChronoUnit.DAYS),
+                Instant.now().plus(12, ChronoUnit.DAYS));
+
+        Map<String, Object> results = given()
+                .when()
+                .filter(accessCookieFilter.cookieFilter())
+                .accept(ContentType.JSON)
+                .header(accessCookieFilter.csrfToken().getHeaderName(), accessCookieFilter.csrfToken().getToken())
+                .contentType(ContentType.JSON)
+                .body(invitationRequest)
+                .post("/api/v1/invitations")
+                .as(new TypeRef<>() {
+                });
+        assertEquals(201, results.get("status"));
+        assertEquals(0, ((List) results.get("recipientInvitationURLs")).size());
+    }
+
+    @Test
+    void resendInviteMailExpirationDate() throws Exception {
+        AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/login", INVITER_SUB);
+        Invitation invitation = invitationRepository.findByHash(Authority.GUEST.name()).get();
+        invitation.setExpiryDate(Instant.now().minus(5, ChronoUnit.DAYS));
+        invitationRepository.save(invitation);
+
+        super.stubForManageProviderById(EntityType.OIDC10_RP, "5");
+        given()
+                .when()
+                .filter(accessCookieFilter.cookieFilter())
+                .accept(ContentType.JSON)
+                .header(accessCookieFilter.csrfToken().getHeaderName(), accessCookieFilter.csrfToken().getToken())
+                .contentType(ContentType.JSON)
+                .pathParam("id", invitation.getId())
+                .put("/api/v1/invitations/{id}")
+                .then()
+                .statusCode(201);
+        Invitation savedInvitation = invitationRepository.findByHash(Authority.GUEST.name()).get();
+        assertTrue(savedInvitation.getExpiryDate().isAfter(Instant.now().plus(13, ChronoUnit.DAYS)));
+    }
+
 }
diff --git a/server/src/test/java/access/api/RoleControllerTest.java b/server/src/test/java/access/api/RoleControllerTest.java
@@ -447,6 +447,22 @@ void createWithAPIUser() throws Exception {
         assertNotNull(newRole.getId());
     }
 
+    @Test
+    void updateWithAPIUser() {
+        Role role = roleRepository.findByName("Mail").get(0);
+        role.setDescription("changed");
+        Role newRole = given()
+                .when()
+                .auth().preemptive().basic("sp_dashboard", "secret")
+                .accept(ContentType.JSON)
+                .contentType(ContentType.JSON)
+                .body(role)
+                .put("/api/external/v1/sp_dashboard/roles")
+                .as(new TypeRef<>() {
+                });
+        assertEquals("changed", newRole.getDescription());
+    }
+
     @Test
     void rolesByApplicationSuperUserWithAPIToken() {
         super.stubForManagerProvidersByIdIn(EntityType.SAML20_SP, List.of("1", "2", "3", "4"));

diff --git a/server/src/test/java/access/cron/IdPMetaDataResolverTest.java b/server/src/test/java/access/cron/IdPMetaDataResolverTest.java
@@ -1,27 +1,28 @@
 package access.cron;
 
-import org.junit.Test;
+
+import org.junit.jupiter.api.Test;
 import org.springframework.core.io.ClassPathResource;
 
 import java.util.Arrays;
 import java.util.List;
 
 import static org.junit.Assert.assertEquals;
 
-public class IdPMetaDataResolverTest {
+class IdPMetaDataResolverTest {
 
     @Test
-    public void resolveIdpMetaDataNoException() {
+    void resolveIdpMetaDataNoException() {
         new IdPMetaDataResolver(null).resolveIdpMetaData();
     }
 
     @Test
-    public void resolveIdpMetaDataNoExceptionFileNotFound() {
+    void resolveIdpMetaDataNoExceptionFileNotFound() {
         new IdPMetaDataResolver(new ClassPathResource("metadata/nope")).resolveIdpMetaData();
     }
 
     @Test
-    public void resolveIdentityProvider() {
+    void resolveIdentityProvider() {
         IdPMetaDataResolver metaDataResolver = new IdPMetaDataResolver(new ClassPathResource("metadata/idps-metatdata-prod.xml"));
         List<String> schacHomes = Arrays.asList("student.ahk.nl", "ahknl.onmicrosoft.com", "ahk.nl");
         schacHomes.forEach(schacHome -> {

diff --git a/server/src/test/java/access/security/ExtendedInMemoryUserDetailsManagerTest.java b/server/src/test/java/access/security/ExtendedInMemoryUserDetailsManagerTest.java
@@ -0,0 +1,23 @@
+package access.security;
+
+import org.junit.jupiter.api.Test;
+import org.springframework.security.core.userdetails.UserDetails;
+import org.springframework.security.core.userdetails.UsernameNotFoundException;
+
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.*;
+class ExtendedInMemoryUserDetailsManagerTest {
+
+    private final RemoteUser remoteUser = new RemoteUser("user", "password", List.of(Scope.profile))    ;
+    private final ExtendedInMemoryUserDetailsManager userDetailsManager =
+            new ExtendedInMemoryUserDetailsManager(List.of(remoteUser));
+
+    @Test
+    void loadUserByUsername() {
+        assertThrows(UsernameNotFoundException.class, () ->  userDetailsManager.loadUserByUsername("nope"));
+
+        RemoteUser user = (RemoteUser) userDetailsManager.loadUserByUsername("user");
+        assertNotEquals(remoteUser, user);
+    }
+}
diff --git a/server/src/test/java/access/security/LocalDevelopmentAuthenticationFilterTest.java b/server/src/test/java/access/security/LocalDevelopmentAuthenticationFilterTest.java
@@ -0,0 +1,31 @@
+package access.security;
+
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import org.junit.jupiter.api.Test;
+import org.springframework.mock.web.MockFilterChain;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.context.SecurityContextHolder;
+
+import java.io.IOException;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class LocalDevelopmentAuthenticationFilterTest {
+
+    private final LocalDevelopmentAuthenticationFilter authenticationFilter = new LocalDevelopmentAuthenticationFilter();
+
+    @Test
+    void doFilter() throws ServletException, IOException {
+        ServletRequest request = new MockHttpServletRequest();
+        ServletResponse response = new MockHttpServletResponse();
+        FilterChain filterChain = new MockFilterChain();
+        authenticationFilter.doFilter(request, response, filterChain);
+        Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+        assertNotNull(authentication);
+    }
+}
diff --git a/server/src/test/java/access/security/RemoteUserPermissionsTest.java b/server/src/test/java/access/security/RemoteUserPermissionsTest.java
@@ -0,0 +1,23 @@
+package access.security;
+
+import access.exception.UserRestrictionException;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class RemoteUserPermissionsTest {
+
+    @Test
+    void assertScopeAccess() {
+        assertThrows(UserRestrictionException.class, () -> RemoteUserPermissions.assertScopeAccess(null));
+        assertThrows(UserRestrictionException.class, () -> RemoteUserPermissions.assertScopeAccess(new RemoteUser(), Scope.profile));
+
+        RemoteUserPermissions.assertScopeAccess(new RemoteUser());
+        RemoteUserPermissions.assertScopeAccess(
+                new RemoteUser("user", "secret", List.of(Scope.profile)), Scope.profile);
+
+    }
+
+}
diff --git a/server/src/test/java/access/security/UserPermissionsTest.java b/server/src/test/java/access/security/UserPermissionsTest.java
@@ -9,6 +9,7 @@
 import java.security.SecureRandom;
 import java.util.*;
 
+import static java.util.Collections.emptyList;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 class UserPermissionsTest extends WithApplicationTest {
@@ -138,7 +139,7 @@ void assertManagerRoleInstitutionAdmin() {
     }
 
     @Test
-    void assertRoleAccessInstitutionAdmin() {
+    void assertRoleAccessInstitutionAdminApplications() {
         User user = new User();
         user.setInstitutionAdmin(true);
         user.setApplications(List.of(Map.of("id", "1")));
@@ -165,7 +166,7 @@ void assertRoleAccessManager() {
     }
 
     @Test
-    void assertRoleAccessImstitutionAdmin() {
+    void assertRoleAccessInstitutionAdmin() {
         String identifier = UUID.randomUUID().toString();
         User user = userWithRole(Authority.INSTITUTION_ADMIN, identifier);
         Role role = new Role("name", "description", application(identifier, EntityType.SAML20_SP), 365, false, false);
@@ -182,6 +183,18 @@ void assertNoRoleAccess() {
         assertThrows(UserRestrictionException.class, () -> UserPermissions.assertRoleAccess(user, role));
     }
 
+    @Test
+    void nullPointerHygiene() {
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertSuperUser(null));
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertInstitutionAdmin(null));
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertInstitutionAdmin(new User()));
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertAuthority(null, Authority.GUEST));
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertValidInvitation(null, Authority.GUEST, emptyList()));
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertManagerRole(emptyList(), null));
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertRoleAccess( null, null,Authority.GUEST));
+        assertThrows(UserRestrictionException.class, () -> UserPermissions.assertRoleAccess( new User(), null,Authority.GUEST));
+    }
+
     private User userWithRole(Authority authority, String manageIdentifier) {
         return userWithRole(new User(), authority, manageIdentifier);
     }