Skip to content

Commit

Permalink
Handling of changed profile information during sign-up when review pr…
Browse files Browse the repository at this point in the history 
…ofile is set to "missing" or T&C is requested
  • Loading branch information
cgeorgilakis-grnet committed Dec 13, 2024
1 parent c1ddf8e commit 33c53bf
Show file tree
Hide file tree
Showing 10 changed files with 78 additions and 21 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ Full Keycloak upstream jira issue can be shown if filtered by Fix version.

Our Keycloak version is working well with PostgreSQL database. For using other SQL databases, text field in database need to be evaluated.

## [Unreleased]
### Fixed
- Handling of changed profile information during sign-up when review profile is set to "missing" or T&C is requested

## [22.0.11-1.11] - 2024-11-19

### Added
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@

<quarkus.version>3.2.12.Final</quarkus.version>
<quarkus.build.version>3.2.12.Final</quarkus.build.version>
<eosc-kc.version>${project.version}-1.11</eosc-kc.version>
<eosc-kc.version>${project.version}-1.12rc1</eosc-kc.version>

<project.build-time>${timestamp}</project.build-time>

Expand Down
48 changes: 42 additions & 6 deletions ...java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,7 @@ protected boolean requiresUpdateProfilePage( AuthenticatorConfigModel authentica
return true;
}

String updateProfileFirstLogin;
if (authenticatorConfig == null || !authenticatorConfig.getConfig().containsKey(IdpReviewProfileAuthenticatorFactory.UPDATE_PROFILE_ON_FIRST_LOGIN)) {
updateProfileFirstLogin = IdentityProviderRepresentation.UPFLM_MISSING;
} else {
updateProfileFirstLogin = authenticatorConfig.getConfig().get(IdpReviewProfileAuthenticatorFactory.UPDATE_PROFILE_ON_FIRST_LOGIN);
}
String updateProfileFirstLogin = calculateUpdateProfileFirstLogin(authenticatorConfig);

if(IdentityProviderRepresentation.UPFLM_MISSING.equals(updateProfileFirstLogin)) {
try {
Expand All @@ -120,6 +115,14 @@ protected boolean requiresUpdateProfilePage( AuthenticatorConfigModel authentica
}
}

private String calculateUpdateProfileFirstLogin(AuthenticatorConfigModel authenticatorConfig) {
if (authenticatorConfig == null || !authenticatorConfig.getConfig().containsKey(IdpReviewProfileAuthenticatorFactory.UPDATE_PROFILE_ON_FIRST_LOGIN)) {
return IdentityProviderRepresentation.UPFLM_MISSING;
} else {
return authenticatorConfig.getConfig().get(IdpReviewProfileAuthenticatorFactory.UPDATE_PROFILE_ON_FIRST_LOGIN);
}
}

@Override
protected void actionImpl(AuthenticationFlowContext context, SerializedBrokeredIdentityContext userCtx, BrokeredIdentityContext brokerContext) {
EventBuilder event = context.getEvent();
Expand Down Expand Up @@ -190,6 +193,39 @@ public String getServiceAccountClientLink() {
attributes.remove(TERMS_FIELD);
attributes.put(TermsAndConditions.USER_ATTRIBUTE, Arrays.asList(Integer.toString(Time.currentTime())));
}
//for security reason keep old values equal to broken data
String updateProfileFirstLogin = calculateUpdateProfileFirstLogin(context.getAuthenticatorConfig());
if (IdentityProviderRepresentation.UPFLM_MISSING.equals(updateProfileFirstLogin)) {
if (userCtx.getUsername() != null) {
attributes.put("username", Stream.of(userCtx.getUsername()).toList());
}
if (userCtx.getEmail() != null) {
attributes.put("email", Stream.of(userCtx.getEmail()).toList());
}
if (userCtx.getFirstName() != null) {
attributes.put("firstName", Stream.of(userCtx.getFirstName()).toList());
}
if (userCtx.getLastName() != null) {
attributes.put("lastName", Stream.of(userCtx.getLastName()).toList());
}
} else if (IdentityProviderRepresentation.UPFLM_OFF.equals(updateProfileFirstLogin)) {
attributes.put("username", Stream.of(userCtx.getUsername()).toList());
if (userCtx.getEmail() != null) {
attributes.put("email", Stream.of(userCtx.getEmail()).toList());
} else {
attributes.remove("email");
}
if (userCtx.getFirstName() != null) {
attributes.put("firstName", Stream.of(userCtx.getFirstName()).toList());
} else {
attributes.remove("firstName");
}
if (userCtx.getLastName() != null) {
attributes.put("lastName", Stream.of(userCtx.getLastName()).toList());
} else {
attributes.remove("lastName");
}
}
UserProfile profile = profileProvider.create(UserProfileContext.IDP_REVIEW, attributes, updatedProfile, formData.containsKey(TERMS_FIELD));

try {
Expand Down
2 changes: 1 addition & 1 deletion ...uillian/tests/base/src/main/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public String getDepartment() {
}

public boolean isDepartmentEnabled() {
return departmentInput.getAttribute("readOnly") == null;
return departmentInput.isEnabled();
}

public boolean isCurrent() {
Expand Down
4 changes: 2 additions & 2 deletions ...n-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/VerifyProfilePage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public String getDepartment() {
}

public boolean isDepartmentEnabled() {
return departmentInput.getAttribute("readOnly") == null;
return departmentInput.isEnabled();
}

public boolean isUsernamePresent() {
Expand All @@ -142,7 +142,7 @@ public boolean isEmailPresent() {

public boolean isUsernameEnabled() {
try {
return driver.findElement(By.id("username")).getAttribute("readOnly") == null;
return driver.findElement(By.id("username")).isEnabled();
} catch (NoSuchElementException nse) {
return false;
}
Expand Down
6 changes: 4 additions & 2 deletions .../tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public void testErrorExistingUserWithUpdateProfile() {

waitForPage(driver, "account already exists", false);
assertTrue(idpConfirmLinkPage.isCurrent());
assertEquals("User with username consumer already exists. How do you want to continue?", idpConfirmLinkPage.getMessage());
assertEquals("User with email [email protected] already exists. How do you want to continue?", idpConfirmLinkPage.getMessage());
}


Expand Down Expand Up @@ -870,6 +870,8 @@ public void testVerifyEmailRequiredActionWhenChangingEmailDuringFirstLogin() {

realm.update(realmRep);

updateExecutions(AbstractBrokerTest::enableUpdateProfileOnFirstLogin);

IdentityProviderRepresentation idpRep = identityProviderResource.toRepresentation();

idpRep.setTrustEmail(true);
Expand Down Expand Up @@ -1088,7 +1090,7 @@ public void testEventsOnUpdateProfileNoEmailChange() {

@Test
public void testEventsOnUpdateProfileWithEmailChange() {
updateExecutions(AbstractBrokerTest::setUpMissingUpdateProfileOnFirstLogin);
updateExecutions(AbstractBrokerTest::enableUpdateProfileOnFirstLogin);

createUser(bc.providerRealmName(), "no-first-name", "password", null, "LastName", "[email protected]");

Expand Down
2 changes: 1 addition & 1 deletion ...an/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ public void testLoginWithDifferentBrokerWhenUpdatingProfile() {

@Test
public void testEditUsername() {
updateExecutions(AbstractBrokerTest::setUpMissingUpdateProfileOnFirstLogin);
updateExecutions(AbstractBrokerTest::enableUpdateProfileOnFirstLogin);

createUser(bc.providerRealmName(), "no-first-name", "password", null,
"LastName", "[email protected]");
Expand Down
15 changes: 15 additions & 0 deletions ...ian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlIdPInitiatedSsoTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.keycloak.testsuite.broker;

import org.keycloak.admin.client.resource.AuthenticationManagementResource;
import org.keycloak.admin.client.resource.ClientsResource;
import org.keycloak.admin.client.resource.IdentityProviderResource;
import org.keycloak.admin.client.resource.UsersResource;
Expand All @@ -13,7 +14,9 @@
import org.keycloak.dom.saml.v2.assertion.NameIDType;
import org.keycloak.dom.saml.v2.assertion.StatementAbstractType;
import org.keycloak.dom.saml.v2.protocol.ResponseType;
import org.keycloak.models.utils.DefaultAuthenticationFlows;
import org.keycloak.protocol.saml.SamlPrincipalType;
import org.keycloak.representations.idm.AuthenticationExecutionInfoRepresentation;
import org.keycloak.representations.idm.ClientRepresentation;
import org.keycloak.representations.idm.FederatedIdentityRepresentation;
import org.keycloak.representations.idm.IdentityProviderRepresentation;
Expand All @@ -39,6 +42,7 @@
import java.io.InputStream;
import java.net.URI;
import java.util.*;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;
import jakarta.ws.rs.core.Response;
import org.jboss.arquillian.graphene.page.Page;
Expand Down Expand Up @@ -115,6 +119,8 @@ public void initRealmUrls() {
urlRealmProvider = getAuthRoot() + "/auth/realms/" + REALM_PROV_NAME;
urlRealmConsumer = getAuthRoot() + "/auth/realms/" + REALM_CONS_NAME;
urlRealmConsumer2 = getAuthRoot() + "/auth/realms/" + REALM_CONS_NAME + "-2";

updateExecutions(AbstractBrokerTest::enableUpdateProfileOnFirstLogin);
}

@Before
Expand Down Expand Up @@ -146,6 +152,14 @@ public void addTestRealms(List<RealmRepresentation> testRealms) {
testRealms.add(loadFromClasspath("kc3731-broker-realm.json", p));
}

private void updateExecutions(BiConsumer< AuthenticationExecutionInfoRepresentation, AuthenticationManagementResource > action) {
AuthenticationManagementResource flows = adminClient.realm(REALM_PROV_NAME).flows();

for (AuthenticationExecutionInfoRepresentation execution : flows.getExecutions(DefaultAuthenticationFlows.FIRST_BROKER_LOGIN_FLOW)) {
action.accept(execution, flows);
}
}

@Test
public void testProviderIdpInitiatedLogin() throws Exception {
driver.navigate().to(getSamlIdpInitiatedUrl(REALM_PROV_NAME, "samlbroker"));
Expand Down Expand Up @@ -291,6 +305,7 @@ public void testConsumerIdpInitiatedLoginToApp() throws Exception {

@Test
public void testTwoConsequentIdpInitiatedLogins() throws Exception {

SAMLDocumentHolder samlResponse = new SamlClientBuilder()
.navigateTo(getSamlIdpInitiatedUrl(REALM_PROV_NAME, "samlbroker"))
// Login in provider realm
Expand Down
8 changes: 4 additions & 4 deletions themes/src/main/resources/theme/base/login/login-update-profile.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
</div>
<div class="${properties.kcInputWrapperClass!}">
<input type="text" id="username" name="username" value="${(user.username!'')}"
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>readonly</#if>
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>disabled</#if>
aria-invalid="<#if messagesPerField.existsError('username')>true</#if>"
/>

Expand All @@ -31,7 +31,7 @@
</div>
<div class="${properties.kcInputWrapperClass!}">
<input type="text" id="email" name="email" value="${(user.email!'')}"
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>readonly</#if>
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>disabled</#if>
aria-invalid="<#if messagesPerField.existsError('email')>true</#if>"
/>

Expand All @@ -50,7 +50,7 @@
</div>
<div class="${properties.kcInputWrapperClass!}">
<input type="text" id="firstName" name="firstName" value="${(user.firstName!'')}"
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>readonly</#if>
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>disabled</#if>
aria-invalid="<#if messagesPerField.existsError('firstName')>true</#if>"
/>

Expand All @@ -68,7 +68,7 @@
</div>
<div class="${properties.kcInputWrapperClass!}">
<input type="text" id="lastName" name="lastName" value="${(user.lastName!'')}"
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>readonly</#if>
class="${properties.kcInputClass!}" <#if isUpdateProfile?has_content && !isUpdateProfile>disabled</#if>
aria-invalid="<#if messagesPerField.existsError('lastName')>true</#if>"
/>

Expand Down
8 changes: 4 additions & 4 deletions themes/src/main/resources/theme/base/login/user-profile-commons.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
<#macro inputTag attribute>
<input type="<@inputTagType attribute=attribute/>" id="${attribute.name}" name="${attribute.name}" value="${(attribute.value!'')}" class="${properties.kcInputClass!}"
aria-invalid="<#if messagesPerField.existsError('${attribute.name}')>true</#if>"
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>readOnly</#if>
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>disabled</#if>
<#if attribute.autocomplete??>autocomplete="${attribute.autocomplete}"</#if>
<#if attribute.annotations.inputTypePlaceholder??>placeholder="${attribute.annotations.inputTypePlaceholder}"</#if>
<#if attribute.annotations.inputTypePattern??>pattern="${attribute.annotations.inputTypePattern}"</#if>
Expand Down Expand Up @@ -106,7 +106,7 @@
<#macro textareaTag attribute>
<textarea id="${attribute.name}" name="${attribute.name}" class="${properties.kcInputClass!}"
aria-invalid="<#if messagesPerField.existsError('${attribute.name}')>true</#if>"
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>readOnly</#if>
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>disabled</#if>
<#if attribute.annotations.inputTypeCols??>cols="${attribute.annotations.inputTypeCols}"</#if>
<#if attribute.annotations.inputTypeRows??>rows="${attribute.annotations.inputTypeRows}"</#if>
<#if attribute.annotations.inputTypeMaxlength??>maxlength="${attribute.annotations.inputTypeMaxlength}"</#if>
Expand All @@ -116,7 +116,7 @@
<#macro selectTag attribute>
<select id="${attribute.name}" name="${attribute.name}" class="${properties.kcInputClass!}"
aria-invalid="<#if messagesPerField.existsError('${attribute.name}')>true</#if>"
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>readOnly</#if>
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>disabled</#if>
<#if attribute.annotations.inputType=='multiselect'>multiple</#if>
<#if attribute.annotations.inputTypeSize??>size="${attribute.annotations.inputTypeSize}"</#if>
>
Expand Down Expand Up @@ -162,7 +162,7 @@
<div class="${classDiv}">
<input type="${inputType}" id="${attribute.name}-${option}" name="${attribute.name}" value="${option}" class="${classInput}"
aria-invalid="<#if messagesPerField.existsError('${attribute.name}')>true</#if>"
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>readOnly</#if>
<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)>disabled</#if>
<#if attribute.values?seq_contains(option)>checked</#if>
/>
<label for="${attribute.name}-${option}" class="${classLabel}<#if attribute.readOnly || (isUpdateProfile?has_content && !isUpdateProfile)> ${properties.kcInputClassRadioCheckboxLabelDisabled!}</#if>"><@selectOptionLabelText attribute=attribute option=option/></label>
Expand Down

0 comments on commit 33c53bf

Please sign in to comment.