From f044261fe0e1805db336f27d600e5009b685503a Mon Sep 17 00:00:00 2001 From: Becky Reamy Date: Fri, 26 Jul 2019 10:20:39 -0400 Subject: [PATCH 1/3] KPMP-1202: Working shib id into user object --- .../shibboleth/ShibbolethUserService.java | 3 +++ src/main/java/org/kpmp/users/User.java | 21 +++++++++++++------ .../java/org/kpmp/users/UserJsonMixin.java | 7 +++++-- .../shibboleth/ShibbolethUserServiceTest.java | 3 +++ src/test/java/org/kpmp/users/UserTest.java | 11 +++++++--- 5 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/kpmp/shibboleth/ShibbolethUserService.java b/src/main/java/org/kpmp/shibboleth/ShibbolethUserService.java index a9803f13..b82caa41 100644 --- a/src/main/java/org/kpmp/shibboleth/ShibbolethUserService.java +++ b/src/main/java/org/kpmp/shibboleth/ShibbolethUserService.java @@ -28,12 +28,15 @@ public User getUser(HttpServletRequest request) throws UnsupportedEncodingExcept String firstName = encoder.convertFromLatin1(value); value = handleNull(request.getHeader("sn")); String lastName = encoder.convertFromLatin1(value); + value = handleNull(request.getHeader("eppn")); + String shibId = encoder.convertFromLatin1(value); User user = new User(); user.setDisplayName(displayName); user.setLastName(lastName); user.setFirstName(firstName); user.setEmail(email); + user.setShibId(shibId); return user; diff --git a/src/main/java/org/kpmp/users/User.java b/src/main/java/org/kpmp/users/User.java index d3d31271..0ed740ce 100755 --- a/src/main/java/org/kpmp/users/User.java +++ b/src/main/java/org/kpmp/users/User.java @@ -12,11 +12,12 @@ public class User { @Id - String id; - String firstName; - String lastName; - String displayName; - String email; + private String id; + private String firstName; + private String lastName; + private String displayName; + private String email; + private String shibId; public String getId() { return id; @@ -61,7 +62,7 @@ public void setEmail(String emailAddress) { @Override public String toString() { return "userId: " + id + ", firstName: " + firstName + ", lastName: " + lastName + ", displayName: " - + displayName + ", email: " + email; + + displayName + ", email: " + email + ", shibId: " + shibId; } public String generateJSON() throws JsonProcessingException { @@ -76,4 +77,12 @@ public String generateJSONForApp() throws JsonProcessingException { ObjectMapper mapper = new ObjectMapper(); return mapper.writeValueAsString(this); } + + public String getShibId() { + return shibId; + } + + public void setShibId(String shibId) { + this.shibId = shibId; + } } diff --git a/src/main/java/org/kpmp/users/UserJsonMixin.java b/src/main/java/org/kpmp/users/UserJsonMixin.java index 03ce5f99..0aded1a0 100755 --- a/src/main/java/org/kpmp/users/UserJsonMixin.java +++ b/src/main/java/org/kpmp/users/UserJsonMixin.java @@ -4,7 +4,10 @@ public abstract class UserJsonMixin { - @JsonIgnore - abstract String getId(); + @JsonIgnore + abstract String getId(); + + @JsonIgnore + abstract String getShibId(); } diff --git a/src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java b/src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java index 98b78155..2380ca6b 100644 --- a/src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java +++ b/src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java @@ -39,10 +39,12 @@ public void testGetUser() throws UnsupportedEncodingException { when(request.getHeader("givenname")).thenReturn("Johnny"); when(request.getHeader("sn")).thenReturn("Cash"); when(request.getHeader("displayname")).thenReturn("Johnny Cash"); + when(request.getHeader("eppn")).thenReturn("shibId"); when(utf8Encoder.convertFromLatin1("Johnny")).thenReturn("Johnny"); when(utf8Encoder.convertFromLatin1("Cash")).thenReturn("Cash"); when(utf8Encoder.convertFromLatin1("Johnny Cash")).thenReturn("Johnny Cash"); when(utf8Encoder.convertFromLatin1("maninblack@jcash.com")).thenReturn("maninblack@jcash.com"); + when(utf8Encoder.convertFromLatin1("shibId")).thenReturn("shibId"); User user = shibbolethUserService.getUser(request); @@ -50,6 +52,7 @@ public void testGetUser() throws UnsupportedEncodingException { assertEquals("Johnny Cash", user.getDisplayName()); assertEquals("Cash", user.getLastName()); assertEquals("Johnny", user.getFirstName()); + assertEquals("shibId", user.getShibId()); } diff --git a/src/test/java/org/kpmp/users/UserTest.java b/src/test/java/org/kpmp/users/UserTest.java index 13602b4b..4f5a3d62 100755 --- a/src/test/java/org/kpmp/users/UserTest.java +++ b/src/test/java/org/kpmp/users/UserTest.java @@ -57,8 +57,9 @@ public void testToString() { testUser.setFirstName("Ziggy"); testUser.setLastName("Stardust"); testUser.setEmail("ziggy@mars.com"); + testUser.setShibId("ziggy@mars.com"); assertEquals("userId: 12345" + ", firstName: Ziggy" + ", lastName: Stardust" + ", displayName: Space Oddity" - + ", email: ziggy@mars.com", testUser.toString()); + + ", email: ziggy@mars.com, shibId: ziggy@mars.com", testUser.toString()); } @Test @@ -68,6 +69,7 @@ public void testGenerateJSON() throws Exception { testUser.setFirstName("firstName"); testUser.setId("id"); testUser.setLastName("lastName"); + testUser.setShibId("shibId"); assertEquals("{\"firstName\":\"firstName\",\"lastName\":\"lastName\"," + "\"displayName\":\"displayName\",\"email\":\"emailAddress\"}", testUser.generateJSON()); @@ -80,8 +82,11 @@ public void testGenerateJSONForApp() throws Exception { testUser.setFirstName("firstName"); testUser.setId("id"); testUser.setLastName("lastName"); + testUser.setShibId("shibId"); - assertEquals("{\"id\":\"id\",\"firstName\":\"firstName\",\"lastName\":\"lastName\"," - + "\"displayName\":\"displayName\",\"email\":\"emailAddress\"}", testUser.generateJSONForApp()); + assertEquals( + "{\"id\":\"id\",\"firstName\":\"firstName\",\"lastName\":\"lastName\"," + + "\"displayName\":\"displayName\",\"email\":\"emailAddress\",\"shibId\":\"shibId\"}", + testUser.generateJSONForApp()); } } From 3000d0c7a00afdba1849ad3b1bb4cc24418c5efc Mon Sep 17 00:00:00 2001 From: Becky Reamy Date: Fri, 26 Jul 2019 11:02:45 -0400 Subject: [PATCH 2/3] KPMP-1202: Fixing tests and ensuring we are capturing user information in logs --- .../org/kpmp/filters/AuthorizationFilter.java | 2 +- .../java/org/kpmp/logging/LoggingService.java | 20 ++++++------ .../packages/CustomPackageRepository.java | 21 ++++-------- .../org/kpmp/packages/PackageController.java | 8 +++-- .../org/kpmp/packages/PackageService.java | 27 ++++++++-------- .../kpmp/filters/AuthorizationFilterTest.java | 4 +-- .../packages/CustomPackageRepositoryTest.java | 32 ++++++++----------- .../kpmp/packages/PackageControllerTest.java | 25 ++++++++++----- .../org/kpmp/packages/PackageServiceTest.java | 19 +++++------ .../state/StateHandlerServiceTest.java | 3 +- 10 files changed, 79 insertions(+), 82 deletions(-) diff --git a/src/main/java/org/kpmp/filters/AuthorizationFilter.java b/src/main/java/org/kpmp/filters/AuthorizationFilter.java index fda9b366..e52feb91 100644 --- a/src/main/java/org/kpmp/filters/AuthorizationFilter.java +++ b/src/main/java/org/kpmp/filters/AuthorizationFilter.java @@ -43,7 +43,7 @@ public void doFilter(ServletRequest incomingRequest, ServletResponse incomingRes // This is where we will implement the logic to talk to the user portal and do // authorization. - logger.logInfoMessage(this.getClass(), user.getEmail(), null, this.getClass().getSimpleName() + ".doFilter", + logger.logInfoMessage(this.getClass(), user, null, this.getClass().getSimpleName() + ".doFilter", "Passing through authentication filter"); chain.doFilter(incomingRequest, incomingResponse); } diff --git a/src/main/java/org/kpmp/logging/LoggingService.java b/src/main/java/org/kpmp/logging/LoggingService.java index bbe98367..197f39ae 100644 --- a/src/main/java/org/kpmp/logging/LoggingService.java +++ b/src/main/java/org/kpmp/logging/LoggingService.java @@ -14,7 +14,7 @@ @Service public class LoggingService { - private static final String LOG_MESSAGE_FORMAT = "USERID: {} | PKGID: {} | URI: {} | MSG: {} "; + private static final String LOG_MESSAGE_FORMAT = "USER: {} | PKGID: {} | URI: {} | MSG: {} "; private ShibbolethUserService shibUserService; @Autowired @@ -27,7 +27,7 @@ public void logInfoMessage(Class clazz, String packageId, String message, HttpSe Logger log = LoggerFactory.getLogger(clazz); try { User user = shibUserService.getUser(request); - log.info(LOG_MESSAGE_FORMAT, user.getEmail(), packageId, request.getRequestURI(), message); + log.info(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); } catch (UnsupportedEncodingException e) { log.error(LOG_MESSAGE_FORMAT, "unknown", packageId, request.getRequestURI(), "ERROR: Unable to get user information from request: " + e.getMessage()); @@ -35,9 +35,9 @@ public void logInfoMessage(Class clazz, String packageId, String message, HttpSe } @SuppressWarnings("rawtypes") - public void logInfoMessage(Class clazz, String userId, String packageId, String uri, String message) { + public void logInfoMessage(Class clazz, User user, String packageId, String uri, String message) { Logger log = LoggerFactory.getLogger(clazz); - log.info(LOG_MESSAGE_FORMAT, userId, packageId, uri, message); + log.info(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); } @SuppressWarnings("rawtypes") @@ -45,7 +45,7 @@ public void logErrorMessage(Class clazz, String packageId, String message, HttpS Logger log = LoggerFactory.getLogger(clazz); try { User user = shibUserService.getUser(request); - log.error(LOG_MESSAGE_FORMAT, user.getEmail(), packageId, request.getRequestURI(), message); + log.error(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); } catch (UnsupportedEncodingException e) { log.error(LOG_MESSAGE_FORMAT, "unknown", packageId, request.getRequestURI(), "ERROR: Unable to get user information from request: " + e.getMessage()); @@ -53,9 +53,9 @@ public void logErrorMessage(Class clazz, String packageId, String message, HttpS } @SuppressWarnings("rawtypes") - public void logErrorMessage(Class clazz, String userId, String packageId, String uri, String message) { + public void logErrorMessage(Class clazz, User user, String packageId, String uri, String message) { Logger log = LoggerFactory.getLogger(clazz); - log.error(LOG_MESSAGE_FORMAT, userId, packageId, uri, message); + log.error(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); } @SuppressWarnings("rawtypes") @@ -63,7 +63,7 @@ public void logWarnMessage(Class clazz, String packageId, String message, HttpSe Logger log = LoggerFactory.getLogger(clazz); try { User user = shibUserService.getUser(request); - log.warn(LOG_MESSAGE_FORMAT, user.getEmail(), packageId, request.getRequestURI(), message); + log.warn(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); } catch (UnsupportedEncodingException e) { log.error(LOG_MESSAGE_FORMAT, "unknown", packageId, request.getRequestURI(), "ERROR: Unable to get user information from request: " + e.getMessage()); @@ -71,9 +71,9 @@ public void logWarnMessage(Class clazz, String packageId, String message, HttpSe } @SuppressWarnings("rawtypes") - public void logWarnMessage(Class clazz, String userId, String packageId, String uri, String message) { + public void logWarnMessage(Class clazz, User user, String packageId, String uri, String message) { Logger log = LoggerFactory.getLogger(clazz); - log.warn(LOG_MESSAGE_FORMAT, userId, packageId, uri, message); + log.warn(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); } } diff --git a/src/main/java/org/kpmp/packages/CustomPackageRepository.java b/src/main/java/org/kpmp/packages/CustomPackageRepository.java index 91f272b3..98045829 100755 --- a/src/main/java/org/kpmp/packages/CustomPackageRepository.java +++ b/src/main/java/org/kpmp/packages/CustomPackageRepository.java @@ -62,13 +62,13 @@ public CustomPackageRepository(PackageRepository repo, MongoTemplate mongoTempla this.logger = logger; } - public String saveDynamicForm(JSONObject packageMetadata) throws JSONException { + public String saveDynamicForm(JSONObject packageMetadata, User userFromHeader) throws JSONException { Date startTime = new Date(); String packageId = universalIdGenerator.generateUniversalId(); - String submitterEmail = packageMetadata.getString(PackageKeys.SUBMITTER_EMAIL.getKey()); + String submitterEmail = userFromHeader.getEmail(); JSONArray files = packageMetadata.getJSONArray(PackageKeys.FILES.getKey()); - logger.logInfoMessage(this.getClass(), submitterEmail, packageId, + logger.logInfoMessage(this.getClass(), userFromHeader, packageId, this.getClass().getSimpleName() + ".saveDynamicForm", fileUploadStartTiming.format(new Object[] { startTime, submitterEmail, packageId, files.length() })); @@ -77,7 +77,7 @@ public String saveDynamicForm(JSONObject packageMetadata) throws JSONException { file.put(PackageKeys.ID.getKey(), universalIdGenerator.generateUniversalId()); } - User user = findUser(packageMetadata, submitterEmail); + User user = findUser(userFromHeader); cleanUpObject(packageMetadata); DBRef userRef = new DBRef(USERS_COLLECTION, new ObjectId(user.getId())); @@ -102,17 +102,10 @@ private void cleanUpObject(JSONObject packageMetadata) { packageMetadata.remove(PackageKeys.SUBMITTER_LAST_NAME.getKey()); } - private User findUser(JSONObject packageMetadata, String submitterEmail) throws JSONException { - User user = userRepository.findByEmail(submitterEmail); + private User findUser(User userFromHeader) throws JSONException { + User user = userRepository.findByEmail(userFromHeader.getEmail()); if (user == null) { - User newUser = new User(); - JSONObject submitter = packageMetadata.getJSONObject(PackageKeys.SUBMITTER.getKey()); - if (submitter.has(PackageKeys.DISPLAY_NAME.getKey())) { - newUser.setDisplayName(submitter.getString(PackageKeys.DISPLAY_NAME.getKey())); - } - newUser.setEmail(submitter.getString(PackageKeys.EMAIL.getKey())); - newUser.setFirstName(submitter.getString(PackageKeys.FIRST_NAME.getKey())); - newUser.setLastName(submitter.getString(PackageKeys.LAST_NAME.getKey())); + User newUser = userFromHeader; user = userRepository.save(newUser); } return user; diff --git a/src/main/java/org/kpmp/packages/PackageController.java b/src/main/java/org/kpmp/packages/PackageController.java index d613740f..7809f800 100755 --- a/src/main/java/org/kpmp/packages/PackageController.java +++ b/src/main/java/org/kpmp/packages/PackageController.java @@ -11,6 +11,7 @@ import org.json.JSONObject; import org.kpmp.logging.LoggingService; import org.kpmp.shibboleth.ShibbolethUserService; +import org.kpmp.users.User; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.core.io.Resource; import org.springframework.core.io.UrlResource; @@ -57,10 +58,11 @@ public PackageController(PackageService packageService, LoggingService logger, @RequestMapping(value = "/v1/packages", method = RequestMethod.POST) public @ResponseBody String postPackageInformation(@RequestBody String packageInfoString, - HttpServletRequest request) throws JSONException { + HttpServletRequest request) throws JSONException, UnsupportedEncodingException { JSONObject packageInfo = new JSONObject(packageInfoString); logger.logInfoMessage(this.getClass(), null, "Posting package info: " + packageInfo, request); - String packageId = packageService.savePackageInformation(packageInfo); + User user = shibUserService.getUser(request); + String packageId = packageService.savePackageInformation(packageInfo, user); return packageId; } @@ -110,7 +112,7 @@ public PackageController(PackageService packageService, LoggingService logger, if (packageService.validatePackageForZipping(packageId, shibUserService.getUser(request))) { try { String removeErrantEqualSign = hostname.replace("=", ""); - packageService.createZipFile(packageId, removeErrantEqualSign); + packageService.createZipFile(packageId, removeErrantEqualSign, shibUserService.getUser(request)); fileUploadResponse = new FileUploadResponse(true); } catch (Exception e) { diff --git a/src/main/java/org/kpmp/packages/PackageService.java b/src/main/java/org/kpmp/packages/PackageService.java index aa8a6dd0..b6c230a7 100755 --- a/src/main/java/org/kpmp/packages/PackageService.java +++ b/src/main/java/org/kpmp/packages/PackageService.java @@ -77,8 +77,8 @@ public Path getPackageFile(String packageId) { return filePath; } - public String savePackageInformation(JSONObject packageMetadata) throws JSONException { - return packageRepository.saveDynamicForm(packageMetadata); + public String savePackageInformation(JSONObject packageMetadata, User user) throws JSONException { + return packageRepository.saveDynamicForm(packageMetadata, user); } public Package findPackage(String packageId) { @@ -94,10 +94,10 @@ public void saveFile(MultipartFile file, String packageId, String filename, bool packageFileHandler.saveMultipartFile(file, packageId, filename, shouldAppend); } - public void createZipFile(String packageId, String origin) throws Exception { + public void createZipFile(String packageId, String origin, User user) throws Exception { Package packageInfo = packageRepository.findByPackageId(packageId); - String packageMetadata = packageRepository.getJSONByPackageId(packageId); + List attachments = packageInfo.getAttachments(); String displaySize = FileUtils.byteCountToDisplaySize(getTotalSizeOfAttachmentsInBytes(attachments)); Date finishUploadTime = new Date(); @@ -105,31 +105,30 @@ public void createZipFile(String packageId, String origin) throws Exception { double uploadRate = calculateUploadRate(duration, attachments); DecimalFormat rateFormat = new DecimalFormat("###.###"); - String submitterEmail = packageInfo.getSubmitter().getEmail(); - logger.logInfoMessage(this.getClass(), submitterEmail, packageId, - this.getClass().getSimpleName() + ".createZipFile", + logger.logInfoMessage(this.getClass(), user, packageId, this.getClass().getSimpleName() + ".createZipFile", fileUploadFinishTiming - .format(new Object[] { finishUploadTime, submitterEmail, packageId, attachments.size(), + .format(new Object[] { finishUploadTime, user.toString(), packageId, attachments.size(), displaySize, duration + " seconds", rateFormat.format(uploadRate) + " MB/sec" })); new Thread() { public void run() { try { + String packageMetadata = packageRepository.getJSONByPackageId(packageId); packageZipper.createZipFile(packageMetadata); stateHandler.sendNotification(packageId, packageInfo.getPackageType(), packageInfo.getCreatedAt(), packageInfo.getSubmitter().getFirstName(), packageInfo.getSubmitter().getLastName(), packageInfo.getSubjectId(), origin); } catch (Exception e) { - logger.logErrorMessage(PackageService.class, submitterEmail, packageId, - PackageService.class.getSimpleName(), e.getMessage()); + logger.logErrorMessage(PackageService.class, user, packageId, PackageService.class.getSimpleName(), + e.getMessage()); } logger.logInfoMessage(PackageService.class, null, packageId, PackageService.class.getSimpleName() + ".createZipFile", zipPackage.format(new Object[] { "Zip file created for package: ", packageId })); long zipDuration = calculateDurationInSeconds(finishUploadTime, new Date()); - logger.logInfoMessage(PackageService.class, submitterEmail, packageId, + logger.logInfoMessage(PackageService.class, user, packageId, PackageService.class.getSimpleName() + ".createZipFile", - zipTiming.format(new Object[] { packageInfo.getCreatedAt(), submitterEmail, packageId, + zipTiming.format(new Object[] { packageInfo.getCreatedAt(), user.toString(), packageId, packageInfo.getAttachments().size(), displaySize, zipDuration + " seconds" })); } @@ -178,7 +177,7 @@ protected boolean validateFileLengthsMatch(List filesInPackage, Stri for (Attachment attachment : filesInPackage) { String filename = attachment.getFileName(); if (new File(packagePath + filename).length() != attachment.getSize()) { - logger.logErrorMessage(this.getClass(), user.getEmail(), packageId, + logger.logErrorMessage(this.getClass(), user, packageId, this.getClass().getSimpleName() + ".validateFileLengthsMatch", zipIssue.format(new Object[] { "File size in metadata does not match file size on disk for file: " + filename })); everythingMatches = false; @@ -191,7 +190,7 @@ protected boolean checkFilesExist(List filesOnDisk, List filesIn User user) { boolean sameFiles = filesOnDisk.equals(filesInPackage); if (!sameFiles) { - logger.logErrorMessage(this.getClass(), user.getEmail(), packageId, + logger.logErrorMessage(this.getClass(), user, packageId, this.getClass().getSimpleName() + ".checkFilesExist", zipIssue.format(new Object[] { "File list in metadata does not match file list on disk" })); } diff --git a/src/test/java/org/kpmp/filters/AuthorizationFilterTest.java b/src/test/java/org/kpmp/filters/AuthorizationFilterTest.java index 68deca25..b7cdd332 100644 --- a/src/test/java/org/kpmp/filters/AuthorizationFilterTest.java +++ b/src/test/java/org/kpmp/filters/AuthorizationFilterTest.java @@ -61,8 +61,8 @@ public void testDoFilter() throws Exception { filter.doFilter(incomingRequest, incomingResponse, chain); verify(chain).doFilter(incomingRequest, incomingResponse); - verify(logger, times(1)).logInfoMessage(AuthorizationFilter.class, "jimmy@buffet.org", null, - "AuthorizationFilter.doFilter", "Passing through authentication filter"); + verify(logger, times(1)).logInfoMessage(AuthorizationFilter.class, user, null, "AuthorizationFilter.doFilter", + "Passing through authentication filter"); } @Test diff --git a/src/test/java/org/kpmp/packages/CustomPackageRepositoryTest.java b/src/test/java/org/kpmp/packages/CustomPackageRepositoryTest.java index 1d2f244c..0c45024e 100644 --- a/src/test/java/org/kpmp/packages/CustomPackageRepositoryTest.java +++ b/src/test/java/org/kpmp/packages/CustomPackageRepositoryTest.java @@ -73,6 +73,7 @@ public void testSaveDynamicForm_happyPath() throws Exception { when(universalIdGenerator.generateUniversalId()).thenReturn("123").thenReturn("456"); when(packageMetadata.getString("submitterEmail")).thenReturn("emailAddress"); User user = mock(User.class); + when(user.getEmail()).thenReturn("emailAddress"); when(user.getId()).thenReturn("5c2f9e01cb5e710049f33121"); when(userRepo.findByEmail("emailAddress")).thenReturn(user); JSONArray files = mock(JSONArray.class); @@ -83,7 +84,7 @@ public void testSaveDynamicForm_happyPath() throws Exception { MongoCollection mongoCollection = mock(MongoCollection.class); when(mongoTemplate.getCollection("packages")).thenReturn(mongoCollection); - String packageId = repo.saveDynamicForm(packageMetadata); + String packageId = repo.saveDynamicForm(packageMetadata, user); ArgumentCaptor documentCaptor = ArgumentCaptor.forClass(Document.class); verify(mongoCollection).insertOne(documentCaptor.capture()); @@ -103,13 +104,13 @@ public void testSaveDynamicForm_happyPath() throws Exception { assertEquals(new ObjectId("5c2f9e01cb5e710049f33121"), objectId); ArgumentCaptor messageCaptor = ArgumentCaptor.forClass(String.class); ArgumentCaptor classCaptor = ArgumentCaptor.forClass(Class.class); - ArgumentCaptor userIdCaptor = ArgumentCaptor.forClass(String.class); + ArgumentCaptor userCaptor = ArgumentCaptor.forClass(User.class); ArgumentCaptor packageIdCaptor = ArgumentCaptor.forClass(String.class); ArgumentCaptor uriCaptor = ArgumentCaptor.forClass(String.class); - verify(logger).logInfoMessage(classCaptor.capture(), userIdCaptor.capture(), packageIdCaptor.capture(), + verify(logger).logInfoMessage(classCaptor.capture(), userCaptor.capture(), packageIdCaptor.capture(), uriCaptor.capture(), messageCaptor.capture()); assertEquals(CustomPackageRepository.class, classCaptor.getValue()); - assertEquals("emailAddress", userIdCaptor.getValue()); + assertEquals(user, userCaptor.getValue()); assertEquals(packageId, packageIdCaptor.getValue()); assertEquals("CustomPackageRepository.saveDynamicForm", uriCaptor.getValue()); assertEquals(true, messageCaptor.getValue().startsWith("Timing|start|")); @@ -120,20 +121,16 @@ public void testSaveDynamicForm_happyPath() throws Exception { @Test public void testSaveDynamicForm_whenNewUser() throws Exception { JSONObject packageMetadata = mock(JSONObject.class); - JSONObject mockSubmitter = mock(JSONObject.class); when(packageMetadata.toString()).thenReturn("{}"); when(universalIdGenerator.generateUniversalId()).thenReturn("123").thenReturn("456"); - when(packageMetadata.getString("submitterEmail")).thenReturn("emailAddress"); - when(packageMetadata.getJSONObject("submitter")).thenReturn(mockSubmitter); - when(mockSubmitter.getString("displayName")).thenReturn("displayName"); - when(mockSubmitter.getString("email")).thenReturn("emailAddress2"); - when(mockSubmitter.getString("firstName")).thenReturn("firstName"); - when(mockSubmitter.getString("lastName")).thenReturn("lastName"); - when(mockSubmitter.has("displayName")).thenReturn(true); User user = mock(User.class); + when(user.getDisplayName()).thenReturn("displayName"); + when(user.getEmail()).thenReturn("emailAddress2"); + when(user.getFirstName()).thenReturn("firstName"); + when(user.getLastName()).thenReturn("lastName"); when(user.getId()).thenReturn("5c2f9e01cb5e710049f33121"); when(userRepo.save(any(User.class))).thenReturn(user); - when(userRepo.findByEmail("emailAddress")).thenReturn(null); + when(userRepo.findByEmail("emailAddress2")).thenReturn(null); JSONArray files = mock(JSONArray.class); when(files.length()).thenReturn(1); JSONObject file = mock(JSONObject.class); @@ -142,7 +139,7 @@ public void testSaveDynamicForm_whenNewUser() throws Exception { MongoCollection mongoCollection = mock(MongoCollection.class); when(mongoTemplate.getCollection("packages")).thenReturn(mongoCollection); - String packageId = repo.saveDynamicForm(packageMetadata); + String packageId = repo.saveDynamicForm(packageMetadata, user); ArgumentCaptor documentCaptor = ArgumentCaptor.forClass(Document.class); verify(mongoCollection).insertOne(documentCaptor.capture()); @@ -169,17 +166,16 @@ public void testSaveDynamicForm_whenNewUser() throws Exception { assertEquals("lastName", actualUser.getLastName()); ArgumentCaptor messageCaptor = ArgumentCaptor.forClass(String.class); ArgumentCaptor classCaptor = ArgumentCaptor.forClass(Class.class); - ArgumentCaptor userIdCaptor = ArgumentCaptor.forClass(String.class); ArgumentCaptor packageIdCaptor = ArgumentCaptor.forClass(String.class); ArgumentCaptor uriCaptor = ArgumentCaptor.forClass(String.class); - verify(logger).logInfoMessage(classCaptor.capture(), userIdCaptor.capture(), packageIdCaptor.capture(), + verify(logger).logInfoMessage(classCaptor.capture(), userCaptor.capture(), packageIdCaptor.capture(), uriCaptor.capture(), messageCaptor.capture()); assertEquals(CustomPackageRepository.class, classCaptor.getValue()); - assertEquals("emailAddress", userIdCaptor.getValue()); + assertEquals(user, userCaptor.getValue()); assertEquals(packageId, packageIdCaptor.getValue()); assertEquals("CustomPackageRepository.saveDynamicForm", uriCaptor.getValue()); assertEquals(true, messageCaptor.getValue().startsWith("Timing|start|")); - assertEquals(true, messageCaptor.getValue().endsWith("|emailAddress|123|1 files")); + assertEquals(true, messageCaptor.getValue().endsWith("|emailAddress2|123|1 files")); } @Test diff --git a/src/test/java/org/kpmp/packages/PackageControllerTest.java b/src/test/java/org/kpmp/packages/PackageControllerTest.java index 7f58c564..a79088a7 100755 --- a/src/test/java/org/kpmp/packages/PackageControllerTest.java +++ b/src/test/java/org/kpmp/packages/PackageControllerTest.java @@ -26,6 +26,7 @@ import org.kpmp.logging.LoggingService; import org.kpmp.shibboleth.ShibbolethUserService; import org.kpmp.users.User; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.springframework.core.io.Resource; @@ -70,15 +71,23 @@ public void testGetAllPackages() throws JSONException, IOException { @Test public void testPostPackageInfo() throws Exception { - String packageInfoString = "{}"; - when(packageService.savePackageInformation(any(JSONObject.class))).thenReturn("universalId"); + String packageInfoString = "{\"packageType\":\"blah\"}"; + + when(packageService.savePackageInformation(any(JSONObject.class), any(User.class))).thenReturn("universalId"); HttpServletRequest request = mock(HttpServletRequest.class); + User user = mock(User.class); + when(shibUserService.getUser(request)).thenReturn(user); String universalId = controller.postPackageInformation(packageInfoString, request); assertEquals("universalId", universalId); - verify(packageService).savePackageInformation(any(JSONObject.class)); - verify(logger).logInfoMessage(PackageController.class, null, "Posting package info: {}", request); + ArgumentCaptor jsonCaptor = ArgumentCaptor.forClass(JSONObject.class); + ArgumentCaptor userCaptor = ArgumentCaptor.forClass(User.class); + verify(packageService).savePackageInformation(jsonCaptor.capture(), userCaptor.capture()); + assertEquals(user, userCaptor.getValue()); + assertEquals("blah", jsonCaptor.getValue().get("packageType")); + verify(logger).logInfoMessage(PackageController.class, null, "Posting package info: {\"packageType\":\"blah\"}", + request); } @Test @@ -116,7 +125,7 @@ public void testFinishUpload() throws Exception { FileUploadResponse result = controller.finishUpload("3545", "origin", request); - verify(packageService).createZipFile("3545", "origin"); + verify(packageService).createZipFile("3545", "origin", user); verify(packageService).validatePackageForZipping("3545", user); assertEquals(true, result.isSuccess()); verify(logger).logInfoMessage(PackageController.class, "3545", "Finishing file upload with packageId: 3545", @@ -129,11 +138,11 @@ public void testFinishUpload_whenCreateZipThrows() throws Exception { User user = mock(User.class); when(shibUserService.getUser(request)).thenReturn(user); when(packageService.validatePackageForZipping("3545", user)).thenReturn(true); - doThrow(new JSONException("OOF")).when(packageService).createZipFile("3545", "origin"); + doThrow(new JSONException("OOF")).when(packageService).createZipFile("3545", "origin", user); FileUploadResponse result = controller.finishUpload("3545", "origin", request); - verify(packageService).createZipFile("3545", "origin"); + verify(packageService).createZipFile("3545", "origin", user); verify(packageService).validatePackageForZipping("3545", user); assertEquals(false, result.isSuccess()); verify(logger).logErrorMessage(PackageController.class, "3545", "error getting metadata for package id: 3545", @@ -149,7 +158,7 @@ public void testFinishUpload_whenMismatchedFiles() throws Exception { FileUploadResponse result = controller.finishUpload("3545", "origin", request); - verify(packageService, times(0)).createZipFile("3545", "origin"); + verify(packageService, times(0)).createZipFile("3545", "origin", user); verify(packageService).validatePackageForZipping("3545", user); assertEquals(false, result.isSuccess()); verify(logger).logErrorMessage(PackageController.class, "3545", "Unable to zip package with package id: 3545", diff --git a/src/test/java/org/kpmp/packages/PackageServiceTest.java b/src/test/java/org/kpmp/packages/PackageServiceTest.java index 3819b12a..afcea7df 100755 --- a/src/test/java/org/kpmp/packages/PackageServiceTest.java +++ b/src/test/java/org/kpmp/packages/PackageServiceTest.java @@ -81,12 +81,13 @@ public void testFindAllPackages() throws JSONException, IOException { @Test public void testSavePackageInformation() throws Exception { JSONObject packageMetadata = mock(JSONObject.class); - when(packageRepository.saveDynamicForm(packageMetadata)).thenReturn("awesomeNewId"); + User user = mock(User.class); + when(packageRepository.saveDynamicForm(packageMetadata, user)).thenReturn("awesomeNewId"); - String packageId = service.savePackageInformation(packageMetadata); + String packageId = service.savePackageInformation(packageMetadata, user); assertEquals("awesomeNewId", packageId); - verify(packageRepository).saveDynamicForm(packageMetadata); + verify(packageRepository).saveDynamicForm(packageMetadata, user); } @Test @@ -276,12 +277,11 @@ public void testValidateFileLengthsMatch_whenMatch() throws Exception { attachment2.setSize(file2.length()); List attachments = Arrays.asList(attachment1, attachment2); User user = mock(User.class); - when(user.getEmail()).thenReturn("email"); assertEquals(true, service.validateFileLengthsMatch(attachments, packagePath.toString(), "packageId", user)); List logsList = listAppender.list; assertEquals(0, logsList.size()); - verify(logger, times(0)).logErrorMessage(PackageService.class, "email", "packageId", + verify(logger, times(0)).logErrorMessage(PackageService.class, user, "packageId", "PackageService.validateFileLengthsMatch", "ERROR|zip|File size in metadata does not match file size on disk for file: file2"); } @@ -306,10 +306,9 @@ public void testValidateFileLengthsMatch_whenNoMatch() throws Exception { attachment2.setSize(1234l); List attachments = Arrays.asList(attachment1, attachment2); User user = mock(User.class); - when(user.getEmail()).thenReturn("email"); assertEquals(false, service.validateFileLengthsMatch(attachments, packagePath.toString(), "packageId", user)); - verify(logger).logErrorMessage(PackageService.class, "email", "packageId", + verify(logger).logErrorMessage(PackageService.class, user, "packageId", "PackageService.validateFileLengthsMatch", "ERROR|zip|File size in metadata does not match file size on disk for file: file2"); @@ -318,22 +317,20 @@ public void testValidateFileLengthsMatch_whenNoMatch() throws Exception { @Test public void testCheckFilesExistTrue() throws Exception { User user = mock(User.class); - when(user.getEmail()).thenReturn("email"); assertEquals(true, service.checkFilesExist(Arrays.asList("file1", "file2"), Arrays.asList("file1", "file2"), "packageId", user)); - verify(logger, times(0)).logErrorMessage(PackageService.class, "email", "packageId", + verify(logger, times(0)).logErrorMessage(PackageService.class, user, "packageId", "PackageService.checkFilesExist", "ERROR|zip|File list in metadata does not match file list on disk"); } @Test public void testCheckFilesExistFalse() throws Exception { User user = mock(User.class); - when(user.getEmail()).thenReturn("email"); assertEquals(false, service.checkFilesExist(Arrays.asList("file1", "file2"), Arrays.asList("file1", "file2, file3"), "packageId", user)); - verify(logger).logErrorMessage(PackageService.class, "email", "packageId", "PackageService.checkFilesExist", + verify(logger).logErrorMessage(PackageService.class, user, "packageId", "PackageService.checkFilesExist", "ERROR|zip|File list in metadata does not match file list on disk"); } diff --git a/src/test/java/org/kpmp/packages/state/StateHandlerServiceTest.java b/src/test/java/org/kpmp/packages/state/StateHandlerServiceTest.java index ce135994..5d24f28f 100644 --- a/src/test/java/org/kpmp/packages/state/StateHandlerServiceTest.java +++ b/src/test/java/org/kpmp/packages/state/StateHandlerServiceTest.java @@ -12,6 +12,7 @@ import org.junit.Before; import org.junit.Test; import org.kpmp.logging.LoggingService; +import org.kpmp.users.User; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -63,7 +64,7 @@ public void testSendNotification() { assertEquals("specimenId", packageInfo.getSpecimenId()); assertEquals("origin", packageInfo.getOrigin()); assertEquals(Boolean.class, classCaptor.getValue()); - verify(logger, times(0)).logErrorMessage(any(Class.class), any(String.class), any(String.class), + verify(logger, times(0)).logErrorMessage(any(Class.class), any(User.class), any(String.class), any(String.class), any(String.class)); } From c71381c97e7f3da6fcb2ea8b217fbd1cdaf91984 Mon Sep 17 00:00:00 2001 From: Becky Reamy Date: Fri, 26 Jul 2019 11:35:38 -0400 Subject: [PATCH 3/3] KPMP-1202: Adjust the logging to handle when we have a null user --- .../java/org/kpmp/logging/LoggingService.java | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/kpmp/logging/LoggingService.java b/src/main/java/org/kpmp/logging/LoggingService.java index 197f39ae..ea938c12 100644 --- a/src/main/java/org/kpmp/logging/LoggingService.java +++ b/src/main/java/org/kpmp/logging/LoggingService.java @@ -27,7 +27,11 @@ public void logInfoMessage(Class clazz, String packageId, String message, HttpSe Logger log = LoggerFactory.getLogger(clazz); try { User user = shibUserService.getUser(request); - log.info(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); + if (user == null) { + log.info(LOG_MESSAGE_FORMAT, "Unknown user", packageId, request.getRequestURI(), message); + } else { + log.info(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); + } } catch (UnsupportedEncodingException e) { log.error(LOG_MESSAGE_FORMAT, "unknown", packageId, request.getRequestURI(), "ERROR: Unable to get user information from request: " + e.getMessage()); @@ -37,7 +41,11 @@ public void logInfoMessage(Class clazz, String packageId, String message, HttpSe @SuppressWarnings("rawtypes") public void logInfoMessage(Class clazz, User user, String packageId, String uri, String message) { Logger log = LoggerFactory.getLogger(clazz); - log.info(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); + if (user == null) { + log.info(LOG_MESSAGE_FORMAT, "Unknown user", packageId, uri, message); + } else { + log.info(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); + } } @SuppressWarnings("rawtypes") @@ -45,7 +53,11 @@ public void logErrorMessage(Class clazz, String packageId, String message, HttpS Logger log = LoggerFactory.getLogger(clazz); try { User user = shibUserService.getUser(request); - log.error(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); + if (user == null) { + log.error(LOG_MESSAGE_FORMAT, "Unknown user", packageId, request.getRequestURI(), message); + } else { + log.error(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); + } } catch (UnsupportedEncodingException e) { log.error(LOG_MESSAGE_FORMAT, "unknown", packageId, request.getRequestURI(), "ERROR: Unable to get user information from request: " + e.getMessage()); @@ -55,7 +67,11 @@ public void logErrorMessage(Class clazz, String packageId, String message, HttpS @SuppressWarnings("rawtypes") public void logErrorMessage(Class clazz, User user, String packageId, String uri, String message) { Logger log = LoggerFactory.getLogger(clazz); - log.error(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); + if (user == null) { + log.error(LOG_MESSAGE_FORMAT, "Unknown user", packageId, uri, message); + } else { + log.error(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); + } } @SuppressWarnings("rawtypes") @@ -63,7 +79,11 @@ public void logWarnMessage(Class clazz, String packageId, String message, HttpSe Logger log = LoggerFactory.getLogger(clazz); try { User user = shibUserService.getUser(request); - log.warn(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); + if (user == null) { + log.warn(LOG_MESSAGE_FORMAT, "Unknown user", packageId, request.getRequestURI(), message); + } else { + log.warn(LOG_MESSAGE_FORMAT, user.toString(), packageId, request.getRequestURI(), message); + } } catch (UnsupportedEncodingException e) { log.error(LOG_MESSAGE_FORMAT, "unknown", packageId, request.getRequestURI(), "ERROR: Unable to get user information from request: " + e.getMessage()); @@ -73,7 +93,11 @@ public void logWarnMessage(Class clazz, String packageId, String message, HttpSe @SuppressWarnings("rawtypes") public void logWarnMessage(Class clazz, User user, String packageId, String uri, String message) { Logger log = LoggerFactory.getLogger(clazz); - log.warn(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); + if (user == null) { + log.warn(LOG_MESSAGE_FORMAT, "Unknown user", packageId, uri, message); + } else { + log.warn(LOG_MESSAGE_FORMAT, user.toString(), packageId, uri, message); + } } }