From 2a4e6f294470184af0de07618501cfc78e1a6367 Mon Sep 17 00:00:00 2001 From: Joerg Matysiak
Date: Tue, 24 May 2022 16:59:38 +0200 Subject: [PATCH] Allow permission configuration for username and email in user profile. Enhanced Account API to respect access to these attributes. Resolves #12599 --- .../userprofile/AttributeMetadata.java | 40 ++++++++-- .../DeclarativeUserProfileProvider.java | 25 ++++-- .../testsuite/pages/VerifyProfilePage.java | 8 ++ ...AccountRestServiceWithUserProfileTest.java | 37 ++++++++- .../testsuite/forms/VerifyProfileTest.java | 57 +++++++++++++ .../user/profile/UserProfileTest.java | 79 +++++++++++++++++++ .../partials/realm-user-profile.html | 2 +- .../app/content/account-page/AccountPage.tsx | 2 +- 8 files changed, 232 insertions(+), 18 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/userprofile/AttributeMetadata.java b/server-spi-private/src/main/java/org/keycloak/userprofile/AttributeMetadata.java index b4717024ddef..32761201414f 100644 --- a/server-spi-private/src/main/java/org/keycloak/userprofile/AttributeMetadata.java +++ b/server-spi-private/src/main/java/org/keycloak/userprofile/AttributeMetadata.java @@ -45,10 +45,10 @@ public final class AttributeMetadata { private String attributeDisplayName; private AttributeGroupMetadata attributeGroupMetadata; private final Predicate
selector; - private final Predicate
writeAllowed; + private final List
> writeAllowed = new ArrayList<>(); /** Predicate to decide if attribute is required, it is handled as required if predicate is null */ private final Predicate
required; - private final Predicate
readAllowed; + private final List
> readAllowed = new ArrayList<>(); private List
validators; private Map
annotations; private int guiOrder; @@ -93,11 +93,22 @@ public final class AttributeMetadata { Predicate
required, Predicate
readAllowed) { this.attributeName = attributeName; + this.guiOrder = guiOrder; this.selector = selector; - this.writeAllowed = writeAllowed; + addWriteCondition(writeAllowed); this.required = required; - this.readAllowed = readAllowed; + addReadCondition(readAllowed); + } + + AttributeMetadata(String attributeName, int guiOrder, Predicate
selector, List
> writeAllowed, + Predicate
required, + List
> readAllowed) { + this.attributeName = attributeName; this.guiOrder = guiOrder; + this.selector = selector; + this.writeAllowed.addAll(writeAllowed); + this.required = required; + this.readAllowed.addAll(readAllowed); } public String getName() { @@ -116,21 +127,34 @@ public AttributeMetadata setGuiOrder(int guiOrder) { public AttributeGroupMetadata getAttributeGroupMetadata() { return attributeGroupMetadata; } - + public boolean isSelected(AttributeContext context) { return selector.test(context); } + private boolean allConditionsMet(List
> predicates, AttributeContext context) { + return predicates.stream().allMatch(p -> p.test(context)); + } + + public AttributeMetadata addReadCondition(Predicate
readAllowed) { + this.readAllowed.add(readAllowed); + return this; + } + + public AttributeMetadata addWriteCondition(Predicate
writeAllowed) { + this.writeAllowed.add(writeAllowed); + return this; + } public boolean isReadOnly(AttributeContext context) { - return !writeAllowed.test(context); + return !canEdit(context); } public boolean canView(AttributeContext context) { - return readAllowed.test(context); + return allConditionsMet(readAllowed, context); } public boolean canEdit(AttributeContext context) { - return writeAllowed.test(context); + return allConditionsMet(writeAllowed, context); } /** diff --git a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java index c53323912168..156ef7e4a425 100644 --- a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java +++ b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java @@ -277,6 +277,7 @@ protected UserProfileMetadata decorateUserProfileForCache(UserProfileMetadata de } Map
groupsByName = asHashMap(parsedConfig.getGroups()); + RealmModel realm = session.getContext().getRealm(); int guiOrder = 0; for (UPAttribute attrConfig : parsedConfig.getAttributes()) { @@ -343,10 +344,19 @@ protected UserProfileMetadata decorateUserProfileForCache(UserProfileMetadata de // make sure username and email are writable if permissions are not set if (permissions == null || permissions.isEmpty()) { writeAllowed = AttributeMetadata.ALWAYS_TRUE; + readAllowed = AttributeMetadata.ALWAYS_TRUE; } List
atts = decoratedMetadata.getAttribute(attributeName); + // Add ImmutableAttributeValidator to ensure that attributes that are configured + // as read-only are marked as such. + // Skip this for username in realms with username = email to allow change of email + // address on initial login with profile via idp + if (!realm.isRegistrationEmailAsUsername() || !UserModel.USERNAME.equals(attributeName)) { + validators.add(new AttributeValidatorMetadata(ImmutableAttributeValidator.ID)); + } + if (atts.isEmpty()) { // attribute metadata doesn't exist so we have to add it. We keep it optional as Abstract base // doesn't require it. @@ -356,12 +366,17 @@ protected UserProfileMetadata decorateUserProfileForCache(UserProfileMetadata de .setAttributeGroupMetadata(groupMetadata); } else { final int localGuiOrder = guiOrder++; - // only add configured validators and annotations if attribute metadata exist + Predicate
readAllowedFinal = readAllowed; + Predicate
writeAllowedFinal = writeAllowed; + + // add configured validators and annotations to existing attribute metadata atts.stream().forEach(c -> c.addValidator(validators) - .addAnnotations(annotations) - .setAttributeDisplayName(attrConfig.getDisplayName()) - .setGuiOrder(localGuiOrder) - .setAttributeGroupMetadata(groupMetadata)); + .addAnnotations(annotations) + .setAttributeDisplayName(attrConfig.getDisplayName()) + .setGuiOrder(localGuiOrder) + .setAttributeGroupMetadata(groupMetadata) + .addReadCondition(readAllowedFinal) + .addWriteCondition(writeAllowedFinal)); } } else { // always add validation for immutable/read-only attributes diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/VerifyProfilePage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/VerifyProfilePage.java index a916655d53d3..b86e31a4e140 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/VerifyProfilePage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/VerifyProfilePage.java @@ -132,6 +132,14 @@ public boolean isUsernamePresent() { } } + public boolean isUsernameEnabled() { + try { + return driver.findElement(By.id("username")).isEnabled(); + } catch (NoSuchElementException nse) { + return false; + } + } + public boolean isDepartmentPresent() { try { isDepartmentEnabled(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java index a97593be68ae..e303f3febb72 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java @@ -66,7 +66,7 @@ protected boolean isDeclarativeUserProfile() { return true; } - private static String UP_CONFIG_FOR_METADATA = "{\"attributes\": [" + private final static String UP_CONFIG_FOR_METADATA = "{\"attributes\": [" + "{\"name\": \"firstName\"," + PERMISSIONS_ALL + ", \"required\": {\"scopes\":[\"profile\"]}, \"displayName\": \"${profile.firstName}\", \"validations\": {\"length\": { \"max\": 255 }}}," + "{\"name\": \"lastName\"," + PERMISSIONS_ALL + ", \"required\": {}, \"displayName\": \"Last name\", \"annotations\": {\"formHintKey\" : \"userEmailFormFieldHint\", \"anotherKey\" : 10, \"yetAnotherKey\" : \"some value\"}}," + "{\"name\": \"attr_with_scope_selector\"," + PERMISSIONS_ALL + ", \"selector\": {\"scopes\": [\"profile\"]}}," @@ -78,20 +78,26 @@ protected boolean isDeclarativeUserProfile() { + "{\"name\": \"attr_no_permission\"," + PERMISSIONS_ADMIN_ONLY + "}" + "]}"; - private static String UP_CONFIG_NO_ACCESS_TO_NAME_FIELDS = "{\"attributes\": [" + private final static String UP_CONFIG_NO_ACCESS_TO_NAME_FIELDS = "{\"attributes\": [" + "{\"name\": \"firstName\"," + PERMISSIONS_ADMIN_ONLY + ", \"required\": {}, \"displayName\": \"${profile.firstName}\", \"validations\": {\"length\": { \"max\": 255 }}}," + "{\"name\": \"lastName\"," + PERMISSIONS_ADMIN_ONLY + ", \"required\": {}, \"displayName\": \"Last name\", \"annotations\": {\"formHintKey\" : \"userEmailFormFieldHint\", \"anotherKey\" : 10, \"yetAnotherKey\" : \"some value\"}}," + "{\"name\": \"attr_readonly\"," + PERMISSIONS_ADMIN_EDITABLE + "}," + "{\"name\": \"attr_no_permission\"," + PERMISSIONS_ADMIN_ONLY + "}" + "]}"; - private static String UP_CONFIG_RO_ACCESS_TO_NAME_FIELDS = "{\"attributes\": [" + private final static String UP_CONFIG_RO_ACCESS_TO_NAME_FIELDS = "{\"attributes\": [" + "{\"name\": \"firstName\"," + PERMISSIONS_ADMIN_EDITABLE + ", \"required\": {}, \"displayName\": \"${profile.firstName}\", \"validations\": {\"length\": { \"max\": 255 }}}," + "{\"name\": \"lastName\"," + PERMISSIONS_ADMIN_EDITABLE + ", \"required\": {}, \"displayName\": \"Last name\", \"annotations\": {\"formHintKey\" : \"userEmailFormFieldHint\", \"anotherKey\" : 10, \"yetAnotherKey\" : \"some value\"}}," + "{\"name\": \"attr_readonly\"," + PERMISSIONS_ADMIN_EDITABLE + "}," + "{\"name\": \"attr_no_permission\"," + PERMISSIONS_ADMIN_ONLY + "}" + "]}"; + private final static String UP_CONFIG_RO_USERNAME_AND_EMAIL = "{\"attributes\": [" + + "{\"name\": \"email\"," + PERMISSIONS_ADMIN_EDITABLE + ", \"required\": {}, \"displayName\": \"${email}\", \"annotations\": {\"formHintKey\" : \"userEmailFormFieldHint\", \"anotherKey\" : 10, \"yetAnotherKey\" : \"some value\"}}," + + "{\"name\": \"attr_readonly\"," + PERMISSIONS_ADMIN_EDITABLE + "}," + + "{\"name\": \"attr_no_permission\"," + PERMISSIONS_ADMIN_ONLY + "}" + + "]}"; + @Test @Override @@ -187,6 +193,31 @@ public void testGetUserProfileMetadata_RoAccessToNameFields() throws IOException } } + @Test + public void testGetUserProfileMetadata_RoAccessToUsernameAndEmail() throws IOException { + + try { + RealmRepresentation realmRep = adminClient.realm("test").toRepresentation(); + realmRep.setEditUsernameAllowed(false); + adminClient.realm("test").update(realmRep); + + setUserProfileConfiguration(UP_CONFIG_RO_USERNAME_AND_EMAIL); + + UserRepresentation user = getUser(); + assertNotNull(user.getUserProfileMetadata()); + + assertUserProfileAttributeMetadata(user, "username", "${username}", true, true); + assertUserProfileAttributeMetadata(user, "email", "${email}", true, true); + + assertUserProfileAttributeMetadata(user, "attr_readonly", "attr_readonly", false, true); + assertNull(getUserProfileAttributeMetadata(user, "attr_no_permission")); + } finally { + RealmRepresentation realmRep = testRealm().toRepresentation(); + realmRep.setEditUsernameAllowed(true); + testRealm().update(realmRep); + } + } + @Test @Override diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/VerifyProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/VerifyProfileTest.java index ca4aca76387c..2b78eaf2ea42 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/VerifyProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/VerifyProfileTest.java @@ -532,6 +532,63 @@ public void testRequiredReadOnlyAttribute() { assertEquals("Last", user.getLastName()); } + @Test + public void testAdminOnlyAttributeNotVisibleToUser() { + + setUserProfileConfiguration("{\"attributes\": [" + + "{\"name\": \"firstName\"," + PERMISSIONS_ALL + ", \"required\": {}}," + + "{\"name\": \"lastName\"," + PERMISSIONS_ALL + "}," + + "{\"name\": \"department\"," + PERMISSIONS_ADMIN_ONLY + "}," + + "{\"name\": \"requiredAttrToTriggerVerifyPage\"," + PERMISSIONS_ALL + ", \"required\": {}}" + + "]}"); + + loginPage.open(); + loginPage.login("login-test6", "password"); + + verifyProfilePage.assertCurrent(); + Assert.assertEquals("ExistingLast", verifyProfilePage.getLastName()); + Assert.assertFalse("Admin-only attribute should not be visible for user", verifyProfilePage.isDepartmentPresent()); + } + + + @Test + public void testUsernameReadOnlyInProfile() { + + setUserProfileConfiguration("{\"attributes\": [" + + "{\"name\": \"firstName\"," + PERMISSIONS_ALL + ", \"required\": {}}," + + "{\"name\": \"lastName\"," + PERMISSIONS_ALL + "}," + + "{\"name\": \"username\"," + PERMISSIONS_ADMIN_EDITABLE + "}," + + "{\"name\": \"requiredAttrToTriggerVerifyPage\"," + PERMISSIONS_ALL + ", \"required\": {}}" + + "]}"); + + loginPage.open(); + loginPage.login("login-test6", "password"); + + verifyProfilePage.assertCurrent(); + Assert.assertEquals("ExistingLast", verifyProfilePage.getLastName()); + + Assert.assertFalse("username should not be editable by user", verifyProfilePage.isUsernameEnabled()); + } + + @Test + public void testUsernameReadNotVisibleInProfile() { + + setUserProfileConfiguration("{\"attributes\": [" + + "{\"name\": \"firstName\"," + PERMISSIONS_ALL + ", \"required\": {}}," + + "{\"name\": \"lastName\"," + PERMISSIONS_ALL + "}," + + "{\"name\": \"username\"," + PERMISSIONS_ADMIN_ONLY + "}," + + "{\"name\": \"requiredAttrToTriggerVerifyPage\"," + PERMISSIONS_ALL + ", \"required\": {}}" + + "]}"); + + loginPage.open(); + loginPage.login("login-test6", "password"); + + verifyProfilePage.assertCurrent(); + Assert.assertEquals("ExistingLast", verifyProfilePage.getLastName()); + + Assert.assertFalse("username should not be shown to user", verifyProfilePage.isUsernamePresent()); + } + @Test public void testAttributeNotVisible() { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java index bed6fb1355f3..7e1ac36f6d7a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java @@ -535,6 +535,85 @@ private static void testReadonlyUpdates(KeycloakSession session) { assertTrue(profile.getAttributes().isReadOnly("department")); } + @Test + public void testReadonlyEmailCannotBeUpdated() { + getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) UserProfileTest::testReadonlyEmailCannotBeUpdated); + } + + private static void testReadonlyEmailCannotBeUpdated(KeycloakSession session) { + Map
attributes = new HashMap<>(); + + attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId()); + attributes.put(UserModel.EMAIL, "
[email protected]
"); + + UserProfileProvider provider = getDynamicUserProfileProvider(session); + + // configure email r/o for user + provider.setConfiguration("{\"attributes\": [{\"name\": \"email\", \"permissions\": {\"edit\": [ \"admin\"]}}]}"); + + UserProfile profile = provider.create(UserProfileContext.ACCOUNT, attributes); + UserModel user = profile.create(); + + assertThat(profile.getAttributes().nameSet(), + containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL)); + + profile = provider.create(UserProfileContext.USER_API, attributes, user); + + Set
attributesUpdated = new HashSet<>(); + + profile.update((attributeName, userModel, oldValue) -> assertTrue(attributesUpdated.add(attributeName))); + + attributes.put(UserModel.EMAIL, "
[email protected]
"); + + profile = provider.create(UserProfileContext.ACCOUNT, attributes, user); + + try { + profile.update(); + fail("Should fail since email is read only"); + } catch (ValidationException ve) { + assertTrue(ve.isAttributeOnError("email")); + } + + assertEquals("E-Mail address shouldn't be changed", "
[email protected]
", user.getEmail()); + } + + @Test + public void testUpdateEmail() { + getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) UserProfileTest::testUpdateEmail); + } + + private static void testUpdateEmail(KeycloakSession session) { + Map
attributes = new HashMap<>(); + + attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId()); + attributes.put(UserModel.EMAIL, "
[email protected]
"); + + UserProfileProvider provider = getDynamicUserProfileProvider(session); + + // configure email r/w for user + provider.setConfiguration("{\"attributes\": [{\"name\": \"email\", \"permissions\": {\"edit\": [ \"user\", \"admin\"]}}]}"); + + UserProfile profile = provider.create(UserProfileContext.ACCOUNT, attributes); + UserModel user = profile.create(); + + assertThat(profile.getAttributes().nameSet(), + containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL)); + + profile = provider.create(UserProfileContext.USER_API, attributes, user); + + Set
attributesUpdated = new HashSet<>(); + + profile.update((attributeName, userModel, oldValue) -> assertTrue(attributesUpdated.add(attributeName))); + + attributes.put("email", "
[email protected]
"); + + profile = provider.create(UserProfileContext.ACCOUNT, attributes, user); + + profile.update(); + + assertEquals("E-Mail address should have been changed!", "
[email protected]
", user.getEmail()); + } + @Test public void testDoNotUpdateUndefinedAttributes() { getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) UserProfileTest::testDoNotUpdateUndefinedAttributes); diff --git a/themes/src/main/resources/theme/base/admin/resources/partials/realm-user-profile.html b/themes/src/main/resources/theme/base/admin/resources/partials/realm-user-profile.html index a8c9b03f5d20..df56951743bf 100755 --- a/themes/src/main/resources/theme/base/admin/resources/partials/realm-user-profile.html +++ b/themes/src/main/resources/theme/base/admin/resources/partials/realm-user-profile.html @@ -174,7 +174,7 @@
-
+
{{:: 'user.profile.attribute.permission' | translate}}
{{:: 'user.profile.attribute.canUserView' | translate}}
diff --git a/themes/src/main/resources/theme/keycloak.v2/account/src/app/content/account-page/AccountPage.tsx b/themes/src/main/resources/theme/keycloak.v2/account/src/app/content/account-page/AccountPage.tsx index 3a4174881104..292e413f959f 100644 --- a/themes/src/main/resources/theme/keycloak.v2/account/src/app/content/account-page/AccountPage.tsx +++ b/themes/src/main/resources/theme/keycloak.v2/account/src/app/content/account-page/AccountPage.tsx @@ -180,7 +180,7 @@ export class AccountPage extends React.Component
this.handleSubmit(event)} className="personal-info-form" > - {!this.isRegistrationEmailAsUsername && ( + {!this.isRegistrationEmailAsUsername && fields.username != undefined && (