Fix recreation of user in Che DB, when it has been recreated on Keycloak (#9280)

6.19.x
Mykhailo Kuznietsov 2018-04-03 12:10:20 +00:00 committed by GitHub
parent d6ade61e9b
commit ee54a220f4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 375 additions and 121 deletions

View File

@ -49,6 +49,10 @@
<groupId>com.google.inject</groupId>
<artifactId>guice</artifactId>
</dependency>
<dependency>
<groupId>com.google.inject.extensions</groupId>
<artifactId>guice-persist</artifactId>
</dependency>
<dependency>
<groupId>com.google.inject.extensions</groupId>
<artifactId>guice-servlet</artifactId>
@ -69,6 +73,10 @@
<groupId>javax.ws.rs</groupId>
<artifactId>javax.ws.rs-api</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-api-account</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-api-auth-shared</artifactId>
@ -113,6 +121,10 @@
<groupId>org.eclipse.che.multiuser</groupId>
<artifactId>che-multiuser-keycloak-shared</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.multiuser</groupId>
<artifactId>che-multiuser-personal-account</artifactId>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>

View File

@ -10,14 +10,10 @@
*/
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;
import java.io.IOException;
import java.security.Principal;
import java.util.Optional;
import javax.inject.Inject;
import javax.inject.Singleton;
import javax.servlet.FilterChain;
@ -28,11 +24,8 @@ import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletRequestWrapper;
import javax.servlet.http.HttpSession;
import org.eclipse.che.api.core.ConflictException;
import org.eclipse.che.api.core.NotFoundException;
import org.eclipse.che.api.core.ServerException;
import org.eclipse.che.api.core.model.user.User;
import org.eclipse.che.api.user.server.UserManager;
import org.eclipse.che.api.user.server.model.impl.UserImpl;
import org.eclipse.che.commons.auth.token.RequestTokenExtractor;
import org.eclipse.che.commons.env.EnvironmentContext;
import org.eclipse.che.commons.subject.Subject;
@ -48,13 +41,13 @@ import org.eclipse.che.multiuser.api.permission.server.PermissionChecker;
@Singleton
public class KeycloakEnvironmentInitalizationFilter extends AbstractKeycloakFilter {
private final UserManager userManager;
private final KeycloakUserManager userManager;
private final RequestTokenExtractor tokenExtractor;
private final PermissionChecker permissionChecker;
@Inject
public KeycloakEnvironmentInitalizationFilter(
UserManager userManager,
KeycloakUserManager userManager,
RequestTokenExtractor tokenExtractor,
PermissionChecker permissionChecker) {
this.userManager = userManager;
@ -84,7 +77,7 @@ public class KeycloakEnvironmentInitalizationFilter extends AbstractKeycloakFilt
try {
User user =
getOrCreateUser(
userManager.getOrCreateUser(
claims.getSubject(),
claims.get("email", String.class),
claims.get("preferred_username", String.class));
@ -106,51 +99,6 @@ public class KeycloakEnvironmentInitalizationFilter extends AbstractKeycloakFilt
}
}
private User getOrCreateUser(String id, String email, String username)
throws ServerException, ConflictException {
Optional<User> user = getUser(id);
if (!user.isPresent()) {
synchronized (this) {
user = getUser(id);
if (!user.isPresent()) {
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);
}
}
}
}
return actualizeUser(user.get(), email);
}
/** Performs check that emails in JWT and local DB are match, and synchronize them otherwise */
private User actualizeUser(User actualUser, String email) throws ServerException {
if (actualUser.getEmail().equals(email)) {
return actualUser;
}
UserImpl update = new UserImpl(actualUser);
update.setEmail(email);
try {
userManager.update(update);
} catch (NotFoundException e) {
throw new ServerException("Unable to actualize user email. User not found.", e);
} catch (ConflictException e) {
throw new ServerException(
"Unable to actualize user email. Another user with such email exists", e);
}
return update;
}
private Optional<User> getUser(String id) throws ServerException {
try {
return Optional.of(userManager.getById(id));
} catch (NotFoundException e) {
return Optional.empty();
}
}
private HttpServletRequest addUserInRequest(
final HttpServletRequest httpRequest, final Subject subject) {
return new HttpServletRequestWrapper(httpRequest) {

View File

@ -0,0 +1,164 @@
/*
* Copyright (c) 2012-2018 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.multiuser.keycloak.server;
import static java.lang.System.currentTimeMillis;
import static java.util.Collections.emptyList;
import static org.eclipse.che.commons.lang.NameGenerator.generate;
import com.google.common.collect.ImmutableMap;
import com.google.inject.Inject;
import com.google.inject.persist.Transactional;
import java.util.Optional;
import javax.inject.Named;
import javax.inject.Singleton;
import org.eclipse.che.account.api.AccountManager;
import org.eclipse.che.api.core.ApiException;
import org.eclipse.che.api.core.ConflictException;
import org.eclipse.che.api.core.NotFoundException;
import org.eclipse.che.api.core.ServerException;
import org.eclipse.che.api.core.model.user.User;
import org.eclipse.che.api.core.notification.EventService;
import org.eclipse.che.api.user.server.event.BeforeUserRemovedEvent;
import org.eclipse.che.api.user.server.event.PostUserPersistedEvent;
import org.eclipse.che.api.user.server.model.impl.UserImpl;
import org.eclipse.che.api.user.server.spi.PreferenceDao;
import org.eclipse.che.api.user.server.spi.ProfileDao;
import org.eclipse.che.api.user.server.spi.UserDao;
import org.eclipse.che.multiuser.api.account.personal.PersonalAccountUserManager;
/**
* Extension of User Manager, providing utility operations related to Keycloak User management, and
* overriding create/remove operations to be compatible with {@link
* org.eclipse.che.multiuser.keycloak.server.dao.KeycloakProfileDao}
*
* @author Mykhailo Kuznietsov
*/
@Singleton
public class KeycloakUserManager extends PersonalAccountUserManager {
@Inject
public KeycloakUserManager(
UserDao userDao,
ProfileDao profileDao,
PreferenceDao preferencesDao,
AccountManager accountManager,
EventService eventService,
@Named("che.auth.reserved_user_names") String[] reservedNames) {
super(userDao, profileDao, preferencesDao, reservedNames, accountManager, eventService);
}
@Transactional(rollbackOn = {RuntimeException.class, ApiException.class})
protected void doCreate(UserImpl user, boolean isTemporary)
throws ConflictException, ServerException {
userDao.create(user);
eventService.publish(new PostUserPersistedEvent(new UserImpl(user))).propagateException();
preferencesDao.setPreferences(
user.getId(),
ImmutableMap.of(
"temporary", Boolean.toString(isTemporary),
"codenvy:created", Long.toString(currentTimeMillis())));
}
@Transactional(rollbackOn = {RuntimeException.class, ApiException.class})
protected void doRemove(String id) throws ServerException {
UserImpl user;
try {
user = userDao.getById(id);
} catch (NotFoundException ignored) {
return;
}
preferencesDao.remove(id);
eventService.publish(new BeforeUserRemovedEvent(user)).propagateException();
userDao.remove(id);
}
/**
* Method is used to retrieve user object from Che DB for given user {@code id}, {@code email},
* and {@code username}. Various actualization operations may be performed:
*
* <p>- if user is found in Che DB by the given {@code id}, then it will check, if it's email,
* matches the {@code email} , and update it in DB if necessary.
*
* <p>- if user is not found in Che DB by the given {@code id} , then attempt to create one. But
* also, it will attempt to get one by {@code email}. If such is found, he will be removed. That
* way, there will be no conflict with existing user id or email upon recreation. In case of
* conflict with user name, it may be prepended randomized symbols
*
* @param id - user id from
* @param email - user email
* @param username - user name
* @return user object from Che Database, with all needed actualization operations performed on
* him
* @throws ServerException if this exception during user creation, removal, or retrieval
* @throws ConflictException if this exception occurs during user creation or removal
*/
public User getOrCreateUser(String id, String email, String username)
throws ServerException, ConflictException {
Optional<User> userById = getUserById(id);
if (!userById.isPresent()) {
synchronized (this) {
userById = getUserById(id);
if (!userById.isPresent()) {
Optional<User> userByEmail = getUserByEmail(email);
if (userByEmail.isPresent()) {
remove(userByEmail.get().getId());
}
final UserImpl cheUser = new UserImpl(id, email, username, generate("", 12), emptyList());
try {
return create(cheUser, false);
} catch (ConflictException ex) {
cheUser.setName(generate(cheUser.getName(), 4));
return create(cheUser, false);
}
}
}
}
return actualizeUserEmail(userById.get(), email);
}
/**
* Performs check that {@code email} matches with the one in local DB, and synchronize them
* otherwise
*/
private User actualizeUserEmail(User actualUser, String email) throws ServerException {
if (actualUser.getEmail().equals(email)) {
return actualUser;
}
UserImpl update = new UserImpl(actualUser);
update.setEmail(email);
try {
update(update);
} catch (NotFoundException e) {
throw new ServerException("Unable to actualize user email. User not found.", e);
} catch (ConflictException e) {
throw new ServerException(
"Unable to actualize user email. Another user with such email exists", e);
}
return update;
}
private Optional<User> getUserById(String id) throws ServerException {
try {
return Optional.of(getById(id));
} catch (NotFoundException e) {
return Optional.empty();
}
}
private Optional<User> getUserByEmail(String email) throws ServerException {
try {
return Optional.of(getByEmail(email));
} catch (NotFoundException e) {
return Optional.empty();
}
}
}

View File

@ -51,7 +51,7 @@ public class KeycloakProfileDao implements ProfileDao {
@Override
public void create(ProfileImpl profile) throws ServerException, ConflictException {
// this method intentionally left blank
throw new ServerException("Given operation doesn't supported on current configured storage.");
}
@Override

View File

@ -14,8 +14,10 @@ import com.google.inject.AbstractModule;
import org.eclipse.che.api.core.rest.HttpJsonRequestFactory;
import org.eclipse.che.api.user.server.TokenValidator;
import org.eclipse.che.api.user.server.spi.ProfileDao;
import org.eclipse.che.multiuser.api.account.personal.PersonalAccountUserManager;
import org.eclipse.che.multiuser.keycloak.server.KeycloakConfigurationService;
import org.eclipse.che.multiuser.keycloak.server.KeycloakTokenValidator;
import org.eclipse.che.multiuser.keycloak.server.KeycloakUserManager;
import org.eclipse.che.multiuser.keycloak.server.dao.KeycloakProfileDao;
import org.eclipse.che.multiuser.keycloak.server.oauth2.KeycloakOAuthAuthenticationService;
@ -30,5 +32,6 @@ public class KeycloakModule extends AbstractModule {
bind(KeycloakOAuthAuthenticationService.class);
bind(ProfileDao.class).to(KeycloakProfileDao.class);
bind(PersonalAccountUserManager.class).to(KeycloakUserManager.class);
}
}

View File

@ -11,7 +11,6 @@
package org.eclipse.che.multiuser.keycloak.server;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.spy;
@ -30,9 +29,6 @@ import javax.servlet.FilterChain;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import org.eclipse.che.api.core.NotFoundException;
import org.eclipse.che.api.core.model.user.User;
import org.eclipse.che.api.user.server.UserManager;
import org.eclipse.che.api.user.server.model.impl.UserImpl;
import org.eclipse.che.commons.auth.token.RequestTokenExtractor;
import org.eclipse.che.commons.env.EnvironmentContext;
@ -51,7 +47,7 @@ import org.testng.annotations.Test;
@Listeners(value = {MockitoTestNGListener.class})
public class KeycloakEnvironmentInitalizationFilterTest {
@Mock private UserManager userManager;
@Mock private KeycloakUserManager userManager;
@Mock private RequestTokenExtractor tokenExtractor;
@Mock private PermissionChecker permissionChecker;
@Mock private FilterChain chain;
@ -101,7 +97,7 @@ public class KeycloakEnvironmentInitalizationFilterTest {
when(tokenExtractor.getToken(any(HttpServletRequest.class))).thenReturn("token2");
when(request.getAttribute("token")).thenReturn(createJwt());
when(session.getAttribute(eq("che_subject"))).thenReturn(existingSubject);
when(userManager.getById(anyString())).thenReturn(user);
when(userManager.getOrCreateUser(anyString(), anyString(), anyString())).thenReturn(user);
EnvironmentContext context = spy(EnvironmentContext.getCurrent());
EnvironmentContext.setCurrent(context);
@ -119,60 +115,6 @@ public class KeycloakEnvironmentInitalizationFilterTest {
assertEquals(expectedSubject.getUserName(), captor.getAllValues().get(1).getUserName());
}
@Test
public void shouldCreateUserIfNoneExists() throws Exception {
UserImpl user = new UserImpl();
DefaultJwt<Claims> jwt = createJwt();
user.setEmail((String) jwt.getBody().get("email"));
user.setId(jwt.getBody().getSubject());
user.setName((String) jwt.getBody().get("preferred_username"));
ArgumentCaptor<UserImpl> captor = ArgumentCaptor.forClass(UserImpl.class);
// given
when(tokenExtractor.getToken(any(HttpServletRequest.class))).thenReturn("token2");
when(request.getScheme()).thenReturn("http");
when(request.getSession()).thenReturn(session);
when(request.getAttribute("token")).thenReturn(jwt);
when(session.getAttribute(eq("che_subject"))).thenReturn(null);
when(userManager.getById(anyString())).thenThrow(NotFoundException.class);
when(userManager.create(any(User.class), anyBoolean())).thenReturn(user);
// when
filter.doFilter(request, response, chain);
// then
verify(session).setAttribute(eq("che_subject"), captor.capture());
verify(userManager).create(captor.capture(), eq(false));
assertEquals(jwt.getBody().getSubject(), captor.getValue().getId());
assertEquals(jwt.getBody().get("email"), captor.getValue().getEmail());
assertEquals(jwt.getBody().get("preferred_username"), captor.getValue().getName());
}
@Test
public void shouldUpdateUserWhenEmailsNotMatch() throws Exception {
Subject existingSubject = new SubjectImpl("name", "id1", "token", false);
UserImpl user = new UserImpl("id2", "test2@test.com", "username2");
DefaultJwt<Claims> jwt = createJwt();
ArgumentCaptor<UserImpl> captor = ArgumentCaptor.forClass(UserImpl.class);
// given
when(tokenExtractor.getToken(any(HttpServletRequest.class))).thenReturn("token2");
when(request.getAttribute("token")).thenReturn(jwt);
when(session.getAttribute(eq("che_subject"))).thenReturn(existingSubject);
when(userManager.getById(anyString())).thenReturn(user);
// when
filter.doFilter(request, response, chain);
// then
verify(userManager).update(captor.capture());
assertEquals(jwt.getBody().get("email"), captor.getValue().getEmail());
}
private DefaultJwt<Claims> createJwt() {
Map<String, Object> claimParams = new HashMap<>();
claimParams.put("email", "test@test.com");

View File

@ -0,0 +1,185 @@
/*
* Copyright (c) 2012-2018 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.multiuser.keycloak.server;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.atLeastOnce;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.MockitoAnnotations.initMocks;
import static org.testng.Assert.assertNotEquals;
import static org.testng.AssertJUnit.assertEquals;
import org.eclipse.che.account.api.AccountManager;
import org.eclipse.che.api.core.ConflictException;
import org.eclipse.che.api.core.NotFoundException;
import org.eclipse.che.api.core.model.user.User;
import org.eclipse.che.api.core.notification.EventService;
import org.eclipse.che.api.user.server.event.BeforeUserRemovedEvent;
import org.eclipse.che.api.user.server.event.PostUserPersistedEvent;
import org.eclipse.che.api.user.server.model.impl.UserImpl;
import org.eclipse.che.api.user.server.spi.PreferenceDao;
import org.eclipse.che.api.user.server.spi.ProfileDao;
import org.eclipse.che.api.user.server.spi.UserDao;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Listeners;
import org.testng.annotations.Test;
/** @author Mykhailo Kuznietsov */
@Listeners(value = {MockitoTestNGListener.class})
public class KeycloakUserManagerTest {
@Mock private UserDao userDao;
@Mock private ProfileDao profileDao;
@Mock private PreferenceDao preferenceDao;
@Mock private AccountManager accountManager;
@Mock private EventService eventService;
@Mock private PostUserPersistedEvent postUserPersistedEvent;
@Mock private BeforeUserRemovedEvent beforeUserRemovedEvent;
KeycloakUserManager keycloakUserManager;
@BeforeMethod
public void setUp() {
initMocks(this);
keycloakUserManager =
new KeycloakUserManager(
userDao, profileDao, preferenceDao, accountManager, eventService, new String[] {});
when(eventService.publish(any()))
.thenAnswer(
invocationOnMock -> {
Object arg = invocationOnMock.getArguments()[0];
if (arg instanceof BeforeUserRemovedEvent) {
return beforeUserRemovedEvent;
} else {
return postUserPersistedEvent;
}
});
}
@Test
public void shouldReturnExistingUser() throws Exception {
UserImpl userImpl = new UserImpl("id", "user@mail.com", "name");
when(userDao.getById(eq("id"))).thenReturn(userImpl);
User user = keycloakUserManager.getOrCreateUser("id", "user@mail.com", "name");
verify(userDao).getById("id");
assertEquals("id", user.getId());
assertEquals("user@mail.com", user.getEmail());
assertEquals("name", user.getName());
}
@Test
public void shouldReturnUserAndUpdateHisEmail() throws Exception {
// given
ArgumentCaptor<UserImpl> captor = ArgumentCaptor.forClass(UserImpl.class);
UserImpl userImpl = new UserImpl("id", "user@mail.com", "name");
when(userDao.getById(eq("id"))).thenReturn(userImpl);
// when
User user = keycloakUserManager.getOrCreateUser("id", "new@mail.com", "name");
// then
verify(userDao, times(2)).getById("id");
verify(userDao).update(captor.capture());
assertEquals("id", captor.getValue().getId());
assertEquals("new@mail.com", captor.getValue().getEmail());
assertEquals("name", captor.getValue().getName());
assertEquals("id", user.getId());
assertEquals("new@mail.com", user.getEmail());
assertEquals("name", user.getName());
}
@Test
public void shoudCreateNewUserIfHeIsNotFoundByIdOrEmail() throws Exception {
// given
ArgumentCaptor<UserImpl> captor = ArgumentCaptor.forClass(UserImpl.class);
when(userDao.getById(eq("id"))).thenThrow(NotFoundException.class);
when(userDao.getByEmail(eq("user@mail.com"))).thenThrow(NotFoundException.class);
// when
keycloakUserManager.getOrCreateUser("id", "user@mail.com", "name");
// then
verify(userDao, times(2)).getById(eq("id"));
verify(userDao).getByEmail(eq("user@mail.com"));
verify(userDao).create((captor.capture()));
assertEquals("id", captor.getValue().getId());
assertEquals("user@mail.com", captor.getValue().getEmail());
assertEquals("name", captor.getValue().getName());
}
@Test
public void shoudRecreateUserIfHeIsntFoundByIdButFoundByEmail() throws Exception {
// given
UserImpl newUserImpl = new UserImpl("id", "user@mail.com", "name");
UserImpl oldUserImpl = new UserImpl("oldId", "user@mail.com", "name");
ArgumentCaptor<UserImpl> captor = ArgumentCaptor.forClass(UserImpl.class);
when(userDao.getById(eq("id"))).thenThrow(NotFoundException.class);
when(userDao.getByEmail(eq("user@mail.com"))).thenReturn(oldUserImpl);
// when
keycloakUserManager.getOrCreateUser("id", "user@mail.com", "name");
// then
verify(userDao, times(2)).getById(eq("id"));
verify(userDao).getByEmail(eq("user@mail.com"));
verify(userDao).remove(eq(oldUserImpl.getId()));
verify(userDao).create((captor.capture()));
assertEquals(newUserImpl.getId(), captor.getValue().getId());
assertEquals(newUserImpl.getEmail(), captor.getValue().getEmail());
assertEquals(newUserImpl.getName(), captor.getValue().getName());
}
@Test
public void shoudRecreateUserWithDifferentNameIfConflictOccures() throws Exception {
// given
UserImpl newUserImpl = new UserImpl("id", "user@mail.com", "name");
ArgumentCaptor<UserImpl> captor = ArgumentCaptor.forClass(UserImpl.class);
ArgumentCaptor<UserImpl> captor2 = ArgumentCaptor.forClass(UserImpl.class);
when(userDao.getById(eq("id"))).thenThrow(NotFoundException.class);
when(userDao.getByEmail(eq("user@mail.com"))).thenThrow(NotFoundException.class);
doAnswer(
invocation -> {
if (((UserImpl) invocation.getArgument(0)).getName().equals("name")) {
throw new ConflictException("");
}
return newUserImpl;
})
.when(userDao)
.create(any());
// when
keycloakUserManager.getOrCreateUser("id", "user@mail.com", "name");
// then
verify(userDao, times(2)).getById(eq("id"));
verify(userDao).getByEmail(eq("user@mail.com"));
verify(userDao, atLeastOnce()).create((captor.capture()));
assertEquals(newUserImpl.getId(), captor.getValue().getId());
assertEquals(newUserImpl.getEmail(), captor.getValue().getEmail());
assertNotEquals(newUserImpl.getName(), captor.getValue().getName());
}
}

View File

@ -55,11 +55,11 @@ public class UserManager {
public static final String PERSONAL_ACCOUNT = "personal";
private final UserDao userDao;
private final ProfileDao profileDao;
private final PreferenceDao preferencesDao;
private final Set<String> reservedNames;
private final EventService eventService;
protected final UserDao userDao;
protected final ProfileDao profileDao;
protected final PreferenceDao preferencesDao;
protected final Set<String> reservedNames;
protected final EventService eventService;
@Inject
public UserManager(