WIP for #312

OpenConext · Oct 9, 2024 · 232472e · 232472e
1 parent 473f23a
commit 232472e
Show file tree

Hide file tree

Showing 15 changed files with 193 additions and 22 deletions.
diff --git a/client/src/locale/en.js b/client/src/locale/en.js
@@ -131,6 +131,7 @@ const en = {
     roles: {
         title: "Access Roles",
         applicationName: "Application",
+        auditable: "Role {{name}} was created by {{createdBy}} at {{createdAt}}",
         roleDetails: "Role details",
         invitationDetails: "Invitation details",
         applicationDetails: "Application(s) this role applies to",

diff --git a/client/src/locale/nl.js b/client/src/locale/nl.js
@@ -131,6 +131,7 @@ const nl = {
     roles: {
         title: "Toegangsrollen",
         applicationName: "Applicatie",
+        auditable: "Rol {{name}} is aangemaakt door {{createdBy}} op {{createdAt}}",
         roleDetails: "Details rol",
         invitationDetails: "Details uitnodiging",
         applicationDetails: "Applicatie(s) voor deze rol",

diff --git a/client/src/pages/RoleForm.js b/client/src/pages/RoleForm.js
@@ -60,12 +60,13 @@ export const RoleForm = () => {
     const [deletedUserRoles, setDeletedUserRoles] = useState(null);
 
     useEffect(() => {
-            if (!isUserAllowed(AUTHORITIES.MANAGER, user)) {
+            const newRole = id === "new";
+            //Managers may edit - certain attributes - of roles, but are not allowed to create new ones
+            if (!isUserAllowed(AUTHORITIES.MANAGER, user) || (newRole && !isUserAllowed(AUTHORITIES.INSTITUTION_ADMIN, user))) {
                 navigate("/404");
                 return;
             }
             setAllowedToEditApplication(isUserAllowed(AUTHORITIES.INSTITUTION_ADMIN, user))
-            const newRole = id === "new";
             const promises = [];
             if (!newRole) {
                 promises.push(roleByID(parseInt(id, 10)));
@@ -269,6 +270,19 @@ export const RoleForm = () => {
                 <h2 className="section-separator">
                     {I18n.t("roles.roleDetails")}
                 </h2>
+
+                {(!isNewRole && user.superUser) &&
+                    <div className="role-auditable">
+                        <p>
+                            {I18n.t("roles.auditable", {
+                                name: role.name,
+                                createdBy: role.auditable.createdBy,
+                                createdAt: dateFromEpoch(role.auditable.createdAt)
+                            })}
+                        </p>
+                    </div>
+                }
+
                 <InputField name={I18n.t("roles.name")}
                             value={role.name || ""}
                             placeholder={I18n.t("roles.namePlaceHolder")}

diff --git a/client/src/pages/RoleForm.scss b/client/src/pages/RoleForm.scss
@@ -77,6 +77,14 @@
             margin-top: 4px;
         }
 
+        div.role-auditable {
+            grid-column-start: first;
+            margin-top: 25px;
+            border-radius: .375rem;
+            padding: 10px;
+            background-color: var(--sds--color--gray--100);
+        }
+
         span.label {
             grid-column-start: first;
             font-weight: 600;

diff --git a/server/src/main/java/access/api/ManageController.java b/server/src/main/java/access/api/ManageController.java
@@ -14,6 +14,7 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
@@ -42,11 +43,15 @@ public class ManageController {
 
     private final Manage manage;
     private final ApplicationRepository applicationRepository;
+    private final boolean limitInstitutionAdminRoleVisibility;
 
     @Autowired
-    public ManageController(Manage manage, ApplicationRepository applicationRepository) {
+    public ManageController(Manage manage,
+                            ApplicationRepository applicationRepository,
+                            @Value("${feature.limit-institution-admin-role-visibility}") boolean limitInstitutionAdminRoleVisibility) {
         this.manage = manage;
         this.applicationRepository = applicationRepository;
+        this.limitInstitutionAdminRoleVisibility = limitInstitutionAdminRoleVisibility;
     }
 
     @GetMapping("provider/{type}/{id}")

diff --git a/server/src/main/java/access/api/RoleController.java b/server/src/main/java/access/api/RoleController.java
@@ -22,6 +22,7 @@
 import io.swagger.v3.oas.annotations.security.SecurityRequirement;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
@@ -58,19 +59,22 @@ public class RoleController {
     private final Manage manage;
     private final ProvisioningService provisioningService;
     private final URLFormatValidator urlFormatValidator = new URLFormatValidator();
+    private final boolean limitInstitutionAdminRoleVisibility;
 
     public RoleController(Config config,
                           RoleRepository roleRepository,
                           ApplicationRepository applicationRepository,
                           ApplicationUsageRepository applicationUsageRepository,
                           Manage manage,
-                          ProvisioningService provisioningService) {
+                          ProvisioningService provisioningService,
+                          @Value("${feature.limit-institution-admin-role-visibility}") boolean limitInstitutionAdminRoleVisibility) {
         this.config = config;
         this.roleRepository = roleRepository;
         this.applicationRepository = applicationRepository;
         this.applicationUsageRepository = applicationUsageRepository;
         this.manage = manage;
         this.provisioningService = provisioningService;
+        this.limitInstitutionAdminRoleVisibility = limitInstitutionAdminRoleVisibility;
     }
 
     @GetMapping("")
@@ -81,6 +85,12 @@ public ResponseEntity<List<Role>> rolesByApplication(@Parameter(hidden = true) U
             return ResponseEntity.ok(manage.addManageMetaData(roleRepository.findAll()));
         }
         UserPermissions.assertAuthority(user, Authority.INSTITUTION_ADMIN);
+
+        if (limitInstitutionAdminRoleVisibility) {
+            List<Role> roles = roleRepository.findByOrganizationGUID(user.getOrganizationGUID());
+            return ResponseEntity.ok(manage.addManageMetaData(roles));
+        }
+
         Set<String> manageIdentifiers = new HashSet<>();
         if (user.isInstitutionAdmin()) {
             Set<String> applicationManageIdentifiers = user.getApplications().stream().map(m -> (String) m.get("id")).collect(Collectors.toSet());
@@ -97,7 +107,7 @@ public ResponseEntity<List<Role>> rolesByApplication(@Parameter(hidden = true) U
         manageIdentifiers.addAll(roleManageIdentifiers);
 
         List<Role> roles = new ArrayList<>();
-        //TODO feature toggle see application.yml feature.enforce-institution-admin-role-visibility
+        //TODO feature toggle see application.yml feature.limit-institution-admin-role-visibility
         //findByOrganizationGUID_ApplicationUsagesApplicationManageId
         manageIdentifiers.forEach(manageId -> roles.addAll(roleRepository.findByApplicationUsagesApplicationManageId(manageId)));
         return ResponseEntity.ok(manage.addManageMetaData(roles));
@@ -152,7 +162,7 @@ public ResponseEntity<Role> newRole(@Validated @RequestBody Role role,
         if (user != null) {
             //OpenID connect login with User
             UserPermissions.assertAuthority(user, Authority.INSTITUTION_ADMIN);
-            //For super_users this is NULL, which is ok
+            //For super_users this is NULL, which is ok (unless they are impersonating an institution_admin)
             role.setOrganizationGUID(user.getOrganizationGUID());
         } else {
             //API user with Basic Authentication
@@ -193,6 +203,9 @@ public ResponseEntity<Void> deleteRole(@PathVariable("id") Long id, @Parameter(h
 
         manage.addManageMetaData(List.of(role));
         UserPermissions.assertRoleAccess(user, role, Authority.INSTITUTION_ADMIN);
+        if (limitInstitutionAdminRoleVisibility && !user.getOrganizationGUID().equals(role.getOrganizationGUID())) {
+            throw new UserRestrictionException();
+        }
 
         provisioningService.deleteGroupRequest(role);
         roleRepository.delete(role);

diff --git a/server/src/main/java/access/manage/Manage.java b/server/src/main/java/access/manage/Manage.java
@@ -2,6 +2,7 @@
 
 import access.model.GroupedProviders;
 import access.model.Role;
+import access.model.User;
 import org.springframework.util.CollectionUtils;
 
 import java.util.*;

diff --git a/server/src/main/java/access/repository/RoleRepository.java b/server/src/main/java/access/repository/RoleRepository.java
@@ -17,7 +17,7 @@ public interface RoleRepository extends JpaRepository<Role, Long> {
 
     List<Role> findByApplicationUsagesApplicationManageId(String manageId);
 
-    List<Role> findByOrganizationGUID_ApplicationUsagesApplicationManageId(String organizationGUID, String manageId);
+    List<Role> findByOrganizationGUID(String organizationGUID);
 
     List<Role> findByName(String name);
 

diff --git a/server/src/main/java/access/security/CustomOidcUserService.java b/server/src/main/java/access/security/CustomOidcUserService.java
@@ -75,7 +75,7 @@ public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2Authenticatio
                 try {
                     provisioningService.updateUserRequest(user);
                 } catch (RuntimeException e) {
-                    //We choose to ignore these
+                    //We choose to ignore these, because remote provisioning errors may not prevent login flow
                     LOG.error("Error in updateUserRequest", e);
                 }
 

diff --git a/server/src/main/java/access/security/UserHandlerMethodArgumentResolver.java b/server/src/main/java/access/security/UserHandlerMethodArgumentResolver.java
@@ -19,13 +19,12 @@
 import org.springframework.web.method.support.ModelAndViewContainer;
 
 import java.security.Principal;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicBoolean;
 
-import static access.security.InstitutionAdmin.INSTITUTION_ADMIN;
+import static access.security.InstitutionAdmin.*;
 import static access.security.SecurityConfig.API_TOKEN_HEADER;
 
 public class UserHandlerMethodArgumentResolver implements HandlerMethodArgumentResolver {
@@ -81,8 +80,8 @@ public User resolveArgument(MethodParameter methodParameter,
             //Does not make any difference security-wise which user we return
             User user = apiUsers.get(0);
             if (StringUtils.hasText(organizationGUID)) {
-                //The overhead is justified for API usage
-                enrichInstitutionAdmin(user, organizationGUID);
+                //The overhead is needed / justified for API usage as this are stateless
+                addInstitutionAdminAttributes(user, organizationGUID);
             }
             return user;
         } else {
@@ -128,7 +127,7 @@ public User resolveArgument(MethodParameter methodParameter,
                 String organizationGUID = user.getOrganizationGUID();
                 if (validImpersonation.get()) {
                     //The overhead for retrieving data from manage is justified when super_user is impersonating institutionAdmin
-                    enrichInstitutionAdmin(user, organizationGUID);
+                    addInstitutionAdminAttributes(user, organizationGUID);
                 } else {
                     user.updateRemoteAttributes(attributes);
                 }
@@ -138,11 +137,9 @@ public User resolveArgument(MethodParameter methodParameter,
 
     }
 
-    private void enrichInstitutionAdmin(User user, String organizationGUID) {
-        Optional<Map<String, Object>> optionalIdentityProvider = manage.identityProviderByInstitutionalGUID(organizationGUID);
-        user.setInstitution(optionalIdentityProvider.orElse(null));
-        List<Map<String, Object>> applications = optionalIdentityProvider.map(manage::providersAllowedByIdP).orElse(Collections.emptyList());
-        user.setApplications(applications);
+    private void addInstitutionAdminAttributes(User user, String organizationGUID) {
+        Map<String, Object> attributes = manage.enrichInstitutionAdmin(organizationGUID);
+        user.updateRemoteAttributes(attributes);
     }
 
 }
diff --git a/server/src/main/java/access/security/UserPermissions.java b/server/src/main/java/access/security/UserPermissions.java
@@ -123,7 +123,7 @@ private static boolean mayInviteByApplication(Set<UserRole> userRoles, Authority
                         userRole.getAuthority().hasEqualOrHigherRights(intendedAuthority));
     }
 
-    //Does the one off the applications has the same application as the role
+    //Does one off the user applications has the same application as the role
     private static boolean mayInviteByInstitutionAdmin(List<Map<String, Object>> applications,
                                                        List<String> manageIdentifiers) {
         return applications.stream()

diff --git a/server/src/main/resources/application-devconf.yml b/server/src/main/resources/application-devconf.yml
@@ -88,6 +88,9 @@ config:
   past-date-allowed: true
   eduid-idp-schac-home-organization: "dev.eduid.nl"
 
+feature:
+  limit-institution-admin-role-visibility: True
+
 # We don't encode in-memory passwords, so we need to prefix them with {noop}
 external-api-configuration:
   remote-users:

diff --git a/server/src/main/resources/application.yml b/server/src/main/resources/application.yml
@@ -121,7 +121,7 @@ config:
   eduid-idp-schac-home-organization: "test.eduid.nl"
 
 feature:
-  enforce-institution-admin-role-visibility: True
+  limit-institution-admin-role-visibility: False
 
 # We don't encode in-memory passwords, so we need to prefix them with {noop}
 external-api-configuration:

diff --git a/server/src/test/java/access/AbstractTest.java b/server/src/test/java/access/AbstractTest.java
@@ -586,15 +586,18 @@ public void doSeed() {
         Role research =
                 new Role("Research", "Research desc",
                         application("4", EntityType.SAML20_SP), 365, false, false);
+
         Role calendar =
                 new Role("Calendar", "Calendar desc",
                         application("5", EntityType.OIDC10_RP), 365, false, false);
-        calendar.setOrganizationGUID(ORGANISATION_GUID);
 
         Role mail =
                 new Role("Mail", "Mail desc",
                         application("5", EntityType.OIDC10_RP), 365, false, false);
-        mail.setOrganizationGUID(ORGANISATION_GUID);
+
+        //These roles will be accessible for the institution admin based
+        network.setOrganizationGUID(ORGANISATION_GUID);
+        research.setOrganizationGUID(ORGANISATION_GUID);
 
         doSave(this.roleRepository, wiki, network, storage, research, calendar, mail);