Merge branch 'feature/external-role-api'

OpenConext · Aug 20, 2024 · f0d678a · f0d678a
2 parents e905064 + b693f11
commit f0d678a
Show file tree

Hide file tree

Showing 15 changed files with 114 additions and 17 deletions.
diff --git a/server/src/main/java/access/api/RoleController.java b/server/src/main/java/access/api/RoleController.java
@@ -13,6 +13,7 @@
 import access.repository.ApplicationRepository;
 import access.repository.ApplicationUsageRepository;
 import access.repository.RoleRepository;
+import access.security.RemoteUser;
 import access.security.UserPermissions;
 import access.validation.URLFormatValidator;
 import io.swagger.v3.oas.annotations.Parameter;
@@ -22,6 +23,8 @@
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.annotation.AuthenticationPrincipal;
 import org.springframework.transaction.annotation.Transactional;
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.StringUtils;
@@ -136,7 +139,10 @@ public ResponseEntity<List<Role>> search(@RequestParam(value = "query") String q
     }
 
     @PostMapping("")
-    public ResponseEntity<Role> newRole(@Validated @RequestBody Role role, @Parameter(hidden = true) User user) {
+    public ResponseEntity<Role> newRole(@Validated @RequestBody Role role,
+                                        @Parameter(hidden = true) User user,
+                                        @Parameter(hidden = true) @AuthenticationPrincipal RemoteUser remoteUser) {
+        //TODO differentiate between RemoteUser and User
         UserPermissions.assertAuthority(user, Authority.INSTITUTION_ADMIN);
         role.setShortName(GroupURN.sanitizeRoleShortName(role.getShortName()));
         role.setIdentifier(UUID.randomUUID().toString());

diff --git a/server/src/main/java/access/api/UserController.java b/server/src/main/java/access/api/UserController.java
@@ -172,7 +172,9 @@ public ResponseEntity<Map<String, Integer>> logout(HttpServletRequest request) {
     @GetMapping("ms-accept-return/{manageId}/{userId}")
     public View msAcceptReturn(@PathVariable("manageId") String manageId, @PathVariable("userId") Long userId) {
         User user = userRepository.findById(userId).orElseThrow(() -> new NotFoundException("User not found"));
-        LOG.debug(String.format("Return from MS accept. User %s",user.getEduPersonPrincipalName()));
+
+        LOG.info(String.format("Return from MS accept. User %s",user.getEduPersonPrincipalName()));
+
         Map<String, Object> provisioningMap = manage.providerById(EntityType.PROVISIONING, manageId);
         Provisioning provisioning = new Provisioning(provisioningMap);
         AtomicReference<String> redirectReference = new AtomicReference<>(this.config.getWelcomeUrl());

diff --git a/server/src/main/java/access/cron/RoleExpirationNotifier.java b/server/src/main/java/access/cron/RoleExpirationNotifier.java
@@ -58,8 +58,8 @@ public List<String> doSweep() {
             List<GroupedProviders> groupedProviders = manage.getGroupedProviders(List.of(userRole.getRole()));
             GroupedProviders groupedProvider = groupedProviders.isEmpty() ? null : groupedProviders.get(0);
             String mail = mailBox.sendUserRoleExpirationNotificationMail(userRole, groupedProvider, roleExpirationNotificationDays);
-            userRole.setExpiryNotifications(1);
-            userRoleRepository.save(userRole);
+            //https://stackoverflow.com/a/75121707
+            userRoleRepository.updateExpiryNotifications(1, userRole.getId());
 
             LOG.info("Send expiration notification mail to " + userRole.getUser().getEmail());
 

diff --git a/server/src/main/java/access/manage/Manage.java b/server/src/main/java/access/manage/Manage.java
@@ -125,8 +125,9 @@ default List<GroupedProviders> getGroupedProviders(List<Role> requestedRoles) {
                 .map(application -> new ManageIdentifier(application.getManageId(), application.getManageType()))
                 .collect(Collectors.toSet())
                 .stream()
-                .map(manageIdentifier -> {
-                    Map<String, Object> provider = providerById(manageIdentifier.manageType(), manageIdentifier.manageId());
+                .map(manageIdentifier -> providerById(manageIdentifier.manageType(), manageIdentifier.manageId()))
+                .filter(provider -> !CollectionUtils.isEmpty(provider))
+                .map(provider -> {
                     String id = (String) provider.get("id");
                     return new GroupedProviders(
                             provider,

diff --git a/server/src/main/java/access/manage/RemoteManage.java b/server/src/main/java/access/manage/RemoteManage.java
@@ -5,6 +5,7 @@
 import org.springframework.core.io.ClassPathResource;
 import org.springframework.http.client.support.BasicAuthenticationInterceptor;
 import org.springframework.util.CollectionUtils;
+import org.springframework.web.client.ResponseErrorHandler;
 import org.springframework.web.client.RestTemplate;
 
 import java.io.IOException;
@@ -28,6 +29,8 @@ public RemoteManage(String url, String user, String password, ObjectMapper objec
         this.queries = objectMapper.readValue(new ClassPathResource("/manage/query_templates.json").getInputStream(), new TypeReference<>() {
         });
         restTemplate.getInterceptors().add(new BasicAuthenticationInterceptor(user, password));
+        ResponseErrorHandler resilientErrorHandler = new ResilientErrorHandler();
+        restTemplate.setErrorHandler(resilientErrorHandler);
     }
 
     @Override

diff --git a/server/src/main/java/access/manage/ResilientErrorHandler.java b/server/src/main/java/access/manage/ResilientErrorHandler.java
@@ -0,0 +1,37 @@
+package access.manage;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.springframework.http.HttpMethod;
+import org.springframework.http.HttpStatusCode;
+import org.springframework.http.client.ClientHttpResponse;
+import org.springframework.web.client.ResponseErrorHandler;
+
+import java.io.IOException;
+import java.net.URI;
+
+public class ResilientErrorHandler implements ResponseErrorHandler {
+
+    private static final Log LOG = LogFactory.getLog(ResilientErrorHandler.class);
+
+    @Override
+    public boolean hasError(ClientHttpResponse response) throws IOException {
+        HttpStatusCode statusCode = response.getStatusCode();
+        return statusCode.isError();
+    }
+
+    @Override
+    public void handleError(ClientHttpResponse response) throws IOException {
+        this.handleError(null, null, response);
+    }
+
+    @Override
+    public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException {
+        //ignore
+        LOG.warn(String.format("Error from Manage: %s, %s, %s, %s",
+                url,
+                method,
+                response.getStatusText(),
+                new String(response.getBody().readAllBytes())));
+    }
+}
diff --git a/server/src/main/java/access/repository/UserRoleRepository.java b/server/src/main/java/access/repository/UserRoleRepository.java
@@ -1,9 +1,7 @@
 package access.repository;
 
-import access.model.Authority;
 import access.model.Role;
 import access.model.UserRole;
-import org.springframework.data.jpa.repository.EntityGraph;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.Modifying;
 import org.springframework.data.jpa.repository.Query;
@@ -12,9 +10,7 @@
 import org.springframework.transaction.annotation.Transactional;
 
 import java.time.Instant;
-import java.util.Collection;
 import java.util.List;
-import java.util.Optional;
 
 @Repository
 public interface UserRoleRepository extends JpaRepository<UserRole, Long> {
@@ -28,7 +24,12 @@ public interface UserRoleRepository extends JpaRepository<UserRole, Long> {
     List<UserRole> findByRoleName(String roleName);
 
     @Modifying
-    @Query("DELETE FROM user_roles WHERE id = ?1")
+    @Query(value = "DELETE FROM user_roles WHERE id = ?1", nativeQuery = true)
     @Transactional(isolation = Isolation.SERIALIZABLE)
     void deleteUserRoleById(Long id);
+
+    @Modifying
+    @Query(value = "UPDATE user_roles SET expiry_notifications= ?1 WHERE id = ?2", nativeQuery = true)
+    @Transactional(isolation = Isolation.SERIALIZABLE)
+    void updateExpiryNotifications(Integer expiryNotifications, Long id);
 }
diff --git a/server/src/main/java/access/security/ExtendedInMemoryUserDetailsManager.java b/server/src/main/java/access/security/ExtendedInMemoryUserDetailsManager.java
@@ -32,6 +32,10 @@ private void fixPassword(RemoteUser remoteUser) {
     @Override
     public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
         RemoteUser remoteUser = users.get(username);
-        return remoteUser != null ? new RemoteUser(remoteUser) : null;
+        //Need to make copy, otherwise the password is erased. See RemoteUser#CredentialsContainer
+        if (remoteUser == null) {
+            throw new UsernameNotFoundException("User not found: " + username);
+        }
+        return new RemoteUser(remoteUser);
     }
 }
diff --git a/server/src/main/java/access/security/RemoteUser.java b/server/src/main/java/access/security/RemoteUser.java
@@ -29,6 +29,7 @@ public RemoteUser(RemoteUser remoteUser) {
 
     @Override
     public Collection<? extends GrantedAuthority> getAuthorities() {
+        //Convention dictates upperCase Role names to be used in @PreAuthorize annotations
         return scopes.stream()
                 .map(scope -> new SimpleGrantedAuthority("ROLE_" + scope.toUpperCase()))
                 .collect(Collectors.toList());

diff --git a/server/src/main/java/access/security/SecurityConfig.java b/server/src/main/java/access/security/SecurityConfig.java
@@ -58,6 +58,8 @@ public class SecurityConfig {
     private final ProvisioningService provisioningService;
     private final ExternalApiConfiguration externalApiConfiguration;
 
+    private final RequestHeaderRequestMatcher apiTokenRequestMatcher = new RequestHeaderRequestMatcher(API_TOKEN_HEADER);
+
     @Autowired
     public SecurityConfig(ClientRegistrationRepository clientRegistrationRepository,
                           InvitationRepository invitationRepository,
@@ -188,11 +190,15 @@ SecurityFilterChain basicAuthenticationSecurityFilterChain(HttpSecurity http) th
                         "/api/aa/**",
                         "/api/external/v1/aa/**",
                         "/api/deprovision/**",
-                        "/api/external/v1/deprovision/**")
+                        "/api/external/v1/deprovision/**",
+                        "/api/external/v1/roles")
                 .sessionManagement(c -> c
                         .sessionCreationPolicy(SessionCreationPolicy.STATELESS)
                 )
                 .authorizeHttpRequests(c -> c
+                        //The API token is secured in the UserHandlerMethodArgumentResolver
+                        .requestMatchers(apiTokenRequestMatcher)
+                        .permitAll()
                         .anyRequest()
                         .authenticated()
                 )
@@ -203,7 +209,7 @@ SecurityFilterChain basicAuthenticationSecurityFilterChain(HttpSecurity http) th
     @Bean
     @Order(3)
     SecurityFilterChain jwtSecurityFilterChain(HttpSecurity http) throws Exception {
-        final RequestHeaderRequestMatcher apiTokenRequestMatcher = new RequestHeaderRequestMatcher(API_TOKEN_HEADER);
+
         http.csrf(c -> c.disable())
                 .securityMatcher("/api/external/v1/**")
                 .authorizeHttpRequests(c -> c

diff --git a/server/src/main/java/access/teams/TeamsController.java b/server/src/main/java/access/teams/TeamsController.java
@@ -112,8 +112,8 @@ public ResponseEntity<Map<String, Integer>> migrateTeam(@RequestBody Team team)
 
     private boolean applicationExists(Application application) {
         try {
-            manage.providerById(application.getManageType(), application.getManageId());
-            return true;
+            Map<String, Object> provider = manage.providerById(application.getManageType(), application.getManageId());
+            return provider != null;
         } catch (RuntimeException e) {
             return false;
         }

diff --git a/server/src/main/resources/application.yml b/server/src/main/resources/application.yml
@@ -158,7 +158,7 @@ email:
 # ignored then and the different manage entities are loaded from json files in `server/src/main/resources/manage`.
 # Each *.json file in this directory corresponds with the contents of that specific entity_type.
 # To test the provisioning (e.g. SCIM, EVA, Graphp) with real endpoints you can set the manage.local property below to
-# False and then the provisioning.local.json file is used which is not in git as it is n .gitignore. You can safely
+# True and then the provisioning.local.json file is used which is not in git as it is n .gitignore. You can safely
 # configure real users / passwords and test against those. See server/src/main/java/access/manage/ManageConf.java
 # and server/src/main/java/access/manage/LocalManage.java to see how it works.
 manage:

diff --git a/server/src/test/java/access/api/RoleControllerTest.java b/server/src/test/java/access/api/RoleControllerTest.java
@@ -12,6 +12,7 @@
 import org.junit.jupiter.api.Test;
 import org.springframework.http.HttpStatus;
 
+import java.lang.reflect.Type;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -434,6 +435,27 @@ void deleteRoleWithAPI() throws Exception {
         assertEquals(0, roleRepository.search("wiki", 1).size());
     }
 
+    @Test
+    void createWithAPIUser() throws Exception {
+        Role role = new Role("New", "New desc", application("1", EntityType.SAML20_SP), 365, false, false);
+
+        super.stubForManagerProvidersByIdIn(EntityType.SAML20_SP, List.of("1"));
+        super.stubForManageProvisioning(List.of("1"));
+        super.stubForCreateScimRole();
+
+        given()
+                .when()
+                .auth().preemptive().basic("voot", "secret")
+                .accept(ContentType.JSON)
+                .contentType(ContentType.JSON)
+                .body(role)
+                .post("/api/external/v1/roles")
+                .then()
+                .statusCode(400);
+        //TODO
+//        assertNotNull(result.get("id"));
+    }
+
     @Test
     void rolesByApplicationSuperUserWithAPIToken() {
         super.stubForManagerProvidersByIdIn(EntityType.SAML20_SP, List.of("1", "2", "3", "4"));

diff --git a/server/src/test/java/access/cron/RoleExpirationNotifierTest.java b/server/src/test/java/access/cron/RoleExpirationNotifierTest.java
@@ -37,6 +37,10 @@ void sweep() {
 
         userRole = userRoleRepository.findByRoleName("Mail").get(0);
         assertEquals(1, userRole.getExpiryNotifications());
+
+        Instant instant = Instant.now().plus(100, ChronoUnit.DAYS);
+        List<UserRole> userRoles = userRoleRepository.findByEndDateBeforeAndExpiryNotifications(instant, 0);
+        assertEquals(0, userRoles.size());
     }
 
     @Test

diff --git a/server/src/test/java/access/manage/RemoteManageTest.java b/server/src/test/java/access/manage/RemoteManageTest.java
@@ -11,6 +11,7 @@
 
 import static com.github.tomakehurst.wiremock.client.WireMock.*;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 
 class RemoteManageTest extends AbstractTest {
 
@@ -64,4 +65,13 @@ void providersByIdIn() throws JsonProcessingException {
         assertEquals(providers, remoteProviders);
     }
 
+    @Test
+    void providerByIdExceptionHandling() {
+        stubFor(get(urlPathMatching("/manage/api/internal/metadata/saml20_sp/1")).willReturn(aResponse()
+                .withHeader("Content-Type", "application/json")
+                .withStatus(404)));
+        Map<String, Object> remoteProvider = manage.providerById(EntityType.SAML20_SP, "1");
+        assertNull(remoteProvider);
+    }
+
 }