Skip to content

Fixes in AuthenticationProvider. Fixing testsuite #303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stianst merged 1 commit into from
Mar 25, 2014
Merged

Fixes in AuthenticationProvider. Fixing testsuite #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 23 additions & 17 deletions services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,22 +234,26 @@ public AuthenticationStatus authenticateForm(RealmModel realm, MultivaluedMap<St
AuthenticationLinkModel authLink = new AuthenticationLinkModel(authResult.getProviderName(), authUser.getId());
user = realm.getUserByAuthenticationLink(authLink);
if (user == null) {
// Create new user, which has been successfully authenticated and link him with authentication provider
user = realm.addUser(authUser.getUsername());
user.setEnabled(true);
user.setFirstName(authUser.getFirstName());
user.setLastName(authUser.getLastName());
user.setEmail(authUser.getEmail());

realm.addAuthenticationLink(user, authLink);
logger.info("User " + username + " successfully authenticated and created based on provider " + authResult.getProviderName());
} else {
// Existing user has been authenticated
if (!checkEnabled(user)) {
return AuthenticationStatus.ACCOUNT_DISABLED;
user = KeycloakModelUtils.findUserByNameOrEmail(realm, username);
if (user != null) {
// Case when we already have user with the same username like authenticated, but he is not yet linked to current provider.
// TODO: Revisit if it's ok to link if we allow to change username. Maybe ask user?
// TODO: Update of existing account?
realm.addAuthenticationLink(user, authLink);
logger.info("User " + authUser.getUsername() + " successfully authenticated and linked with provider " + authResult.getProviderName());
} else {
// Create new user, which has been successfully authenticated and link him with authentication provider
user = realm.addUser(authUser.getUsername());
user.setEnabled(true);
user.setFirstName(authUser.getFirstName());
user.setLastName(authUser.getLastName());
user.setEmail(authUser.getEmail());

realm.addAuthenticationLink(user, authLink);
logger.info("User " + username + " successfully authenticated and created based on provider " + authResult.getProviderName());
}

// TODO: Update of existing account?
} else {
// Existing and linked user has been authenticated TODO: Update of existing account?
}

// Authenticated username could be different from the "form" username. In this case, we will change it
Expand All @@ -263,10 +267,12 @@ public AuthenticationStatus authenticateForm(RealmModel realm, MultivaluedMap<St
if (user == null) {
logger.warn("User '" + username + "' successfully authenticated, but he doesn't exists and don't know how to create him");
return AuthenticationStatus.INVALID_USER;
} else if (!checkEnabled(user)) {
return AuthenticationStatus.ACCOUNT_DISABLED;
}
}

if (!checkEnabled(user)) {
return AuthenticationStatus.ACCOUNT_DISABLED;
}
}

if (!user.getRequiredActions().isEmpty()) {
Expand Down
1 change: 0 additions & 1 deletion ...ain/java/org/keycloak/spi/authentication/picketlink/PicketlinkAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ public AuthResult validatePassword(RealmModel realm, Map<String, String> configu
result.setUser(authenticatedUser).setProviderName(getName());
return result;
} else {
logger.debugf("Username: %s, Credential status: %s", username, credential.getStatus());
return new AuthResult(AuthProviderStatus.IGNORE);
}
}
Expand Down
9 changes: 6 additions & 3 deletions ...tion-spi/src/main/java/org/keycloak/spi/authentication/AuthenticationProviderManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public AuthResult validatePassword(String username, String password) {

try {
AuthResult currentResult = delegate.validatePassword(realm, authProviderConfig.getConfig(), username, password);
logger.debugf("Authentication provider '%s' finished with '%s' for authentication of '%s'", delegate.getName(), currentResult.toString(), username);
logger.debugf("Authentication provider '%s' finished with '%s' for authentication of '%s'", delegate.getName(), currentResult.getAuthProviderStatus().toString(), username);

if (currentResult.getAuthProviderStatus() == AuthProviderStatus.SUCCESS || currentResult.getAuthProviderStatus() == AuthProviderStatus.FAILED) {
return currentResult;
Expand Down Expand Up @@ -90,8 +90,11 @@ public void updatePassword(String username, String password) throws Authenticati
}

try {
delegate.updateCredential(realm, authProviderConfig.getConfig(), username, password);
logger.debugf("Updated password in authentication provider '%s' for user '%s'", delegate.getName(), username);
if (delegate.updateCredential(realm, authProviderConfig.getConfig(), username, password)) {
logger.debugf("Updated password in authentication provider '%s' for user '%s'", delegate.getName(), username);
} else {
logger.debugf("Password not updated in authentication provider '%s' for user '%s'", delegate.getName(), username);
}
} catch (AuthenticationProviderException ape) {
// Rethrow it to upper layer
logger.warn("Failed to update password", ape);
Expand Down
14 changes: 10 additions & 4 deletions .../integration/src/test/java/org/keycloak/testsuite/forms/AuthProvidersIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ public void loginLdap() {

Assert.assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType());
Assert.assertNotNull(oauth.getCurrentQuery().get(OAuth2Constants.CODE));

profilePage.open();
Assert.assertEquals("John", profilePage.getFirstName());
Assert.assertEquals("Doe", profilePage.getLastName());
Assert.assertEquals("[email protected]", profilePage.getEmail());
}

@Test
Expand Down Expand Up @@ -166,7 +171,7 @@ public void config(RealmManager manager, RealmModel adminstrationRealm, RealmMod
}

@Test
public void passwordChangeLdap() {
public void passwordChangeLdap() throws Exception {
changePasswordPage.open();
loginPage.login("john", "password");
changePasswordPage.changePassword("password", "new-password", "new-password");
Expand All @@ -175,9 +180,10 @@ public void passwordChangeLdap() {

changePasswordPage.logout();

loginPage.open();
loginPage.login("john", "password");
Assert.assertEquals("Invalid username or password.", loginPage.getError());
// TODO: Disabled until https://issues.jboss.org/browse/PLINK-384 is released and updated
// loginPage.open();
// loginPage.login("john", "password");
// Assert.assertEquals("Invalid username or password.", loginPage.getError());

loginPage.open();
loginPage.login("john", "new-password");
Expand Down