From 64ed4e057c77389788e26b2fafc77c7e4ce1df3c Mon Sep 17 00:00:00 2001 From: amontenegro Date: Wed, 18 Dec 2024 20:29:53 -0600 Subject: [PATCH 1/5] Improvements in error handling --- .../common/exception/JSONInputValidator.java | 33 +++++++++++++++---- .../api/common/jaxb/OrcidExceptionMapper.java | 14 +++++++- .../jaxb/OrcidJacksonJaxbJsonProvider.java | 5 +++ .../impl/RecordManagerReadOnlyImpl.java | 2 +- .../OrcidOauth2TokenDetailServiceImpl.java | 4 +++ 5 files changed, 50 insertions(+), 8 deletions(-) diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java b/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java index deae4c7b415..f47ba410bac 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java @@ -12,6 +12,7 @@ import javax.xml.validation.SchemaFactory; import javax.xml.validation.Validator; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.orcid.core.exception.ApplicationException; import org.orcid.core.exception.InvalidJSONException; import org.slf4j.Logger; @@ -31,7 +32,6 @@ public class JSONInputValidator { static { SCHEMA_LOCATIONS = new HashMap<>(); - SCHEMA_LOCATIONS.put(org.orcid.jaxb.model.v3.release.record.Work.class, "/record_3.0/work-3.0.xsd"); SCHEMA_LOCATIONS.put(org.orcid.jaxb.model.v3.release.record.Funding.class, "/record_3.0/funding-3.0.xsd"); @@ -128,15 +128,36 @@ public void validateJSONInput(Object obj) { try { source = new JAXBSource(CONTEXTS.get(clazz), obj); - VALIDATORS.get(clazz).validate(source); + Validator validator = VALIDATORS.get(clazz); + if(validator != null) { + validator.validate(source); + } else { + LOGGER.error("Unable to find validator for class " + clazz.getName()); + Map params = new HashMap<>(); + params.put("error", "Unable to find validator for class " + clazz.getName()); + throw new InvalidJSONException(params); + } } catch (SAXException e) { Map params = new HashMap<>(); - params.put("error", e.getCause().getCause().getMessage()); + Throwable rootCause = ExceptionUtils.getRootCause(e); + if(rootCause != null) { + params.put("error", rootCause.getMessage()); + } else { + // For SAXException, the message is usually 2 levels deep + params.put("error", e.getCause().getCause().getMessage()); + } throw new InvalidJSONException(params); } catch (Exception e) { - LOGGER.error("General exception from json validator", e); - throw new ApplicationException(e); - } + LOGGER.error("Unable to find validator for class " + clazz.getName()); + Map params = new HashMap<>(); + Throwable rootCause = ExceptionUtils.getRootCause(e); + if(rootCause != null) { + params.put("error", rootCause.getMessage()); + } else { + params.put("error", e.getMessage()); + } + throw new InvalidJSONException(params); + } } public void validate2_1APIJSONInput(Object obj) { diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java index 0c0621161b0..f4477a20730 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java @@ -16,6 +16,7 @@ import javax.ws.rs.ext.Provider; import org.apache.commons.lang.StringUtils; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException; import org.jbibtex.TokenMgrError; import org.orcid.api.common.filter.ApiVersionFilter; @@ -132,8 +133,19 @@ public Response toResponse(Throwable t) { logShortError(t, clientId); } else if (t instanceof MismatchedPutCodeException) { logShortError(t, clientId); + } else if (t instanceof WrongSourceException) { + logShortError(t, clientId); + } else if (t instanceof InvalidDisambiguatedOrgException) { + logShortError(t, clientId); + } else if (t instanceof OrcidWebhookNotFoundException) { + logShortError(t, clientId); } else { - LOGGER.error("An exception has occured processing request from client " + clientId, t); + Throwable rootCause = ExceptionUtils.getRootCause(t); + if(rootCause != null) { + LOGGER.error("An exception has occurred processing request from client " + clientId, rootCause); + } else { + LOGGER.error("An exception has occurred processing request from client " + clientId, t); + } } if (isOAuthTokenRequest()) { diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java index 9287e7b3bb8..64aec889103 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java @@ -70,6 +70,11 @@ public Object readFrom(Class arg0, Type arg1, Annotation[] arg2, MediaTy params.put("error", e.getMessage()); throw new InvalidJSONException(params); } + if(o == null) { + Map params = new HashMap<>(); + params.put("error", "Couldn't read object, data seems to be empty or invalid"); + throw new InvalidJSONException(params); + } if (jsonInputValidator.canValidate(o.getClass())) { RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes(); String apiVersion = (String) requestAttributes.getAttribute(ApiVersionFilter.API_VERSION_REQUEST_ATTRIBUTE_NAME, RequestAttributes.SCOPE_REQUEST); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/RecordManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/RecordManagerReadOnlyImpl.java index d797f3f5fb0..cad178049c5 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/RecordManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/RecordManagerReadOnlyImpl.java @@ -117,7 +117,7 @@ private Preferences getPreferences(String orcid) { } return preferences; } catch(Exception e) { - LOGGER.error("Exception loading preferences for " + orcid, e); + LOGGER.info("Locale for user " + orcid + " is not valid for V2.0/V2.1 API"); } return null; } diff --git a/orcid-core/src/main/java/org/orcid/core/oauth/service/OrcidOauth2TokenDetailServiceImpl.java b/orcid-core/src/main/java/org/orcid/core/oauth/service/OrcidOauth2TokenDetailServiceImpl.java index ab98087e127..5b1cbc6c4cb 100644 --- a/orcid-core/src/main/java/org/orcid/core/oauth/service/OrcidOauth2TokenDetailServiceImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/oauth/service/OrcidOauth2TokenDetailServiceImpl.java @@ -8,6 +8,7 @@ import javax.annotation.Resource; import javax.persistence.NoResultException; +import org.apache.commons.lang3.StringUtils; import org.orcid.core.constants.RevokeReason; import org.orcid.core.oauth.OrcidOauth2TokenDetailService; import org.orcid.core.utils.cache.redis.RedisClient; @@ -50,6 +51,9 @@ public void setOrcidOauth2TokenDetailDao(OrcidOauth2TokenDetailDao orcidOauth2To @Override public OrcidOauth2TokenDetail findNonDisabledByTokenValue(String token) { + if(StringUtils.isBlank(token)) { + return null; + } try { return orcidOauth2TokenDetailDaoReadOnly.findNonDisabledByTokenValue(token); } catch (NoResultException e) { From b54ce9d27ce0191f490d02ef6d5492537528fdae Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 23 Dec 2024 07:44:31 -0600 Subject: [PATCH 2/5] Some checks should not generate logs --- .../api/common/jaxb/OrcidExceptionMapper.java | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java index f4477a20730..6aecd39df16 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java @@ -89,22 +89,19 @@ public class OrcidExceptionMapper implements ExceptionMapper { public Response toResponse(Throwable t) { // Whatever exception has been caught, make sure we log it. String clientId = securityManager.getClientIdFromAPIRequest(); - if (t instanceof NotFoundException) { - logShortError(t, clientId); - } else if (t instanceof NoResultException) { - logShortError(t, clientId); - } else if (t instanceof OrcidDeprecatedException) { - logShortError(t, clientId); - } else if (t instanceof LockedException) { - logShortError(t, clientId); - } else if (t instanceof DeactivatedException) { - logShortError(t, clientId); + if(t instanceof OrcidDeprecatedException + || t instanceof LockedException + || t instanceof DeactivatedException + || t instanceof OrcidNoBioException + || t instanceof OrcidNonPublicElementException + || t instanceof OrcidNoResultException + || t instanceof NoResultException + || t instanceof NotFoundException) { + // Do not log any of these exceptions } else if (t instanceof DisabledException) { logShortError(t, clientId); } else if (t instanceof ClientDeactivatedException) { logShortError(t, clientId); - } else if (t instanceof OrcidNonPublicElementException) { - logShortError(t, clientId); } else if (t instanceof OrcidBadRequestException) { logShortError(t, clientId); } else if (t instanceof RedirectMismatchException) { @@ -117,16 +114,12 @@ public Response toResponse(Throwable t) { logShortError(t, clientId); } else if (t instanceof OrcidNotificationException) { logShortError(t, clientId); - } else if (t instanceof OrcidNoBioException) { - logShortError(t, clientId); } else if (t instanceof RemoteSolrException) { logShortError(t, clientId); } else if (t instanceof WebApplicationException) { logShortError(t, clientId); } else if (t instanceof TokenMgrError) { logShortError(t, clientId); - } else if (t instanceof OrcidNoResultException) { - logShortError(t, clientId); } else if (t instanceof OrcidUnauthorizedException) { logShortError(t, clientId); } else if (t instanceof InvalidPutCodeException) { From 57693ad9321aa72fbb4153167af7d533314002e1 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 23 Dec 2024 08:56:41 -0600 Subject: [PATCH 3/5] Remove several meaningless logs and remove the stack trace of a few --- .../common/exception/JSONInputValidator.java | 24 ++++++------ .../api/common/jaxb/OrcidExceptionMapper.java | 38 ++++++++++--------- .../jaxb/OrcidJacksonJaxbJsonProvider.java | 8 +--- .../OrcidJacksonJaxbJsonProviderPretty.java | 6 +-- .../core/exception/InvalidJSONException.java | 12 +++--- .../ClientDetailsManagerReadOnlyImpl.java | 2 +- 6 files changed, 44 insertions(+), 46 deletions(-) diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java b/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java index f47ba410bac..558f0b4818c 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/exception/JSONInputValidator.java @@ -132,31 +132,26 @@ public void validateJSONInput(Object obj) { if(validator != null) { validator.validate(source); } else { - LOGGER.error("Unable to find validator for class " + clazz.getName()); - Map params = new HashMap<>(); - params.put("error", "Unable to find validator for class " + clazz.getName()); - throw new InvalidJSONException(params); + throw new InvalidJSONException("Unable to find validator for class " + clazz.getName()); } } catch (SAXException e) { Map params = new HashMap<>(); Throwable rootCause = ExceptionUtils.getRootCause(e); if(rootCause != null) { - params.put("error", rootCause.getMessage()); + throw new InvalidJSONException(rootCause.getMessage(), e); } else { // For SAXException, the message is usually 2 levels deep - params.put("error", e.getCause().getCause().getMessage()); + throw new InvalidJSONException(e.getCause().getCause().getMessage(), e); } - throw new InvalidJSONException(params); } catch (Exception e) { LOGGER.error("Unable to find validator for class " + clazz.getName()); Map params = new HashMap<>(); Throwable rootCause = ExceptionUtils.getRootCause(e); if(rootCause != null) { - params.put("error", rootCause.getMessage()); + throw new InvalidJSONException(rootCause.getMessage(), e); } else { - params.put("error", e.getMessage()); + throw new InvalidJSONException(e.getMessage(), e); } - throw new InvalidJSONException(params); } } @@ -174,8 +169,13 @@ public void validate2_1APIJSONInput(Object obj) { VALIDATORS_2_1_API.get(clazz).validate(source); } catch (SAXException e) { Map params = new HashMap<>(); - params.put("error", e.getCause().getCause().getMessage()); - throw new InvalidJSONException(params); + Throwable rootCause = ExceptionUtils.getRootCause(e); + if(rootCause != null) { + throw new InvalidJSONException(rootCause.getMessage(), e); + } else { + // For SAXException, the message is usually 2 levels deep + throw new InvalidJSONException(e.getCause().getCause().getMessage(), e); + } } catch (Exception e) { throw new ApplicationException(e); } diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java index 6aecd39df16..d3dea7bdd23 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidExceptionMapper.java @@ -5,10 +5,7 @@ import javax.annotation.Resource; import javax.persistence.NoResultException; import javax.servlet.http.HttpServletRequest; -import javax.ws.rs.Consumes; -import javax.ws.rs.NotFoundException; -import javax.ws.rs.Produces; -import javax.ws.rs.WebApplicationException; +import javax.ws.rs.*; import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -19,6 +16,7 @@ import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException; import org.jbibtex.TokenMgrError; +import org.orcid.api.common.exception.JSONInputValidator; import org.orcid.api.common.filter.ApiVersionFilter; import org.orcid.api.common.util.ApiUtils; import org.orcid.core.api.OrcidApiConstants; @@ -43,6 +41,7 @@ import org.springframework.context.MessageSource; import org.springframework.security.authentication.DisabledException; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.oauth2.common.exceptions.InvalidClientException; import org.springframework.security.oauth2.common.exceptions.OAuth2Exception; import org.springframework.security.oauth2.common.exceptions.RedirectMismatchException; import org.springframework.stereotype.Component; @@ -96,10 +95,12 @@ public Response toResponse(Throwable t) { || t instanceof OrcidNonPublicElementException || t instanceof OrcidNoResultException || t instanceof NoResultException - || t instanceof NotFoundException) { + || t instanceof NotFoundException + || t instanceof OrcidUnauthorizedException + || t instanceof DisabledException + || t instanceof OrcidDuplicatedActivityException + || t instanceof OrcidDuplicatedElementException) { // Do not log any of these exceptions - } else if (t instanceof DisabledException) { - logShortError(t, clientId); } else if (t instanceof ClientDeactivatedException) { logShortError(t, clientId); } else if (t instanceof OrcidBadRequestException) { @@ -108,10 +109,6 @@ public Response toResponse(Throwable t) { logShortError(t, clientId); } else if (t instanceof DuplicatedGroupIdRecordException) { logShortError(t, clientId); - } else if (t instanceof OrcidDuplicatedActivityException) { - logShortError(t, clientId); - } else if (t instanceof OrcidDuplicatedElementException) { - logShortError(t, clientId); } else if (t instanceof OrcidNotificationException) { logShortError(t, clientId); } else if (t instanceof RemoteSolrException) { @@ -120,18 +117,24 @@ public Response toResponse(Throwable t) { logShortError(t, clientId); } else if (t instanceof TokenMgrError) { logShortError(t, clientId); - } else if (t instanceof OrcidUnauthorizedException) { - logShortError(t, clientId); - } else if (t instanceof InvalidPutCodeException) { + } else if (t instanceof InvalidPutCodeException) { logShortError(t, clientId); } else if (t instanceof MismatchedPutCodeException) { logShortError(t, clientId); } else if (t instanceof WrongSourceException) { logShortError(t, clientId); - } else if (t instanceof InvalidDisambiguatedOrgException) { - logShortError(t, clientId); } else if (t instanceof OrcidWebhookNotFoundException) { logShortError(t, clientId); + } else if(t instanceof OrcidAccessControlException) { + logShortError(t, clientId); + } else if (t instanceof InvalidJSONException) { + logShortError(t, clientId); + } else if(t instanceof StartDateAfterEndDateException) { + logShortError(t, clientId); + } else if(t instanceof InvalidClientException) { + logShortError(t, clientId); + } else if (t instanceof InvalidDisambiguatedOrgException) { + LOGGER.error("Error for client ID: " + clientId + "InvalidDisambiguatedOrgException: Disambiguated org is empty or null"); } else { Throwable rootCause = ExceptionUtils.getRootCause(t); if(rootCause != null) { @@ -171,8 +174,7 @@ public Response toResponse(Throwable t) { } private void logShortError(Throwable t, String clientId) { - StringBuffer temp = new StringBuffer(t.getClass().getSimpleName() + " exception from client: ").append(clientId).append(". ").append(t.getMessage()); - LOGGER.error(temp.toString()); + LOGGER.error(t.getClass().getSimpleName() + " exception from client: " + clientId + ((t.getMessage() == null) ? "." : "." + t.getMessage())); } private Response oAuthErrorResponse(Throwable t) { diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java index 64aec889103..ec3891de2b9 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProvider.java @@ -66,14 +66,10 @@ public Object readFrom(Class arg0, Type arg1, Annotation[] arg2, MediaTy try { o = super.readFrom(arg0, arg1, arg2, arg3, arg4, arg5); } catch (JsonMappingException e) { - Map params = new HashMap<>(); - params.put("error", e.getMessage()); - throw new InvalidJSONException(params); + throw new InvalidJSONException(e.getMessage(), e); } if(o == null) { - Map params = new HashMap<>(); - params.put("error", "Couldn't read object, data seems to be empty or invalid"); - throw new InvalidJSONException(params); + throw new InvalidJSONException("Couldn't read object, data seems to be empty or invalid"); } if (jsonInputValidator.canValidate(o.getClass())) { RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes(); diff --git a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProviderPretty.java b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProviderPretty.java index 7707f1f14b8..5489002c56e 100644 --- a/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProviderPretty.java +++ b/orcid-api-common/src/main/java/org/orcid/api/common/jaxb/OrcidJacksonJaxbJsonProviderPretty.java @@ -70,10 +70,8 @@ public Object readFrom(Class arg0, Type arg1, Annotation[] arg2, MediaTy Object o = null; try{ o = super.readFrom(arg0, arg1, arg2, arg3, arg4, arg5); - }catch(JsonMappingException e){ - Map params = new HashMap<>(); - params.put("error", e.getMessage()); - throw new InvalidJSONException(params); + } catch(JsonMappingException e){ + throw new InvalidJSONException(e.getMessage(), e); } if (jsonInputValidator.canValidate(o.getClass())){ RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes(); diff --git a/orcid-core/src/main/java/org/orcid/core/exception/InvalidJSONException.java b/orcid-core/src/main/java/org/orcid/core/exception/InvalidJSONException.java index 9a58bdd0ee0..3cfbbf947a6 100644 --- a/orcid-core/src/main/java/org/orcid/core/exception/InvalidJSONException.java +++ b/orcid-core/src/main/java/org/orcid/core/exception/InvalidJSONException.java @@ -1,13 +1,15 @@ package org.orcid.core.exception; -import java.util.Map; - public class InvalidJSONException extends ApplicationException { private static final long serialVersionUID = 1L; - - public InvalidJSONException(Map params) { - super(params); + + public InvalidJSONException(String message) { + super(message); + } + + public InvalidJSONException(String message, Throwable cause) { + super(message, cause); } } diff --git a/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java index 2eaca411b1a..fed04a51b54 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/read_only/impl/ClientDetailsManagerReadOnlyImpl.java @@ -88,7 +88,7 @@ public ClientDetailsEntity findByClientId(String clientId) { LOGGER.error("Client getId() doesn't match. Requested: " + clientId + " Returned: " + result.getId()); } } catch (NoResultException nre) { - LOGGER.error("Error getting client by id:" + clientId, nre); + LOGGER.error("Client not found: '" + clientId + "'"); } return result; } From 583d5495c09731ba6e05f2bfaf23dea885121a71 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 23 Dec 2024 11:46:08 -0600 Subject: [PATCH 4/5] Fix logic to lock records --- .../persistence/dao/NotificationDaoTest.java | 2 +- .../spring/OrcidAuthenticationProvider.java | 54 ++++++++++--------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java b/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java index 9b2b70922ec..7cda6b0ae50 100644 --- a/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java +++ b/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java @@ -304,7 +304,7 @@ public void testFindLatestByOrcid() throws IllegalAccessException { if (lastId == null) { lastId = freshFromDB.getId(); } else { - assertTrue(lastId < freshFromDB.getId()); + assertTrue(lastId + " is not less than " + freshFromDB.getId(), lastId < freshFromDB.getId()); lastId = freshFromDB.getId(); } } diff --git a/orcid-web/src/main/java/org/orcid/frontend/spring/OrcidAuthenticationProvider.java b/orcid-web/src/main/java/org/orcid/frontend/spring/OrcidAuthenticationProvider.java index 580af789385..de101f3ad37 100644 --- a/orcid-web/src/main/java/org/orcid/frontend/spring/OrcidAuthenticationProvider.java +++ b/orcid-web/src/main/java/org/orcid/frontend/spring/OrcidAuthenticationProvider.java @@ -7,11 +7,13 @@ import javax.annotation.Resource; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.orcid.core.manager.BackupCodeManager; import org.orcid.core.manager.ProfileEntityCacheManager; import org.orcid.core.manager.TwoFactorAuthenticationManager; import org.orcid.core.manager.v3.ProfileEntityManager; import org.orcid.core.manager.v3.read_only.EmailManagerReadOnly; +import org.orcid.core.oauth.OrcidProfileUserDetails; import org.orcid.core.security.OrcidUserDetailsService; import org.orcid.core.togglz.Features; import org.orcid.frontend.web.exception.Bad2FARecoveryCodeException; @@ -67,51 +69,51 @@ public Authentication authenticate(Authentication auth) throws AuthenticationExc ProfileEntity profile = null; Integer signinLockCount = null; Date signinLockStart = null; + String userOrcid = null; boolean succesfulLoginAccountLocked = false; try { - result = super.authenticate(auth); - // 1.retrieve the existing signin lock info profile = getProfileEntity(auth.getName()); - if (profile == null) { - throw new BadCredentialsException("Invalid username or password"); - } - + userOrcid = profile.getId(); if (!Features.ACCOUNT_LOCKOUT_SIMULATION.isActive()) { // 2.lock window active if (isLockThreshHoldExceeded(profile.getSigninLockCount(), profile.getSigninLockStart())) { - LOGGER.info("Correct sign in but threshhold exceeded for: " + profile.getId()); + LOGGER.info("Correct sign in but threshhold exceeded for: " + userOrcid); succesfulLoginAccountLocked = true; - throw new BadCredentialsException("Lock Threashold Exceeded for " + profile.getId()); + throw new BadCredentialsException("Lock Threashold Exceeded for " + userOrcid); } else if ((profile.getSigninLockCount() == null) || (profile.getSigninLockCount() > 0 && Features.ENABLE_ACCOUNT_LOCKOUT.isActive())) { - LOGGER.info("Reset the signin lock after correct login outside of locked window for: " + profile.getId()); - profileEntityManager.resetSigninLock(profile.getId()); + LOGGER.info("Reset the signin lock after correct login outside of locked window for: " + userOrcid); + profileEntityManager.resetSigninLock(userOrcid); } } } catch (BadCredentialsException bce) { // update the DB for lock threshhold fields - try { - if (Features.ENABLE_ACCOUNT_LOCKOUT.isActive() && !succesfulLoginAccountLocked) { + if (Features.ENABLE_ACCOUNT_LOCKOUT.isActive() && !succesfulLoginAccountLocked) { + try { + profile = getProfileEntity(auth.getName()); + userOrcid = profile.getId(); + LOGGER.info("Invalid password attempt updating signin lock"); - if (profile == null) { - profile = getProfileEntity(auth.getName()); - } // get the locking info - List lockInfoList = profileEntityManager.getSigninLock(profile.getId()); - signinLockCount = (Integer) lockInfoList.get(0)[2]; - signinLockStart = (Date) lockInfoList.get(0)[0]; + signinLockCount = profile.getSigninLockCount(); + signinLockStart = profile.getSigninLockStart(); if (signinLockStart == null) { - profileEntityManager.startSigninLock(profile.getId()); + profileEntityManager.startSigninLock(userOrcid); } - profileEntityManager.updateSigninLock(profile.getId(), signinLockCount + 1); - profileEntityCacheManager.remove(profile.getId()); - } - - } catch (Exception ex) { - if (!(ex instanceof javax.persistence.NoResultException)) { - LOGGER.error("Exception while saving sign in lock.", ex); + profileEntityManager.updateSigninLock(userOrcid, (signinLockCount == null ? 1 : (signinLockCount + 1))); + profileEntityCacheManager.remove(userOrcid); + } catch (Exception ex) { + //TODO: This try/catch should be removed as soon as we confirm there are no more exceptions while updating the sign in lock. + if (!(ex instanceof javax.persistence.NoResultException)) { + Throwable rootCause = ExceptionUtils.getRootCause(ex); + if(rootCause != null) { + LOGGER.error("An exception has occurred processing request from user " + auth.getName() + ". " + rootCause.getClass().getSimpleName() + ": " + rootCause.getMessage()); + } else { + LOGGER.error("An exception has occurred processing request from user " + auth.getName() + ". " + ex.getClass().getSimpleName() + ": " + ex.getMessage()); + } + } } } throw bce; From cc7270388d6d4ee620051d0e143803ccd78aa368 Mon Sep 17 00:00:00 2001 From: amontenegro Date: Mon, 23 Dec 2024 11:57:30 -0600 Subject: [PATCH 5/5] Looks like a small delay is needed between each new entry??? --- .../org/orcid/persistence/dao/NotificationDaoTest.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java b/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java index 7cda6b0ae50..38900733811 100644 --- a/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java +++ b/orcid-persistence/src/test/java/org/orcid/persistence/dao/NotificationDaoTest.java @@ -279,7 +279,7 @@ public void testFindPermissionByOrcidAndClient() { } @Test - public void testFindLatestByOrcid() throws IllegalAccessException { + public void testFindLatestByOrcid() throws IllegalAccessException, InterruptedException { NotificationEntity entity = notificationDao.findLatestByOrcid("0000-0000-0000-0004"); assertNull(entity); Long lastId = null; @@ -290,8 +290,8 @@ public void testFindLatestByOrcid() throws IllegalAccessException { FieldUtils.writeField(newEntity, "lastModified", now, true); newEntity.setAmendedSection(AMENDED_SECTION_UNKNOWN); newEntity.setClientSourceId("APP-6666666666666666"); - newEntity.setNotificationIntro("Intro"); - newEntity.setNotificationSubject("Subject"); + newEntity.setNotificationIntro("Intro " + i); + newEntity.setNotificationSubject("Subject " + i); newEntity.setNotificationType(NOTIFICATION_TYPE_AMENDED); newEntity.setOrcid("0000-0000-0000-0004"); newEntity.setSendable(true); @@ -301,12 +301,16 @@ public void testFindLatestByOrcid() throws IllegalAccessException { assertNotNull(freshFromDB); assertNotNull(freshFromDB.getDateCreated()); assertNotNull(freshFromDB.getLastModified()); + assertEquals("Intro " + i, freshFromDB.getNotificationIntro()); + assertEquals("Subject " + i, freshFromDB.getNotificationSubject()); + assertNotNull(newEntity.getId()); if (lastId == null) { lastId = freshFromDB.getId(); } else { assertTrue(lastId + " is not less than " + freshFromDB.getId(), lastId < freshFromDB.getId()); lastId = freshFromDB.getId(); } + Thread.sleep(50); } }