diff --git a/client/src/locale/en.js b/client/src/locale/en.js index 8dd44fa1..19fdb918 100644 --- 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 index 0b04e67f..dfa79743 100644 --- 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 index 58a27895..d218a4ba 100644 --- 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 = () => {

{I18n.t("roles.roleDetails")}

+ + {(!isNewRole && user.superUser) && +
+

+ {I18n.t("roles.auditable", { + name: role.name, + createdBy: role.auditable.createdBy, + createdAt: dateFromEpoch(role.auditable.createdAt) + })} +

+
+ } + > rolesByApplication(@Parameter(hidden = true) U return ResponseEntity.ok(manage.addManageMetaData(roleRepository.findAll())); } UserPermissions.assertAuthority(user, Authority.INSTITUTION_ADMIN); + + if (limitInstitutionAdminRoleVisibility) { + List roles = roleRepository.findByOrganizationGUID(user.getOrganizationGUID()); + return ResponseEntity.ok(manage.addManageMetaData(roles)); + } + Set manageIdentifiers = new HashSet<>(); if (user.isInstitutionAdmin()) { Set applicationManageIdentifiers = user.getApplications().stream().map(m -> (String) m.get("id")).collect(Collectors.toSet()); @@ -97,7 +107,7 @@ public ResponseEntity> rolesByApplication(@Parameter(hidden = true) U manageIdentifiers.addAll(roleManageIdentifiers); List 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 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 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 index 11397c51..7f9709a5 100644 --- 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 index 30ab8e6b..41dc1eca 100644 --- 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 { List findByApplicationUsagesApplicationManageId(String manageId); - List findByOrganizationGUID_ApplicationUsagesApplicationManageId(String organizationGUID, String manageId); + List findByOrganizationGUID(String organizationGUID); List findByName(String name); diff --git a/server/src/main/java/access/security/CustomOidcUserService.java b/server/src/main/java/access/security/CustomOidcUserService.java index d7dea099..94825af2 100644 --- 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 index 91738444..ac59b1b8 100755 --- 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> optionalIdentityProvider = manage.identityProviderByInstitutionalGUID(organizationGUID); - user.setInstitution(optionalIdentityProvider.orElse(null)); - List> applications = optionalIdentityProvider.map(manage::providersAllowedByIdP).orElse(Collections.emptyList()); - user.setApplications(applications); + private void addInstitutionAdminAttributes(User user, String organizationGUID) { + Map attributes = manage.enrichInstitutionAdmin(organizationGUID); + user.updateRemoteAttributes(attributes); } } \ No newline at end of file diff --git a/server/src/main/java/access/security/UserPermissions.java b/server/src/main/java/access/security/UserPermissions.java index 11874bd5..c3f1ea88 100644 --- 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 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> applications, List manageIdentifiers) { return applications.stream() diff --git a/server/src/main/resources/application-devconf.yml b/server/src/main/resources/application-devconf.yml index c08bd6d8..a6976628 100644 --- 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 index 56a536e4..de36f62d 100644 --- 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 index 621c1605..089e462e 100644 --- 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); diff --git a/server/src/test/java/access/api/RoleControllerLimitInstitutionAdminVisibilityTest.java b/server/src/test/java/access/api/RoleControllerLimitInstitutionAdminVisibilityTest.java new file mode 100644 index 00000000..d4aad407 --- /dev/null +++ b/server/src/test/java/access/api/RoleControllerLimitInstitutionAdminVisibilityTest.java @@ -0,0 +1,125 @@ +package access.api; + +import access.AbstractTest; +import access.AccessCookieFilter; +import access.manage.EntityType; +import access.model.Application; +import access.model.ApplicationUsage; +import access.model.RemoteProvisionedGroup; +import access.model.Role; +import io.restassured.common.mapper.TypeRef; +import io.restassured.http.ContentType; +import org.junit.jupiter.api.Test; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.http.HttpStatus; + +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; + +import static access.security.SecurityConfig.API_TOKEN_HEADER; +import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +//Subclass properties are not merged, so we copy all from AbstractTest and add limit-institution-admin-role-visibility +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, + properties = { + "oidcng.introspect-url=http://localhost:8081/introspect", + "config.past-date-allowed=False", + "spring.security.oauth2.client.provider.oidcng.authorization-uri=http://localhost:8081/authorization", + "spring.security.oauth2.client.provider.oidcng.token-uri=http://localhost:8081/token", + "spring.security.oauth2.client.provider.oidcng.user-info-uri=http://localhost:8081/user-info", + "spring.security.oauth2.client.provider.oidcng.jwk-set-uri=http://localhost:8081/jwk-set", + "manage.url: http://localhost:8081", + "manage.enabled: true", + "feature.limit-institution-admin-role-visibility=true" + }) +class RoleControllerLimitInstitutionAdminVisibilityTest extends AbstractTest { + @Test + void rolesByApplicationInstitutionAdminByAPI() throws Exception { + super.stubForManagerProvidersByIdIn(EntityType.SAML20_SP, List.of("1", "2")); + super.stubForManagerProvidersByIdIn(EntityType.OIDC10_RP, List.of("5")); + super.stubForManageProvidersAllowedByIdP(ORGANISATION_GUID); + + List roles = given() + .when() + .header(API_TOKEN_HEADER, API_TOKEN_HASH) + .accept(ContentType.JSON) + .contentType(ContentType.JSON) + .get("/api/external/v1/roles") + .as(new TypeRef<>() { + }); + assertEquals(2, roles.size()); + } + + @Test + void rolesByApplicationInstitutionAdmin() throws Exception { + super.stubForManagerProvidersByIdIn(EntityType.SAML20_SP, List.of("1", "2")); + super.stubForManagerProvidersByIdIn(EntityType.OIDC10_RP, List.of("5")); + + super.stubForManageProvidersAllowedByIdP(ORGANISATION_GUID); + + AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/me", INSTITUTION_ADMIN_SUB, + institutionalAdminEntitlementOperator(ORGANISATION_GUID)); + List 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(2, roles.size()); + } + + @Test + void rolesByApplication() throws Exception { + //Because the user is changed and provisionings are queried + stubForManageProvisioning(List.of()); + stubForManageProvidersAllowedByIdP(ORGANISATION_GUID); + stubForManagerProvidersByIdIn(EntityType.OIDC10_RP, List.of("5")); + stubForManagerProvidersByIdIn(EntityType.SAML20_SP, List.of("1", "2")); + + AccessCookieFilter accessCookieFilter = openIDConnectFlow("/api/v1/users/login", INSTITUTION_ADMIN_SUB); + + super.stubForManagerProvidersByIdIn(EntityType.SAML20_SP, List.of("1")); + + List 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(2, roles.size()); + } + + @Test + void deleteRoleWithAPI() throws Exception { + Role role = roleRepository.search("network", 1).get(0); + //Ensure delete provisioning is done + remoteProvisionedGroupRepository.save(new RemoteProvisionedGroup(role, UUID.randomUUID().toString(), "7")); + Application application = role.applicationsUsed().iterator().next(); + super.stubForManagerProvidersByIdIn(application.getManageType(), List.of(application.getManageId())); + super.stubForManageProvidersAllowedByIdP(ORGANISATION_GUID); + super.stubForManageProvisioning(List.of(application.getManageId())); + super.stubForDeleteScimRole(); + + given() + .when() + .accept(ContentType.JSON) + .header(API_TOKEN_HEADER, API_TOKEN_HASH) + .contentType(ContentType.JSON) + .pathParams("id", role.getId()) + .delete("/api/external/v1/roles/{id}") + .then() + .statusCode(204); + assertEquals(0, roleRepository.search("network", 1).size()); + } + +} \ No newline at end of file