From 1bea871d0af5acf31eccdb6ba6fa2878850bbe25 Mon Sep 17 00:00:00 2001 From: Sergii Kabashniuk Date: Wed, 11 Oct 2017 16:21:59 +0300 Subject: [PATCH] Fixed possible constraint violation with an existed organization. (#6674) * Fixed possible constraint violation with an existed organization. Usecase is following: If we have already an organization with name, let's say "org" and the new user also has name "org" then we will have constraint violation. In this case, we will try to create the user with name "org"+random string. --- .../che-multiuser-keycloak-server/pom.xml | 4 ++ ...eycloakEnvironmentInitalizationFilter.java | 44 +++++++++++-------- .../che/account/spi/jpa/JpaAccountDao.java | 15 ++++--- .../che/account/spi/tck/AccountDaoTest.java | 8 ++++ 4 files changed, 47 insertions(+), 24 deletions(-) diff --git a/multiuser/keycloak/che-multiuser-keycloak-server/pom.xml b/multiuser/keycloak/che-multiuser-keycloak-server/pom.xml index daeddacec1..29310c20e3 100644 --- a/multiuser/keycloak/che-multiuser-keycloak-server/pom.xml +++ b/multiuser/keycloak/che-multiuser-keycloak-server/pom.xml @@ -74,6 +74,10 @@ org.eclipse.che.core che-core-commons-auth + + org.eclipse.che.core + che-core-commons-lang + org.eclipse.che.multiuser che-multiuser-api-authorization diff --git a/multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitalizationFilter.java b/multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitalizationFilter.java index 2f548b2986..81f5fbfe1c 100644 --- a/multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitalizationFilter.java +++ b/multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitalizationFilter.java @@ -11,6 +11,7 @@ package org.eclipse.che.multiuser.keycloak.server; import static java.util.Collections.emptyList; +import static org.eclipse.che.commons.lang.NameGenerator.generate; import io.jsonwebtoken.Claims; import io.jsonwebtoken.Jwt; @@ -79,15 +80,21 @@ public class KeycloakEnvironmentInitalizationFilter extends AbstractKeycloakFilt throw new ServletException("Cannot detect or instantiate user."); } Claims claims = (Claims) jwtToken.getBody(); - User user = - getOrCreateUser( - claims.getSubject(), - claims.get("email", String.class), - claims.get("preferred_username", String.class)); - subject = - new AuthorizedSubject( - new SubjectImpl(user.getName(), user.getId(), token, false), permissionChecker); - session.setAttribute("che_subject", subject); + + try { + User user = + getOrCreateUser( + claims.getSubject(), + claims.get("email", String.class), + claims.get("preferred_username", String.class)); + subject = + new AuthorizedSubject( + new SubjectImpl(user.getName(), user.getId(), token, false), permissionChecker); + session.setAttribute("che_subject", subject); + } catch (ServerException | ConflictException e) { + throw new ServletException( + "Unable to identify user " + claims.getSubject() + " in Che database", e); + } } try { @@ -98,19 +105,20 @@ public class KeycloakEnvironmentInitalizationFilter extends AbstractKeycloakFilt } } - private synchronized User getOrCreateUser(String id, String email, String username) - throws ServletException { + private User getOrCreateUser(String id, String email, String username) + throws ServerException, ConflictException { try { return userManager.getById(id); } catch (NotFoundException e) { - try { - final UserImpl cheUser = new UserImpl(id, email, username, "secret", emptyList()); - return userManager.create(cheUser, false); - } catch (ServerException | ConflictException ex) { - throw new ServletException("Unable to create new user", ex); + synchronized (this) { + final UserImpl cheUser = new UserImpl(id, email, username, generate("", 12), emptyList()); + try { + return userManager.create(cheUser, false); + } catch (ConflictException ex) { + cheUser.setName(generate(cheUser.getName(), 4)); + return userManager.create(cheUser, false); + } } - } catch (ServerException e) { - throw new ServletException("Unable to get user", e); } } diff --git a/wsmaster/che-core-api-account/src/main/java/org/eclipse/che/account/spi/jpa/JpaAccountDao.java b/wsmaster/che-core-api-account/src/main/java/org/eclipse/che/account/spi/jpa/JpaAccountDao.java index 3e68d63f53..6817b29f0c 100644 --- a/wsmaster/che-core-api-account/src/main/java/org/eclipse/che/account/spi/jpa/JpaAccountDao.java +++ b/wsmaster/che-core-api-account/src/main/java/org/eclipse/che/account/spi/jpa/JpaAccountDao.java @@ -14,7 +14,6 @@ import static java.lang.String.format; import static java.util.Objects.requireNonNull; import com.google.inject.persist.Transactional; -import java.util.Optional; import javax.inject.Inject; import javax.inject.Provider; import javax.inject.Singleton; @@ -111,7 +110,9 @@ public class JpaAccountDao implements AccountDao { @Transactional protected void doCreate(AccountImpl account) { - managerProvider.get().persist(account); + final EntityManager manager = managerProvider.get(); + manager.persist(account); + manager.flush(); } @Transactional @@ -127,10 +128,12 @@ public class JpaAccountDao implements AccountDao { } @Transactional - protected Optional doRemove(String id) { + protected void doRemove(String id) { final EntityManager manager = managerProvider.get(); - final Optional account = Optional.ofNullable(manager.find(AccountImpl.class, id)); - account.ifPresent(manager::remove); - return account; + AccountImpl account = manager.find(AccountImpl.class, id); + if (account != null) { + manager.remove(account); + manager.flush(); + } } } diff --git a/wsmaster/che-core-api-account/src/test/java/org/eclipse/che/account/spi/tck/AccountDaoTest.java b/wsmaster/che-core-api-account/src/test/java/org/eclipse/che/account/spi/tck/AccountDaoTest.java index ff7eb603c1..48a2e8ff3b 100644 --- a/wsmaster/che-core-api-account/src/test/java/org/eclipse/che/account/spi/tck/AccountDaoTest.java +++ b/wsmaster/che-core-api-account/src/test/java/org/eclipse/che/account/spi/tck/AccountDaoTest.java @@ -96,6 +96,14 @@ public class AccountDaoTest { accountDao.update(account); } + @Test(expectedExceptions = ConflictException.class) + public void shouldThrowConflictExceptionWhenCreatingAccountWithExistingName() throws Exception { + AccountImpl account = + new AccountImpl(NameGenerator.generate("account", 5), accounts[0].getName(), "test"); + + accountDao.create(account); + } + @Test(expectedExceptions = NotFoundException.class) public void shouldThrowNotFoundExceptionWhenUpdatingNonExistingAccount() throws Exception { AccountImpl account = accounts[0];