From 08e93a13cc6d65df05c20355ff3381ede0fcacae Mon Sep 17 00:00:00 2001 From: Sanjay Dalal Date: Thu, 29 Apr 2010 19:34:58 +0000 Subject: [PATCH] CSPACE-1520 unique constraint violation on userid now sends back 400 with a message instead of 500 extracted out cs idp persistence management into UserStorageClient test: account service M services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountStorageClient.java A services/account/service/src/main/java/org/collectionspace/services/account/storage/csidp A services/account/service/src/main/java/org/collectionspace/services/account/storage/csidp/UserStorageClient.java M services/account/client/src/test/java/org/collectionspace/services/account/client/test/AccountServiceTest.java --- .../client/test/AccountServiceTest.java | 4 +- .../account/storage/AccountStorageClient.java | 126 +++++----------- .../storage/csidp/UserStorageClient.java | 141 ++++++++++++++++++ 3 files changed, 181 insertions(+), 90 deletions(-) create mode 100644 services/account/service/src/main/java/org/collectionspace/services/account/storage/csidp/UserStorageClient.java diff --git a/services/account/client/src/test/java/org/collectionspace/services/account/client/test/AccountServiceTest.java b/services/account/client/src/test/java/org/collectionspace/services/account/client/test/AccountServiceTest.java index 072dcc3c4..25e7ad0ae 100644 --- a/services/account/client/src/test/java/org/collectionspace/services/account/client/test/AccountServiceTest.java +++ b/services/account/client/src/test/java/org/collectionspace/services/account/client/test/AccountServiceTest.java @@ -127,7 +127,7 @@ public class AccountServiceTest extends AbstractServiceTestImpl { } Assert.assertTrue(REQUEST_TYPE.isValidStatusCode(statusCode), invalidStatusCodeMessage(REQUEST_TYPE, statusCode)); - Assert.assertEquals(statusCode, Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()); + Assert.assertEquals(statusCode, Response.Status.BAD_REQUEST.getStatusCode()); } @Test(dataProvider = "testName", dataProviderClass = AbstractServiceTestImpl.class, @@ -241,7 +241,7 @@ public class AccountServiceTest extends AbstractServiceTestImpl { Assert.assertEquals(statusCode, Response.Status.BAD_REQUEST.getStatusCode()); } - @Test(dataProvider = "testName", dataProviderClass = AbstractServiceTestImpl.class, + @Test(dataProvider = "testName", dataProviderClass = AbstractServiceTestImpl.class, dependsOnMethods = {"create"}) public void createWithMostInvalid(String testName) throws Exception { diff --git a/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountStorageClient.java b/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountStorageClient.java index cd54e782b..f2714520b 100644 --- a/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountStorageClient.java +++ b/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountStorageClient.java @@ -28,6 +28,7 @@ import javax.persistence.EntityManager; import javax.persistence.EntityManagerFactory; import javax.persistence.Query; import org.collectionspace.services.account.AccountsCommon; +import org.collectionspace.services.account.storage.csidp.UserStorageClient; import org.collectionspace.services.authentication.User; import org.collectionspace.services.common.context.ServiceContext; import org.collectionspace.services.common.document.BadRequestException; @@ -55,6 +56,7 @@ import org.slf4j.LoggerFactory; public class AccountStorageClient extends JpaStorageClientImpl { private final Logger logger = LoggerFactory.getLogger(AccountStorageClient.class); + private UserStorageClient userStorageClient = new UserStorageClient(); public AccountStorageClient() { } @@ -74,9 +76,9 @@ public class AccountStorageClient extends JpaStorageClientImpl { } EntityManagerFactory emf = null; EntityManager em = null; + AccountsCommon account = (AccountsCommon) handler.getCommonPart(); try { handler.prepare(Action.CREATE); - AccountsCommon account = (AccountsCommon) handler.getCommonPart(); DocumentWrapper wrapDoc = new DocumentWrapperImpl(account); handler.handle(Action.CREATE, wrapDoc); @@ -84,8 +86,9 @@ public class AccountStorageClient extends JpaStorageClientImpl { em = emf.createEntityManager(); em.getTransaction().begin(); //if userid and password are given, add to default id provider - if (account.getUserId() != null && account.getPassword() != null) { - User user = createUser(account); + if (account.getUserId() != null && isForCSIdP(account.getPassword())) { + User user = userStorageClient.create(account.getUserId(), + account.getPassword()); em.persist(user); } // if (accountReceived.getTenant() != null) { @@ -103,11 +106,21 @@ public class AccountStorageClient extends JpaStorageClientImpl { } throw bre; } catch (Exception e) { + if (logger.isDebugEnabled()) { + logger.debug("Caught exception ", e); + } + boolean uniqueConstraint = false; + if (userStorageClient.get(em, account.getUserId()) != null) { + //might be unique constraint violation + uniqueConstraint = true; + } if (em != null && em.getTransaction().isActive()) { em.getTransaction().rollback(); } - if (logger.isDebugEnabled()) { - logger.debug("Caught exception ", e); + if (uniqueConstraint) { + String msg = "UserId exists. Non unique userId=" + account.getUserId(); + logger.error(msg); + throw new BadRequestException(msg); } throw new DocumentException(e); } finally { @@ -141,8 +154,10 @@ public class AccountStorageClient extends JpaStorageClientImpl { checkAllowedUpdates(accountReceived, accountFound); //if userid and password are given, add to default id provider if (accountReceived.getUserId() != null - && hasPassword(accountReceived.getPassword())) { - updateUser(em, accountReceived); + && isForCSIdP(accountReceived.getPassword())) { + userStorageClient.update(em, + accountReceived.getUserId(), + accountReceived.getPassword()); } DocumentWrapper wrapDoc = new DocumentWrapperImpl(accountFound); @@ -188,37 +203,12 @@ public class AccountStorageClient extends JpaStorageClientImpl { try { emf = JpaStorageUtils.getEntityManagerFactory(); em = emf.createEntityManager(); - //TODO investigate if deep delete is possible - //query an delete is inefficient - AccountsCommon accountFound = getAccount(em, id); - - //TODO: add tenant id - //if userid gives any indication about the id provider, it should - //be used to avoid the following approach - Query usrDel = null; - User userLocal = getUser(em, accountFound); - if (userLocal != null) { - StringBuilder usrDelStr = new StringBuilder("DELETE FROM "); - usrDelStr.append(User.class.getCanonicalName()); - usrDelStr.append(" WHERE username = :username"); - //TODO: add tenant id - usrDel = em.createQuery(usrDelStr.toString()); - usrDel.setParameter("username", accountFound.getUserId()); - } + AccountsCommon accountFound = getAccount(em, id); em.getTransaction().begin(); - - if (userLocal != null) { - int usrDelCount = usrDel.executeUpdate(); - if (usrDelCount != 1) { - if (em != null && em.getTransaction().isActive()) { - em.getTransaction().rollback(); - } - String msg = "could not find user with username=" + accountFound.getUserId(); - logger.error(msg); - throw new DocumentNotFoundException(msg); - } - } + //if userid gives any indication about the id provider, it should + //be used to avoid delete + userStorageClient.delete(em, accountFound.getUserId()); em.remove(accountFound); em.getTransaction().commit(); @@ -257,64 +247,24 @@ public class AccountStorageClient extends JpaStorageClientImpl { private boolean checkAllowedUpdates(AccountsCommon toAccount, AccountsCommon fromAccount) throws BadRequestException { if (!fromAccount.getUserId().equals(toAccount.getUserId())) { - String msg = "User id " + toAccount.getUserId() + " does not match " - + "for given account with csid=" + fromAccount.getCsid(); + String msg = "userId=" + toAccount.getUserId() + " of existing account does not match " + + "the userId=" + fromAccount.getUserId() + + " with csid=" + fromAccount.getCsid(); logger.error(msg); - logger.debug(msg + " found userid=" + fromAccount.getUserId()); - throw new BadRequestException(msg); - } - return true; - } - - private User createUser(AccountsCommon account) throws Exception { - User user = new User(); - user.setUsername(account.getUserId()); - if (hasPassword(account.getPassword())) { - user.setPasswd(getEncPassword(account)); - } - user.setCreatedAtItem(new Date()); - return user; - } - - private User getUser(EntityManager em, AccountsCommon account) throws DocumentNotFoundException { - User userFound = em.find(User.class, account.getUserId()); - if (userFound == null) { - if (em != null && em.getTransaction().isActive()) { - em.getTransaction().rollback(); - } - String msg = "could not find user with id=" + account.getUserId(); - logger.error(msg); - throw new DocumentNotFoundException(msg); - } - return userFound; - } - - private void updateUser(EntityManager em, AccountsCommon account) throws Exception { - User userFound = getUser(em, account); - if (userFound != null) { - userFound.setPasswd(getEncPassword(account)); - userFound.setUpdatedAtItem(new Date()); if (logger.isDebugEnabled()) { - logger.debug("updated user=" + JaxbUtils.toString(userFound, User.class)); + logger.debug(msg + " found userid=" + fromAccount.getUserId()); } - em.persist(userFound); - } - } - - private String getEncPassword(AccountsCommon account) throws BadRequestException { - //jaxb unmarshaller already unmarshal xs:base64Binary, no need to b64 decode - //byte[] bpass = Base64.decodeBase64(accountReceived.getPassword()); - try { - SecurityUtils.validatePassword(new String(account.getPassword())); - } catch (Exception e) { - throw new BadRequestException(e.getMessage()); + throw new BadRequestException(msg); } - String secEncPasswd = SecurityUtils.createPasswordHash( - account.getUserId(), new String(account.getPassword())); - return secEncPasswd; + return true; } - private boolean hasPassword(byte[] bpass) { + /** + * isForCSIdP deteremines if the create/update is also needed for CS IdP + * @param bpass + * @return + */ + private boolean isForCSIdP(byte[] bpass) { return bpass != null && bpass.length > 0; } // private UserTenant createTenantAssoc(AccountsCommon accountReceived) { diff --git a/services/account/service/src/main/java/org/collectionspace/services/account/storage/csidp/UserStorageClient.java b/services/account/service/src/main/java/org/collectionspace/services/account/storage/csidp/UserStorageClient.java new file mode 100644 index 000000000..2345ec102 --- /dev/null +++ b/services/account/service/src/main/java/org/collectionspace/services/account/storage/csidp/UserStorageClient.java @@ -0,0 +1,141 @@ +/** + * This document is a part of the source code and related artifacts + * for CollectionSpace, an open source collections management system + * for museums and related institutions: + + * http://www.collectionspace.org + * http://wiki.collectionspace.org + + * Copyright 2010 University of California at Berkeley + + * Licensed under the Educational Community License (ECL), Version 2.0. + * You may not use this file except in compliance with this License. + + * You may obtain a copy of the ECL 2.0 License at + + * https://source.collectionspace.org/collection-space/LICENSE.txt + + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +/* + * To change this template, choose Tools | Templates + * and open the template in the editor. + */ +package org.collectionspace.services.account.storage.csidp; + +import java.util.Date; +import javax.persistence.EntityManager; +import javax.persistence.Query; +import org.collectionspace.services.authentication.User; +import org.collectionspace.services.common.document.BadRequestException; +import org.collectionspace.services.common.document.DocumentNotFoundException; +import org.collectionspace.services.common.document.JaxbUtils; +import org.collectionspace.services.common.security.SecurityUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * UserStorageClient manages persistence for CS IdP + * Note: this class is always used by the AccountStorageClient which provides + * access to entity manager + * @author + */ +public class UserStorageClient { + + private final Logger logger = LoggerFactory.getLogger(UserStorageClient.class); + + /** + * create user with given userId and password + * @param userId + * @param password + * @return user + */ + public User create(String userId, byte[] password) throws Exception { + User user = new User(); + user.setUsername(userId); + user.setPasswd(getEncPassword(userId, password)); + user.setCreatedAtItem(new Date()); + return user; + } + + /** + * getUser get user for given userId + * @param em EntityManager + * @param userId + */ + public User get(EntityManager em, String userId) throws DocumentNotFoundException { + User userFound = em.find(User.class, userId); + if (userFound == null) { + if (em != null && em.getTransaction().isActive()) { + em.getTransaction().rollback(); + } + String msg = "could not find user with userId=" + userId; + logger.error(msg); + throw new DocumentNotFoundException(msg); + } + return userFound; + } + + /** + * updateUser for given userId + * @param entity manager + * @param userId + * @param password + */ + public void update(EntityManager em, String userId, byte[] password) + throws DocumentNotFoundException, Exception { + User userFound = get(em, userId); + if (userFound != null) { + userFound.setPasswd(getEncPassword(userId, password)); + userFound.setUpdatedAtItem(new Date()); + if (logger.isDebugEnabled()) { + logger.debug("updated user=" + JaxbUtils.toString(userFound, User.class)); + } + em.persist(userFound); + } + } + + /** + * delete deletes user with given userId + * @param em entity manager + * @param userId + * @throws Exception if user for given userId not found + */ + public void delete(EntityManager em, String userId) + throws DocumentNotFoundException, Exception { + //if userid gives any indication about the id provider, it should + //be used to avoid the following approach + StringBuilder usrDelStr = new StringBuilder("DELETE FROM "); + usrDelStr.append(User.class.getCanonicalName()); + usrDelStr.append(" WHERE username = :username"); + //TODO: add tenant id + Query usrDel = em.createQuery(usrDelStr.toString()); + usrDel.setParameter("username", userId); + int usrDelCount = usrDel.executeUpdate(); + if (usrDelCount != 1) { + if (em != null && em.getTransaction().isActive()) { + em.getTransaction().rollback(); + } + String msg = "could not find user with username=" + userId; + logger.error(msg); + throw new DocumentNotFoundException(msg); + } + } + + private String getEncPassword(String userId, byte[] password) throws BadRequestException { + //jaxb unmarshaller already unmarshal xs:base64Binary, no need to b64 decode + //byte[] bpass = Base64.decodeBase64(accountReceived.getPassword()); + try { + SecurityUtils.validatePassword(new String(password)); + } catch (Exception e) { + throw new BadRequestException(e.getMessage()); + } + String secEncPasswd = SecurityUtils.createPasswordHash( + userId, new String(password)); + return secEncPasswd; + } +} -- 2.47.3