From c7f5e492b0e50c6a1cf44a80e5424b3c97e5c03d Mon Sep 17 00:00:00 2001 From: Richard Millet Date: Fri, 26 Feb 2021 11:28:19 -0800 Subject: [PATCH] DRYD-947: Refactored connection and transaction handling for password-reset workflow to prevent intermitten user account corruption. --- .../services/account/AccountResource.java | 60 ++++++++++++------- .../storage/AccountDocumentHandler.java | 16 ++++- .../account/storage/AccountStorageClient.java | 6 +- .../services/common/SecurityResourceBase.java | 24 +++++++- .../context/RemoteServiceContextImpl.java | 7 ++- .../storage/jpa/JpaStorageClientImpl.java | 2 + 6 files changed, 82 insertions(+), 33 deletions(-) diff --git a/services/account/service/src/main/java/org/collectionspace/services/account/AccountResource.java b/services/account/service/src/main/java/org/collectionspace/services/account/AccountResource.java index 970212295..46a6c7f9a 100644 --- a/services/account/service/src/main/java/org/collectionspace/services/account/AccountResource.java +++ b/services/account/service/src/main/java/org/collectionspace/services/account/AccountResource.java @@ -206,11 +206,13 @@ public class AccountResource extends SecurityResourceBase(); for (RoleValue roleValue: roleValueList) { - String displayName = roleValue.getDisplayName(); - if (displayName == null) { - displayName = RoleClient.inferDisplayName(roleValue.getRoleName(), tenantId); + if (roleValue != null) { + String displayName = roleValue.getDisplayName(); + if (displayName == null) { + displayName = RoleClient.inferDisplayName(roleValue.getRoleName(), tenantId); + } + result.add(displayName); } - result.add(displayName); } } } @@ -224,15 +226,14 @@ public class AccountResource extends SecurityResourceBase parentContext, UriInfo ui, String csid, AccountsCommon theUpdate) { - return (AccountsCommon)update(parentContext, ui, csid, theUpdate, AccountsCommon.class); + private AccountsCommon updateAccountPassword(ServiceContext parentContext, UriInfo ui, String csid, AccountsCommon theUpdate) { + return (AccountsCommon)update(parentContext, ui, csid, theUpdate, AccountsCommon.class, false); } - /** * Resets an accounts password. * @@ -328,7 +329,7 @@ public class AccountResource extends SecurityResourceBase queryParams = ui.getQueryParameters(); String email = queryParams.getFirst(AccountClient.EMAIL_QUERY_PARAM); - if (email == null) { + if (email == null || email.isEmpty()) { response = Response.status(Response.Status.BAD_REQUEST).entity("You must specify an 'email' query paramater.").type("text/plain").build(); return response; } String tenantId = queryParams.getFirst(AuthN.TENANT_ID_QUERY_PARAM); - if (tenantId == null) { + if (tenantId == null || tenantId.isEmpty()) { response = Response.status(Response.Status.BAD_REQUEST).entity("You must specify an 'tid' (tenant ID) query paramater.").type("text/plain").build(); return response; } - + // + // Search for an account with the provided email and tenant ID + // + boolean found = false; + AccountListItem accountListItem = null; AccountsCommonList accountList = getAccountList(ui); - if (accountList == null || accountList.getTotalItems() == 0) { - response = Response.status(Response.Status.NOT_FOUND).entity("Could not locate an account associated with the email: " + - email).type("text/plain").build(); - } else if (accountList.getTotalItems() > 1) { - response = Response.status(Response.Status.BAD_REQUEST).entity("Located more than one account associated with the email: " + - email).type("text/plain").build(); - } else { - AccountListItem accountListItem = accountList.getAccountListItem().get(0); - try { + if (accountList != null || accountList.getTotalItems() > 0) { + List itemsList = accountList.getAccountListItem(); + for (AccountListItem item : itemsList) { + if (item != null && item.getTenantid() != null && item.getTenantid().equalsIgnoreCase(tenantId)) { + accountListItem = item; + found = true; + break; + } + } + } + + if (found == true) { + try { response = requestPasswordReset(ui, tenantId, accountListItem); } catch (Exception e) { - response = Response.status(Response.Status.INTERNAL_SERVER_ERROR).entity(e.getMessage()).type("text/plain").build(); + response = Response.status(Response.Status.INTERNAL_SERVER_ERROR).entity(e.getMessage()).type("text/plain").build(); } + } else { + String msg = String.format("Could not locate an account associated with the email '%s' and tenant ID '%s'", + email , tenantId); + response = Response.status(Response.Status.NOT_FOUND).entity(msg).type("text/plain").build(); } return response; diff --git a/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountDocumentHandler.java b/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountDocumentHandler.java index 9d1f870b8..2a17628ba 100644 --- a/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountDocumentHandler.java +++ b/services/account/service/src/main/java/org/collectionspace/services/account/storage/AccountDocumentHandler.java @@ -232,7 +232,21 @@ public class AccountDocumentHandler AccountListItem accListItem = new AccountListItem(); accListItem.setScreenName(account.getScreenName()); accListItem.setUserid(account.getUserId()); - accListItem.setTenantid(account.getTenants().get(0).getTenantId()); // pick the default/first tenant + // + // Since accounts can be associated with more than 1 tenant, we only want to include + // the tenant information for the current service context. + // + String tenantInCtx = this.getServiceContext().getTenantId(); + List associatedTenantList = account.getTenants(); + for (AccountTenant associatedTenant : associatedTenantList) { + if (associatedTenant != null && associatedTenant.getTenantId() != null) { + if (associatedTenant.getTenantId().equalsIgnoreCase(tenantInCtx)) { + accListItem.setTenantid(associatedTenant.getTenantId()); + break; + } + } + } + accListItem.setTenants(account.getTenants()); accListItem.setEmail(account.getEmail()); accListItem.setStatus(account.getStatus()); 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 1a0df03bf..b42977406 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 @@ -184,14 +184,10 @@ public class AccountStorageClient extends JpaStorageClientImpl { accountReceived.getPassword()); } DocumentWrapper wrapDoc = - new DocumentWrapperImpl(accountFound); + new DocumentWrapperImpl(accountFound); handler.handle(Action.UPDATE, wrapDoc); handler.complete(Action.UPDATE, wrapDoc); jpaConnectionContext.commitTransaction(); - // - // Don't sanitize until we've committed changes to the DB - // - handler.sanitize(wrapDoc); } catch (BadRequestException bre) { jpaConnectionContext.markForRollback(); throw bre; diff --git a/services/common/src/main/java/org/collectionspace/services/common/SecurityResourceBase.java b/services/common/src/main/java/org/collectionspace/services/common/SecurityResourceBase.java index 1d59dd2d6..81b2a7b4d 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/SecurityResourceBase.java +++ b/services/common/src/main/java/org/collectionspace/services/common/SecurityResourceBase.java @@ -4,6 +4,8 @@ import org.collectionspace.services.common.context.ServiceContext; import org.collectionspace.services.common.document.DocumentException; import org.collectionspace.services.common.document.DocumentFilter; import org.collectionspace.services.common.document.DocumentHandler; +import org.collectionspace.services.common.document.DocumentWrapper; +import org.collectionspace.services.common.document.DocumentWrapperImpl; import org.collectionspace.services.common.storage.TransactionContext; import org.collectionspace.services.common.storage.jpa.JPATransactionContext; import org.slf4j.Logger; @@ -141,6 +143,12 @@ public abstract class SecurityResourceBase extends AbstractCollectionSpa throw bigReThrow(e, ServiceMessages.LIST_FAILED); } } + + protected OT sanitize(DocumentHandler handler, OT outputObject) { + DocumentWrapper wrapDoc = new DocumentWrapperImpl(outputObject); + handler.sanitize(wrapDoc); + return outputObject; + } public Object update(String csid, IT theUpdate, Class objectClass) { return update((UriInfo)null, csid, theUpdate, objectClass); @@ -155,13 +163,19 @@ public abstract class SecurityResourceBase extends AbstractCollectionSpa ServiceContext ctx = createServiceContext(theUpdate, objectClass, ui); DocumentHandler handler = createDocumentHandler(ctx); getStorageClient(ctx).update(ctx, csid, handler); - return ctx.getOutput(); + return sanitize(handler, ctx.getOutput()); } catch (Exception e) { throw bigReThrow(e, ServiceMessages.PUT_FAILED, csid); } } public Object update(ServiceContext parentCtx, UriInfo ui, String csid, IT theUpdate, Class objectClass) { + return update(parentCtx, ui, csid, theUpdate, objectClass, true); + } + + public Object update(ServiceContext parentCtx, UriInfo ui, String csid, IT theUpdate, Class objectClass, boolean sanitize) { + Object result = null; + if (logger.isDebugEnabled()) { logger.debug("updateRole with csid=" + csid); } @@ -171,10 +185,16 @@ public abstract class SecurityResourceBase extends AbstractCollectionSpa ServiceContext ctx = createServiceContext(parentCtx, theUpdate, objectClass, ui); DocumentHandler handler = createDocumentHandler(ctx); getStorageClient(ctx).update(ctx, csid, handler); - return ctx.getOutput(); + if (sanitize == true) { + result = sanitize(handler, ctx.getOutput()); + } else { + result = ctx.getOutput(); + } } catch (Exception e) { throw bigReThrow(e, ServiceMessages.PUT_FAILED, csid); } + + return result; } protected ServiceContext createServiceContext( diff --git a/services/common/src/main/java/org/collectionspace/services/common/context/RemoteServiceContextImpl.java b/services/common/src/main/java/org/collectionspace/services/common/context/RemoteServiceContextImpl.java index 17ba6aea3..fd0cd6dad 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/context/RemoteServiceContextImpl.java +++ b/services/common/src/main/java/org/collectionspace/services/common/context/RemoteServiceContextImpl.java @@ -290,8 +290,11 @@ public class RemoteServiceContextImpl TransactionContext transactionCtx = getCurrentTransactionContext(); if (transactionCtx != null) { if (--transactionConnectionRefCount == 0) { - transactionCtx.close(); - this.setProperty(StorageClient.SC_TRANSACTION_CONTEXT_KEY, null); + try { + transactionCtx.close(); + } finally { + this.setProperty(StorageClient.SC_TRANSACTION_CONTEXT_KEY, null); + } } } else { throw new TransactionException("Attempted to release a non-existent storage connection. Transaction context missing from service context."); diff --git a/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageClientImpl.java b/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageClientImpl.java index 86665d0ac..b963929a3 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageClientImpl.java +++ b/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageClientImpl.java @@ -256,11 +256,13 @@ public class JpaStorageClientImpl implements StorageClient { handler.complete(Action.GET_ALL, wrapDoc); jpaConnectionContext.commitTransaction(); } catch (DocumentException de) { + jpaConnectionContext.markForRollback(); throw de; } catch (Exception e) { if (logger.isDebugEnabled()) { logger.debug("Caught exception ", e); } + jpaConnectionContext.markForRollback(); throw new DocumentException(e); } finally { ctx.closeConnection(); -- 2.47.3