From 8338187bc8169382039e35bb7382c19668a410ef Mon Sep 17 00:00:00 2001 From: ssettipalli Date: Tue, 1 Aug 2023 11:33:19 -0500 Subject: [PATCH 1/4] PEPPER-130 elastic upgrade (#2635) --- .../model/dashboard/BasicChartStrategy.java | 2 +- .../dsm/model/dashboard/CountStrategy.java | 2 +- .../elastic/mapping/FieldTypeExtractor.java | 10 +++--- .../model/elastic/search/ElasticSearch.java | 10 +++--- .../dsm/util/ElasticSearchUtil.java | 12 +++---- .../dashboard/MockMultiSearchResponse.java | 29 ----------------- ...rticalHighlightedBarChartStrategyTest.java | 32 ++++++++++++++++--- .../search/ESParticipantsResultReader.java | 3 +- .../ddp/export/DataExporter.java | 6 +--- .../ddp/util/ElasticsearchServiceUtil.java | 7 ++-- .../GetDsmParticipantStatusRouteTest.java | 6 ++-- pepper-apis/parent-pom.xml | 2 +- pepper-apis/pom.xml | 2 +- 13 files changed, 56 insertions(+), 67 deletions(-) delete mode 100644 pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/MockMultiSearchResponse.java diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/BasicChartStrategy.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/BasicChartStrategy.java index abc5173f17..c861a0bf00 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/BasicChartStrategy.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/BasicChartStrategy.java @@ -43,7 +43,7 @@ protected void fillLabelsValues() { MultiSearchResponse.Item response = msearch.getResponses()[i]; long count = 0; if (response != null && response.getResponse() != null) { - count = response.getResponse().getHits().getTotalHits(); + count = response.getResponse().getHits().getTotalHits().value; } fillData(values, labels, dashboardDto.getLabels().get(i), count); } diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/CountStrategy.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/CountStrategy.java index 39bb72e19c..322d2b83a0 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/CountStrategy.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/dashboard/CountStrategy.java @@ -21,7 +21,7 @@ public DashboardData get() { if (response != null && response.getResponse() != null) { return new CountData(dashboardDto.getDisplayType(), Collections.emptyList(), dashboardDto.getSize(), dashboardDto.getDisplayText(), dashboardDto.getOrder(), - response.getResponse().getHits().getTotalHits() + response.getResponse().getHits().getTotalHits().value ); } return null; diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/mapping/FieldTypeExtractor.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/mapping/FieldTypeExtractor.java index 935dfea8f0..a61551ce73 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/mapping/FieldTypeExtractor.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/mapping/FieldTypeExtractor.java @@ -24,9 +24,9 @@ public class FieldTypeExtractor implements TypeExtractor> { @Override public Map extract() { if (isFieldsNotCached()) { - Map mapping = getMapping().get(index); + Map mapping = getMapping().get(index); Map fieldTypeMapping = new HashMap<>(); - for (Map.Entry entry : mapping.entrySet()) { + for (Map.Entry entry : mapping.entrySet()) { fieldTypeMapping.put(getRightMostFieldName(entry.getKey()), extractType(entry.getKey(), entry.getValue())); } cachedFieldTypes.putAll(fieldTypeMapping); @@ -44,13 +44,13 @@ private List notCachedFields() { .collect(Collectors.toList()); } - private String extractType(String fullFieldName, GetFieldMappingsResponse.FieldMappingMetaData value) { + private String extractType(String fullFieldName, GetFieldMappingsResponse.FieldMappingMetadata value) { String key = getRightMostFieldName(fullFieldName); return (String) ((Map) value.sourceAsMap().get(key)).get(MappingGenerator.TYPE); } - private Map> getMapping() { - Map> result = new HashMap<>(); + private Map> getMapping() { + Map> result = new HashMap<>(); GetFieldMappingsRequest request = new GetFieldMappingsRequest(); request.indices(index); String[] fields = this.notCachedFields().toArray(new String[] {}); diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/search/ElasticSearch.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/search/ElasticSearch.java index ce9f083598..6baa360061 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/search/ElasticSearch.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/elastic/search/ElasticSearch.java @@ -175,7 +175,7 @@ public ElasticSearch getParticipantsWithinRange(String esParticipantsIndex, int } List esParticipants = parseSourceMaps(response.getHits().getHits()); logger.info("Got " + esParticipants.size() + " participants from ES for instance " + esParticipantsIndex); - return new ElasticSearch(esParticipants, response.getHits().getTotalHits()); + return new ElasticSearch(esParticipants, response.getHits().getTotalHits().value); } @Override @@ -199,7 +199,7 @@ public ElasticSearch getParticipantsByIds(String esIndex, List participa List esParticipants = parseSourceMaps(response.getHits().getHits()); logger.info("Got " + esParticipants.size() + " participants from ES for instance " + esIndex); - return new ElasticSearch(esParticipants, response.getHits().getTotalHits()); + return new ElasticSearch(esParticipants, response.getHits().getTotalHits().value); } @Override @@ -247,7 +247,7 @@ public ElasticSearch getParticipantsByRangeAndFilter(String esParticipantsIndex, } List esParticipants = parseSourceMaps(response.getHits().getHits()); logger.info("Got " + esParticipants.size() + " participants from ES for instance " + esParticipantsIndex); - return new ElasticSearch(esParticipants, response.getHits().getTotalHits()); + return new ElasticSearch(esParticipants, response.getHits().getTotalHits().value); } private AbstractQueryBuilder addOsteo2Filter(AbstractQueryBuilder queryBuilder) { @@ -287,7 +287,7 @@ public ElasticSearch getParticipantsByRangeAndIds(String participantIndexES, int } List esParticipants = parseSourceMaps(response.getHits().getHits()); logger.info("Got " + esParticipants.size() + " participants from ES for instance " + participantIndexES); - return new ElasticSearch(esParticipants, response.getHits().getTotalHits()); + return new ElasticSearch(esParticipants, response.getHits().getTotalHits().value); } @Override @@ -331,7 +331,7 @@ public ElasticSearch getAllParticipantsDataByInstanceIndex(String esParticipants } List elasticSearchParticipantDtos = parseSourceMaps(searchResponse.getHits().getHits()); logger.info("Got " + elasticSearchParticipantDtos.size() + " participants from ES for instance " + esParticipantsIndex); - return new ElasticSearch(elasticSearchParticipantDtos, searchResponse.getHits().getTotalHits()); + return new ElasticSearch(elasticSearchParticipantDtos, searchResponse.getHits().getTotalHits().value); } private BoolQueryBuilder getBoolQueryOfParticipantsId(List participantIds) { diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/ElasticSearchUtil.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/ElasticSearchUtil.java index 4fee955daf..44296f8236 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/ElasticSearchUtil.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/ElasticSearchUtil.java @@ -53,7 +53,7 @@ import org.elasticsearch.client.RestClientBuilder; import org.elasticsearch.client.RestHighLevelClient; import org.elasticsearch.client.indices.GetMappingsRequest; -import org.elasticsearch.cluster.metadata.MappingMetaData; +import org.elasticsearch.cluster.metadata.MappingMetadata; import org.elasticsearch.common.Strings; import org.elasticsearch.index.query.AbstractQueryBuilder; import org.elasticsearch.index.query.BoolQueryBuilder; @@ -130,7 +130,7 @@ public class ElasticSearchUtil { // These clients are expensive. They internally have thread pools and other resources. Let's // create one instance and reuse it as much as possible. Client is thread-safe per the docs. private static RestHighLevelClient client; - private static Map fieldMappings; + private static Map fieldMappings; static { initClient(); @@ -196,7 +196,7 @@ public static RestHighLevelClient getClientForElasticsearchCloud(@NonNull String httpClientBuilder.setDefaultIOReactorConfig(IOReactorConfig.custom().setSoKeepAlive(true).build()); } return httpClientBuilder; - }).setMaxRetryTimeoutMillis(100000); + }); return new RestHighLevelClient(builder); } @@ -350,7 +350,7 @@ public static ElasticSearchParticipantDto getElasticSearchForGivenMatch(String i response = client.search(searchRequest, RequestOptions.DEFAULT); response.getHits(); ElasticSearch elasticSearch = new ElasticSearch(); - return elasticSearch.parseSourceMap(response.getHits().getTotalHits() > 0 ? response.getHits().getAt(0).getSourceAsMap() : null) + return elasticSearch.parseSourceMap(response.getHits().getTotalHits().value > 0 ? response.getHits().getAt(0).getSourceAsMap() : null) .get(); } @@ -425,7 +425,7 @@ public static Map getParticipa try { response = client.search(searchRequest, RequestOptions.DEFAULT); - totalHits = response.getHits().getTotalHits(); + totalHits = response.getHits().getTotalHits().value; pageNumber++; } catch (IOException e) { throw new RuntimeException( @@ -757,7 +757,7 @@ public static Optional getParticipantProfileByGuidOrAltPid(String index SearchResponse response = client.search(searchRequest, RequestOptions.DEFAULT); Profile profile = null; - if (response.getHits().getTotalHits() > 0) { + if (response.getHits().getTotalHits().value > 0) { Map source = response.getHits().getAt(0).getSourceAsMap(); profile = new ElasticSearch().parseSourceMap(source).flatMap(ElasticSearchParticipantDto::getProfile).orElse(null); if (profile != null) { diff --git a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/MockMultiSearchResponse.java b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/MockMultiSearchResponse.java deleted file mode 100644 index 4c54704330..0000000000 --- a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/MockMultiSearchResponse.java +++ /dev/null @@ -1,29 +0,0 @@ -package org.broadinstitute.dsm.model.dashboard; - -import org.elasticsearch.action.search.MultiSearchResponse; -import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.search.SearchHits; - -class MockMultiSearchResponse extends MultiSearchResponse { - - public MockMultiSearchResponse() { - super(null); - } - - @Override - public Item[] getResponses() { - SearchResponse searchResponse = new SearchResponse() { - @Override - public SearchHits getHits() { - return new SearchHits(null, VerticalHighlightedBarChartStrategyTest.TOTAL_HITS, 0L); - } - }; - SearchResponse searchResponse2 = new SearchResponse() { - @Override - public SearchHits getHits() { - return new SearchHits(null, VerticalHighlightedBarChartStrategyTest.TOTAL_HITS2, 0L); - } - }; - return new Item[] {new Item(searchResponse, null), new Item(searchResponse2, null)}; - } -} diff --git a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/VerticalHighlightedBarChartStrategyTest.java b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/VerticalHighlightedBarChartStrategyTest.java index bb1dd07e66..b46aa3bcbb 100644 --- a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/VerticalHighlightedBarChartStrategyTest.java +++ b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/model/dashboard/VerticalHighlightedBarChartStrategyTest.java @@ -1,21 +1,46 @@ package org.broadinstitute.dsm.model.dashboard; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.IOException; import java.util.Arrays; import java.util.function.Supplier; +import org.apache.lucene.search.TotalHits; import org.broadinstitute.dsm.db.dto.dashboard.DashboardDto; import org.broadinstitute.dsm.db.dto.dashboard.DashboardLabelDto; +import org.elasticsearch.action.search.MultiSearchResponse; +import org.elasticsearch.action.search.MultiSearchResponse.Item; +import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.search.SearchHits; import org.junit.Assert; +import org.junit.BeforeClass; import org.junit.Test; public class VerticalHighlightedBarChartStrategyTest { public static final long TOTAL_HITS = 10; public static final long TOTAL_HITS2 = 15; + static MultiSearchResponse mockedSearchResponse = mock(MultiSearchResponse.class); + static SearchResponse mockSearchResponse1 = mock(SearchResponse.class); + static SearchResponse mockSearchResponse2 = mock(SearchResponse.class); + @BeforeClass + public static void setup() { + SearchHits searchHits = new SearchHits(null, new TotalHits(TOTAL_HITS, + TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO), 0L); + SearchHits searchHits2 = new SearchHits(null, new TotalHits(TOTAL_HITS2, + TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO), 0L); + when(mockSearchResponse1.getHits()).thenReturn(searchHits); + when(mockSearchResponse2.getHits()).thenReturn(searchHits2); + Item[] itemArray = new Item[] {new Item(mockSearchResponse1, null), + new Item(mockSearchResponse2, null)}; + when(mockedSearchResponse.getResponses()).thenReturn(itemArray); + } @Test - public void get() { + public void get() throws IOException { String darkIndigo = "dark indigo"; String yourCenter = "Your center"; DashboardLabelDto mainLabel = new DashboardLabelDto.Builder() @@ -30,8 +55,7 @@ public void get() { .withDisplayType(DisplayType.VERTICAL_HIGHLIGHTED_BAR_CHART) .withLabels(Arrays.asList(mainLabel, label2)) .build(); - MockMultiSearchResponse multiSearchResponse = new MockMultiSearchResponse(); - ChartStrategyPayload chartStrategyPayload = new ChartStrategyPayload(dashboardDto, multiSearchResponse); + ChartStrategyPayload chartStrategyPayload = new ChartStrategyPayload(dashboardDto, mockedSearchResponse); Supplier chartStrategy = new VerticalHighlightedBarChartStrategy(chartStrategyPayload); BarChartData dashboardData = (BarChartData) chartStrategy.get(); Assert.assertEquals(TOTAL_HITS, dashboardData.getValuesY().get(0)); @@ -39,7 +63,5 @@ public void get() { Assert.assertEquals(yourCenter, dashboardData.getValuesX().get(0)); Assert.assertEquals(darkIndigo, dashboardData.getColor().get(0)); } - - } diff --git a/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/elastic/participantslookup/search/ESParticipantsResultReader.java b/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/elastic/participantslookup/search/ESParticipantsResultReader.java index ef0cdc1c58..25c345e187 100644 --- a/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/elastic/participantslookup/search/ESParticipantsResultReader.java +++ b/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/elastic/participantslookup/search/ESParticipantsResultReader.java @@ -68,7 +68,8 @@ public Map readResults(SearchRes results.put(participantResult.getGuid(), participantResult); } - participantsLookupResult.setTotalCount(Long.valueOf(response.getHits().getTotalHits()).intValue()); + participantsLookupResult.setTotalCount((int) response.getHits().getTotalHits().value); + return results; } } diff --git a/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/export/DataExporter.java b/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/export/DataExporter.java index a97838a070..368d401e23 100644 --- a/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/export/DataExporter.java +++ b/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/export/DataExporter.java @@ -145,7 +145,7 @@ import org.elasticsearch.action.update.UpdateRequest; import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.RestHighLevelClient; -import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.xcontent.XContentType; import org.jdbi.v3.core.Handle; @Slf4j @@ -154,8 +154,6 @@ public class DataExporter { public static final DateTimeFormatter TIMESTAMP_FMT = DateTimeFormatter .ofPattern(TIMESTAMP_PATTERN).withZone(ZoneOffset.UTC); - private static final String REQUEST_TYPE = "_doc"; - // A cache for user auth0 emails, storing (auth0UserId -> email). private static final Map emailStore = new HashMap<>(); @@ -433,7 +431,6 @@ private void exportDataToElasticSearch(String index, Map data) t String esDoc = gson.toJson(value); UpdateRequest updateRequest = new UpdateRequest() .index(index) - .type(REQUEST_TYPE) .id(key) .doc(esDoc, XContentType.JSON) .docAsUpsert(true); @@ -637,7 +634,6 @@ private void convertInfoToJSONAndExportToES(Handle handle, participantRecords.forEach((key, value) -> { UpdateRequest updateRequest = new UpdateRequest() .index(index) - .type(REQUEST_TYPE) .id(key) .doc(value, XContentType.JSON) .docAsUpsert(true); diff --git a/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/util/ElasticsearchServiceUtil.java b/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/util/ElasticsearchServiceUtil.java index 24992b7077..218c2356a7 100644 --- a/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/util/ElasticsearchServiceUtil.java +++ b/pepper-apis/dss-core/src/main/java/org/broadinstitute/ddp/util/ElasticsearchServiceUtil.java @@ -84,7 +84,7 @@ public static synchronized RestHighLevelClient getElasticsearchClient(Config cfg } RestClientBuilder builder = RestClient.builder( - new HttpHost(url.getHost(), url.getPort(), url.getProtocol())) + new HttpHost(url.getHost(), url.getPort(), url.getProtocol())) .setHttpClientConfigCallback(httpClientBuilder -> { httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider); if (proxyUrl != null) { @@ -92,10 +92,9 @@ public static synchronized RestHighLevelClient getElasticsearchClient(Config cfg httpClientBuilder.setConnectionReuseStrategy(NoConnectionReuseStrategy.INSTANCE); } return httpClientBuilder; - }) - .setMaxRetryTimeoutMillis(100000); - + }); esClient = new RestHighLevelClient(builder); + ES_CLIENTS.put(key, esClient); log.info("Created new Elasticsearch client for URL: {}", url); diff --git a/pepper-apis/dss-core/src/test/java/org/broadinstitute/ddp/route/GetDsmParticipantStatusRouteTest.java b/pepper-apis/dss-core/src/test/java/org/broadinstitute/ddp/route/GetDsmParticipantStatusRouteTest.java index 18ddf2925f..dfb142bb3a 100644 --- a/pepper-apis/dss-core/src/test/java/org/broadinstitute/ddp/route/GetDsmParticipantStatusRouteTest.java +++ b/pepper-apis/dss-core/src/test/java/org/broadinstitute/ddp/route/GetDsmParticipantStatusRouteTest.java @@ -69,8 +69,8 @@ public void testProcess_returnStatusInfo() throws IOException { var expected = new ParticipantStatus(1577404800L, 1577491200L, 1582848000L, 1583798400L, 1583798400L, List.of( new ParticipantStatus.Sample("a", "BLOOD", 6L, 7L, 8L, "tracking9", "carrier10"))); - GetResponse getResponse = new GetResponse(new GetResult("index", "id", "id01", 0, - 1, 1, true, + GetResponse getResponse = new GetResponse(new GetResult("index", "id", "id01", 0L, + 1L, 1L, true, new BytesArray("{\"workflows\":[{\"workflow\":\"ACCEPTANCE_STATUS\",\"status\":\"ACCEPTED\"," + "\"data\":{\"subjectId\":\"XYZ\",\"name\":\"foobar\"}}]," + "\"samples\": [{\"trackingIn\": \"testtrackingIn\",\"kitType\": \"testType\",\"carrier\": " @@ -81,7 +81,7 @@ public void testProcess_returnStatusInfo() throws IOException { + ":[{\"requested\":\"2020-02-28\",\"histology\":\"testType\",\"datePx\":\"2020-02-28\"," + "\"received\":\"2020-03-10\",\"locationPx\":\"testLocation\",\"typePx\":\"testType\"," + "\"sent\":\"2020-02-29\",\"accessionNumber\":\"423423233232\",\"tissueRecordsId\":5729}]}}"), - null)); + null, null)); when(mockESClient.get(any(GetRequest.class), eq(RequestOptions.DEFAULT))) .thenReturn(getResponse); diff --git a/pepper-apis/parent-pom.xml b/pepper-apis/parent-pom.xml index e23bfce857..0d3d36d261 100644 --- a/pepper-apis/parent-pom.xml +++ b/pepper-apis/parent-pom.xml @@ -18,7 +18,7 @@ 7.1.4 3.0.3 4.8.0 - 6.8.15 + 7.17.10 1.4.5 2.12.1 diff --git a/pepper-apis/pom.xml b/pepper-apis/pom.xml index 2467006193..041ce3a157 100644 --- a/pepper-apis/pom.xml +++ b/pepper-apis/pom.xml @@ -22,7 +22,7 @@ 7.1.4 3.0.3 4.8.0 - 6.8.2 + 7.17.10 2.3 2.12.1 From d7e34d0a72bc6f353cea74af19249147c7bb908b Mon Sep 17 00:00:00 2001 From: denniscunningham <129115982+denniscunningham@users.noreply.github.com> Date: Tue, 1 Aug 2023 17:32:28 -0400 Subject: [PATCH 2/4] Finish role display text implementation (#2639) Fix user admin routes; finish role display text implementation --- .../java/org/broadinstitute/dsm/DSMServer.java | 4 ++-- .../dsm/service/admin/UserAdminService.java | 14 ++++++++------ .../PEPPER-972_access_role_display_update.xml | 14 ++++++++++++++ .../src/main/resources/master-changelog.xml | 1 + .../dsm/service/admin/UserAdminServiceTest.java | 6 ++++++ 5 files changed, 31 insertions(+), 8 deletions(-) create mode 100644 pepper-apis/dsm-core/src/main/resources/liquibase/PEPPER-972_access_role_display_update.xml diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java index d49500010f..5d274a72eb 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java @@ -930,11 +930,11 @@ private void setupAdminRoutes() { UserRoleRoute userRoleRoute = new UserRoleRoute(); get(uiRoot + RoutePath.USER_ROLE, userRoleRoute, new JsonTransformer()); post(uiRoot + RoutePath.USER_ROLE, userRoleRoute, new JsonTransformer()); - delete(uiRoot + RoutePath.USER_ROLE, userRoleRoute, new JsonTransformer()); + put(uiRoot + RoutePath.USER_ROLE, userRoleRoute, new JsonTransformer()); UserRoute userRoute = new UserRoute(); post(uiRoot + RoutePath.USER, userRoute, new JsonTransformer()); - delete(uiRoot + RoutePath.USER, userRoute, new JsonTransformer()); + put(uiRoot + RoutePath.USER, userRoute, new JsonTransformer()); } diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java index 79fcb6150d..ff7f07deef 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java @@ -41,12 +41,12 @@ public class UserAdminService { "SELECT dg.group_id FROM ddp_group dg WHERE dg.name = ?"; private static final String SQL_SELECT_STUDY_ROLES = - "SELECT ar.role_id, ar.name FROM access_role ar " + "SELECT ar.role_id, ar.name, ar.display_text FROM access_role ar " + "JOIN ddp_group_role dgr on ar.role_id = dgr.role_id " + "WHERE dgr.group_id = ?"; private static final String SQL_SELECT_STUDY_ROLES_FOR_ADMIN = - "SELECT ar.role_id, ar.name FROM access_role ar " + "SELECT ar.role_id, ar.name, ar.display_text FROM access_role ar " + "JOIN ddp_group_role dgr on ar.role_id = dgr.role_id " + "WHERE dgr.group_id = ? " + "AND dgr.admin_role_id = (select role_id from access_role ar where ar.name = ?)"; @@ -641,9 +641,10 @@ protected static Map getAllRolesForStudy(int groupId) { stmt.setInt(1, groupId); try (ResultSet rs = stmt.executeQuery()) { while (rs.next()) { - // TODO: temp until we have display text String name = rs.getString(2); - RoleInfo info = new RoleInfo(rs.getInt(1), name, name); + String displayText = rs.getString(3); + RoleInfo info = new RoleInfo(rs.getInt(1), name, + StringUtils.isBlank(displayText) ? name : displayText); roles.put(name, info); } } @@ -663,9 +664,10 @@ protected static Map getRolesForAdmin(int groupId, String admi stmt.setString(2, adminRole); try (ResultSet rs = stmt.executeQuery()) { while (rs.next()) { - // TODO: temp until we have display text String name = rs.getString(2); - RoleInfo info = new RoleInfo(rs.getInt(1), name, name); + String displayText = rs.getString(3); + RoleInfo info = new RoleInfo(rs.getInt(1), name, + StringUtils.isBlank(displayText) ? name : displayText); roles.put(name, info); } } diff --git a/pepper-apis/dsm-core/src/main/resources/liquibase/PEPPER-972_access_role_display_update.xml b/pepper-apis/dsm-core/src/main/resources/liquibase/PEPPER-972_access_role_display_update.xml new file mode 100644 index 0000000000..db9c8a8db1 --- /dev/null +++ b/pepper-apis/dsm-core/src/main/resources/liquibase/PEPPER-972_access_role_display_update.xml @@ -0,0 +1,14 @@ + + + + + + + name = 'mr_no_request_tissue' + + + diff --git a/pepper-apis/dsm-core/src/main/resources/master-changelog.xml b/pepper-apis/dsm-core/src/main/resources/master-changelog.xml index 2f0a2048f1..8b5569fde2 100644 --- a/pepper-apis/dsm-core/src/main/resources/master-changelog.xml +++ b/pepper-apis/dsm-core/src/main/resources/master-changelog.xml @@ -137,4 +137,5 @@ + diff --git a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java index c5f91138b2..ccc690ce00 100644 --- a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java +++ b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java @@ -267,6 +267,7 @@ public void testValidateRoles() { @Test public void testSetUserRoles() { String role1 = "upload_onc_history"; + String role1Display = "Onc history: Upload"; String role2 = "upload_ror_file"; List roles = List.of(role1, role2); Map rolesToId = getRoleIds(roles); @@ -323,6 +324,11 @@ public void testSetUserRoles() { List userInfoList = res.getUsers(); Map> userRoles = getUserRoles(userInfoList); + for (var userInfo: userInfoList) { + UserRole userRole = userInfo.getRoles().stream().filter(r -> + r.getName().equals(role1)).collect(Collectors.toList()).get(0); + Assert.assertEquals(role1Display, userRole.getDisplayText()); + } verifyResponseRoles(userRoles.get(user1), List.of(role1, role2), List.of(role1, role2)); verifyResponseRoles(userRoles.get(user2), List.of(role1, role2), List.of(role1, role2)); From a81bf1e1e2c4c4a961e637b7b4d0e35a8ae2871d Mon Sep 17 00:00:00 2001 From: denniscunningham <129115982+denniscunningham@users.noreply.github.com> Date: Tue, 1 Aug 2023 22:57:57 -0400 Subject: [PATCH 3/4] Remove obsolete DSM user create code from AuthenticationRoute (#2641) * WIP * WIP * WIP * Remove obsolete method --- .../org/broadinstitute/dsm/DSMServer.java | 3 +- .../dsm/route/AuthenticationRoute.java | 108 +++++++++--------- .../dsm/route/KitDiscardRoute.java | 3 +- .../dsm/security/Auth0Util.java | 15 ++- .../org/broadinstitute/dsm/util/UserUtil.java | 45 ++------ 5 files changed, 72 insertions(+), 102 deletions(-) diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java index 5d274a72eb..61baad40db 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/DSMServer.java @@ -647,7 +647,6 @@ protected void setupCustomRouting(@NonNull Config cfg) { setupDDPConfigurationLookup(cfg.getString(ApplicationConfigConstants.DDP)); AuthenticationRoute authenticationRoute = new AuthenticationRoute(auth0Util, - userUtil, cfg.getString(ApplicationConfigConstants.AUTH0_DOMAIN), cfg.getString(ApplicationConfigConstants.AUTH0_MGT_SECRET), cfg.getString(ApplicationConfigConstants.AUTH0_MGT_KEY), @@ -1060,6 +1059,8 @@ private void setupRouteGenericErrorHandlers() { response.body(exception.getMessage()); }); exception(DsmInternalError.class, (exception, request, response) -> { + logger.error("Internal error {}", exception.toString()); + exception.printStackTrace(); response.status(500); response.body(exception.getMessage()); }); diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/AuthenticationRoute.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/AuthenticationRoute.java index abc2a7da9e..31edf2e2b4 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/AuthenticationRoute.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/AuthenticationRoute.java @@ -8,8 +8,8 @@ import com.google.gson.Gson; import com.google.gson.JsonObject; +import com.google.gson.JsonParseException; import com.google.gson.JsonParser; -import com.google.gson.JsonSyntaxException; import lombok.NonNull; import org.apache.commons.lang3.StringUtils; import org.apache.http.entity.ContentType; @@ -17,8 +17,10 @@ import org.broadinstitute.dsm.db.dao.user.UserDao; import org.broadinstitute.dsm.db.dto.user.UserDto; import org.broadinstitute.dsm.exception.AuthenticationException; -import org.broadinstitute.dsm.util.UserUtil; +import org.broadinstitute.dsm.exception.DSMBadRequestException; +import org.broadinstitute.dsm.exception.DsmInternalError; import org.broadinstitute.dsm.security.Auth0Util; +import org.broadinstitute.dsm.util.UserUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import spark.Request; @@ -37,18 +39,16 @@ public class AuthenticationRoute implements Route { private final Auth0Util auth0Util; - private final UserUtil userUtil; private final String auth0Domain; private final String clientSecret; private final String auth0ClientId; private final String auth0MgmntAudience; private final String audienceNameSpace; - public AuthenticationRoute(@NonNull Auth0Util auth0Util, @NonNull UserUtil userUtil, @NonNull String auth0Domain, + public AuthenticationRoute(@NonNull Auth0Util auth0Util, @NonNull String auth0Domain, @NonNull String clientSecret, @NonNull String auth0ClientId, @NonNull String auth0MgmntAudience, @NonNull String audienceNameSpace) { this.auth0Util = auth0Util; - this.userUtil = userUtil; this.auth0Domain = auth0Domain; this.clientSecret = clientSecret; this.auth0ClientId = auth0ClientId; @@ -58,65 +58,59 @@ public AuthenticationRoute(@NonNull Auth0Util auth0Util, @NonNull UserUtil userU @Override public Object handle(Request request, Response response) { - logger.info("Check user..."); try { JsonObject jsonObject = JsonParser.parseString(request.body()).getAsJsonObject(); String auth0Token = jsonObject.get(payloadToken).getAsString(); - if (StringUtils.isNotBlank(auth0Token)) { - // checking if Auth0 knows that token - try { - Auth0Util.Auth0UserInfo auth0UserInfo = auth0Util.getAuth0UserInfo(auth0Token, auth0Domain); - if (auth0UserInfo != null) { - String email = auth0UserInfo.getEmail(); - logger.info("User (" + email + ") was found "); - Gson gson = new Gson(); - Map claims = new HashMap<>(); - UserDao userDao = new UserDao(); - UserDto userDto = - userDao.getUserByEmail(email).orElseThrow(() -> new RuntimeException("User " + email + " not found!")); - if (userDto == null) { - userUtil.insertUser(email, email); - userDto = userDao.getUserByEmail(email) - .orElseThrow(() -> new RuntimeException("new inserted user " + email + " not found!")); - claims.put(userAccessRoles, "user needs roles and groups"); - } else { - String userSetting = gson.toJson(userUtil.getUserAccessRoles(email), ArrayList.class); - claims.put(userAccessRoles, userSetting); - logger.info(userSetting); - claims.put(userSettings, gson.toJson(UserSettings.getUserSettings(email), UserSettings.class)); - } - claims.put(authUserId, String.valueOf(userDto.getId())); - claims.put(authUserName, userDto.getName().orElse("")); - claims.put(authUserEmail, email); - - try { - String dsmToken = auth0Util.getNewAuth0TokenWithCustomClaims(claims, clientSecret, auth0ClientId, auth0Domain, - auth0MgmntAudience, audienceNameSpace); - if (dsmToken != null) { - return new DSMToken(dsmToken); - } else { - haltWithErrorMsg(401, response, "DSMToken was null! Not authorized user"); - } - } catch (AuthenticationException e) { - haltWithErrorMsg(401, response, "DSMToken was null! Not authorized user", e); - } - } else { - haltWithErrorMsg(400, response, "user was null"); - } - } catch (AuthenticationException e) { - haltWithErrorMsg(400, response, "Problem getting user info from Auth0 token", e); - } - } else { - haltWithErrorMsg(400, response, "There was no token in the payload"); + if (StringUtils.isBlank(auth0Token)) { + haltWithErrorMsg(400, response, "There was no Auth0 token in the payload"); } - } catch (JsonSyntaxException e) { - haltWithErrorMsg(400, response, "The provided JSON in the request was malformed", e); + return new DSMToken(updateToken(auth0Token)); + } catch (AuthenticationException e) { + haltWithErrorMsg(400, response, "Unable to get user information from Auth0 token", e); + } catch (JsonParseException e) { + haltWithErrorMsg(400, response, "Unable to get Auth0 token from request", e); } + // DSMInternalError and DSMBadRequestException are handled via Spark return response; } + private String updateToken(String auth0Token) { + Auth0Util.Auth0UserInfo auth0UserInfo = auth0Util.getAuth0UserInfo(auth0Token, auth0Domain); + String email = auth0UserInfo.getEmail(); + + logger.info("Authenticating user {}", email); + UserDao userDao = new UserDao(); + UserDto userDto = userDao.getUserByEmail(email).orElseThrow(() -> + new DSMBadRequestException("User not found: " + email)); + + Map claims = updateClaims(userDto); + String dsmToken = auth0Util.getNewAuth0TokenWithCustomClaims(claims, clientSecret, auth0ClientId, auth0Domain, + auth0MgmntAudience, audienceNameSpace); + if (dsmToken == null) { + throw new DsmInternalError("Assert: Auth token should not be null"); + } + return dsmToken; + } + + private Map updateClaims(UserDto userDto) { + Map claims = new HashMap<>(); + try { + Gson gson = new Gson(); + String email = userDto.getEmail().orElseThrow(() -> new DsmInternalError("User email cannot be null")); + String roles = gson.toJson(UserUtil.getUserAccessRoles(email), ArrayList.class); + claims.put(userAccessRoles, roles); + claims.put(userSettings, gson.toJson(UserSettings.getUserSettings(email), UserSettings.class)); + claims.put(authUserId, String.valueOf(userDto.getId())); + claims.put(authUserName, userDto.getName().orElse("")); + claims.put(authUserEmail, email); + } catch (JsonParseException e) { + throw new DsmInternalError("Error converting class to JSON", e); + } + return claims; + } + private static class DSMToken { - private String dsmToken; + private final String dsmToken; public DSMToken(String token) { this.dsmToken = token; @@ -128,6 +122,8 @@ public DSMToken(String token) { */ public static void haltWithErrorMsg(int responseStatus, Response response, String message) { response.type(ContentType.APPLICATION_JSON.getMimeType()); + // TODO: this is currently called for bad request status. Do we want to log that at error level? + // Or perhaps we could use the return status to determine the log level? -DC logger.error(message); String errorMsgJson = new Gson().toJson(new Error(message)); halt(responseStatus, errorMsgJson); @@ -135,6 +131,8 @@ public static void haltWithErrorMsg(int responseStatus, Response response, Strin public static void haltWithErrorMsg(int responseStatus, Response response, String message, Throwable t) { if (t != null) { + // TODO: this is currently called for bad request status. Do we want to log that at error level? + // Or perhaps we could use the return status to determine the log level? -DC logger.error("Authentication Error", t); } haltWithErrorMsg(responseStatus, response, message); diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/KitDiscardRoute.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/KitDiscardRoute.java index b5c65e4317..9472a21805 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/KitDiscardRoute.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/route/KitDiscardRoute.java @@ -4,6 +4,7 @@ import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.util.ArrayList; +import java.util.List; import com.google.gson.Gson; import lombok.NonNull; @@ -129,7 +130,7 @@ public Object processRequest(Request request, Response response, String userId) String email = auth0UserInfo.getEmail(); UserDto userDto = new UserDao().getUserByEmail(email).orElseThrow(); if (userDto != null && userDto.getId() > 0) { - ArrayList userSetting = userUtil.getUserAccessRoles(email); + List userSetting = userUtil.getUserAccessRoles(email); if (userSetting.contains(DBConstants.KIT_SHIPPING) || userSetting.contains(DBConstants.DISCARD_SAMPLE)) { KitDiscard kit = KitDiscard.getKitDiscard(kitAction.getKitDiscardId()); if (kit.getChangedById() != userDto.getId()) { diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/security/Auth0Util.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/security/Auth0Util.java index 599809c46d..a99e25a090 100755 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/security/Auth0Util.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/security/Auth0Util.java @@ -29,6 +29,8 @@ import org.apache.http.NameValuePair; import org.apache.http.message.BasicNameValuePair; import org.broadinstitute.dsm.exception.AuthenticationException; +import org.broadinstitute.dsm.exception.DSMBadRequestException; +import org.broadinstitute.dsm.exception.DsmInternalError; import org.broadinstitute.dsm.model.auth0.Auth0M2MResponse; import org.broadinstitute.dsm.util.DDPRequestUtil; import org.slf4j.Logger; @@ -70,8 +72,8 @@ public Auth0UserInfo getAuth0UserInfo(@NonNull String idToken, String auth0Domai verifyUserConnection(auth0Claims.get("sub").asString(), userInfo.getEmail()); return userInfo; - } catch (AuthenticationException e) { - throw new AuthenticationException("couldn't get Auth0 user info", e); + } catch (Exception e) { + throw new AuthenticationException("Could not get Auth0 user info", e); } } @@ -104,7 +106,7 @@ private String findUserConnection(List list) { } if (connection == null) { - throw new RuntimeException("User does not have an approved connection."); + throw new DSMBadRequestException("User does not have an approved connection."); } return connection; } @@ -116,20 +118,17 @@ private void verifyUserConnection(@NonNull String userId, @NonNull String email) User user = userRequest.execute(); findUserConnection(user.getIdentities()); } catch (Exception ex) { - throw new RuntimeException("User connection verification failed for user " + email, ex); + throw new DsmInternalError("User connection verification failed for user " + email, ex); } } public static Map verifyAndParseAuth0TokenClaims(String auth0Token, String auth0Domain) throws AuthenticationException { - Map auth0Claims = new HashMap<>(); try { Optional maybeToken = verifyAuth0Token(auth0Token, auth0Domain); - maybeToken.orElseThrow(); - auth0Claims = maybeToken.get().getClaims(); + return maybeToken.orElseThrow().getClaims(); } catch (Exception e) { throw new AuthenticationException("Could not verify auth0 token.", e); } - return auth0Claims; } /** diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/UserUtil.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/UserUtil.java index 14d0d42d53..3e27b1d0d4 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/UserUtil.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/UserUtil.java @@ -6,7 +6,6 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; -import java.sql.Statement; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -15,9 +14,9 @@ import lombok.NonNull; import org.apache.commons.lang3.StringUtils; -import org.broadinstitute.dsm.db.UserSettings; import org.broadinstitute.dsm.db.dao.user.UserDao; import org.broadinstitute.dsm.db.dto.user.UserDto; +import org.broadinstitute.dsm.exception.DsmInternalError; import org.broadinstitute.dsm.model.NameValue; import org.broadinstitute.dsm.model.patch.Patch; import org.broadinstitute.dsm.statics.ApplicationConfigConstants; @@ -40,7 +39,6 @@ public class UserUtil { public static final String SHIPPING_MENU = "shipping"; private static final Logger logger = LoggerFactory.getLogger(UserUtil.class); private static final String SQL_SELECT_USER = "SELECT user_id, name FROM access_user"; - private static final String SQL_INSERT_USER = "INSERT INTO access_user (name, email) VALUES (?,?)"; private static final String SQL_SELECT_USER_ACCESS_ROLE = "SELECT role.name FROM access_user_role_group roleGroup, access_user user, access_role role " + "WHERE roleGroup.user_id = user.user_id AND roleGroup.role_id = role.role_id AND user.is_active = 1"; @@ -283,11 +281,13 @@ public static boolean checkUserAccessForPatch(String realm, String userId, Strin } else { // for now, let's do what DSM did previously and let them change the data. // Still we need to log this and fix the patch from frontend - logger.error("The id in patch is not a number and also not an email, id in patch is "+ userEmailOrIdInPatch + "and id in token is "+ userId); + logger.error("The id in patch is not a number and also not an email, id in patch is " + userEmailOrIdInPatch + + "and id in token is " + userId); return checkUserAccess(realm, userId, role, userIdRequest); } - if (!userId.equals(userIdFromPatch)){ - String msg = "User id in patch did not match the one in token, user Id in patch is " + userIdFromPatch + " user Id in token " + userIdRequest; + if (!userId.equals(userIdFromPatch)) { + String msg = "User id in patch did not match the one in token, user Id in patch is " + userIdFromPatch + " user Id in token " + + userIdRequest; logger.warn(msg); throw new RuntimeException(msg); } @@ -329,7 +329,7 @@ public static boolean checkKitShippingAccessForPatch(String realm, String userId && DBConstants.DDP_KIT_ALIAS.equals(patch.getTableAlias()); } - public ArrayList getUserAccessRoles(@NonNull String email) { + public static List getUserAccessRoles(@NonNull String email) { ArrayList roles = new ArrayList<>(); SimpleResult results = inTransaction((conn) -> { SimpleResult dbVals = new SimpleResult(); @@ -347,37 +347,8 @@ public ArrayList getUserAccessRoles(@NonNull String email) { }); if (results.resultException != null) { - throw new RuntimeException("Error getting list of roles ", results.resultException); + throw new DsmInternalError("Error getting roles for " + email, results.resultException); } return roles; } - - public int insertUser(@NonNull String name, @NonNull String email) { - SimpleResult results = inTransaction((conn) -> { - SimpleResult dbVals = new SimpleResult(); - try (PreparedStatement insertStmt = conn.prepareStatement(SQL_INSERT_USER, Statement.RETURN_GENERATED_KEYS)) { - insertStmt.setString(1, name); - insertStmt.setString(2, email); - insertStmt.executeUpdate(); - try (ResultSet rs = insertStmt.getGeneratedKeys()) { - if (rs.next()) { - UserSettings.insertUserSetting(conn, rs.getInt(1)); - dbVals.resultValue = rs.getInt(1); - } - } catch (Exception e) { - throw new RuntimeException("Error getting id of new kit request ", e); - } - } catch (SQLException ex) { - logger.error( - "User " + name + ", " + email + " already exists but doesn't have any access roles or is set to is_active=0..."); - } - return dbVals; - }); - - if (results.resultException != null) { - throw new RuntimeException("Error getting list of realms ", results.resultException); - } - - return (int) results.resultValue; - } } From d126c98fa035713fe3a439fcbc22e946099f6975 Mon Sep 17 00:00:00 2001 From: denniscunningham <129115982+denniscunningham@users.noreply.github.com> Date: Wed, 2 Aug 2023 22:22:00 -0400 Subject: [PATCH 4/4] Support email validation and email case-insensitive matching for user admin (#2642) * Support email validation and email case-insensitive matching * One more email comparison case --- pepper-apis/dsm-core/pom.xml | 5 ++ .../dsm/db/dao/user/UserDao.java | 4 +- .../dsm/service/admin/UserAdminService.java | 66 +++++++++++-------- .../dsm/service/admin/UserRequest.java | 2 +- .../service/admin/UserAdminServiceTest.java | 62 +++++++++++++---- 5 files changed, 98 insertions(+), 41 deletions(-) diff --git a/pepper-apis/dsm-core/pom.xml b/pepper-apis/dsm-core/pom.xml index 6dddcf81c2..3341425567 100644 --- a/pepper-apis/dsm-core/pom.xml +++ b/pepper-apis/dsm-core/pom.xml @@ -57,6 +57,11 @@ + + commons-validator + commons-validator + + org.apache.commons commons-dbcp2 diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java index 2007f0974d..4f40736e79 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/user/UserDao.java @@ -29,7 +29,7 @@ public class UserDao implements Dao { private static final String SQL_DELETE_USER_BY_ID = "DELETE FROM access_user WHERE user_id = ?"; private static final String SQL_SELECT_USER_BY_EMAIL = "SELECT user.user_id, user.name, user.email, user.phone_number, user.is_active FROM access_user user " - + "WHERE user.email = ?"; + + "WHERE UPPER(user.email) = ?"; private static final String SQL_SELECT_USER_BY_ID = "SELECT user.user_id, user.name, user.email, user.phone_number, user.is_active FROM access_user user " + "WHERE user.user_id = ?"; @@ -38,7 +38,7 @@ public Optional getUserByEmail(@NonNull String email) { SimpleResult results = inTransaction((conn) -> { SimpleResult dbVals = new SimpleResult(); try (PreparedStatement stmt = conn.prepareStatement(SQL_SELECT_USER_BY_EMAIL)) { - stmt.setString(1, email); + stmt.setString(1, email.toUpperCase()); try (ResultSet rs = stmt.executeQuery()) { if (rs.next()) { dbVals.resultValue = new UserDto(rs.getInt(USER_ID), diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java index ff7f07deef..c66ce34040 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserAdminService.java @@ -20,6 +20,7 @@ import lombok.Data; import lombok.extern.slf4j.Slf4j; import org.apache.commons.collections4.CollectionUtils; +import org.apache.commons.validator.routines.EmailValidator; import org.broadinstitute.dsm.db.UserSettings; import org.broadinstitute.dsm.db.dao.user.UserDao; import org.broadinstitute.dsm.db.dto.user.UserDto; @@ -265,8 +266,9 @@ protected Map validateUsers(List users, int groupId) { throw new DSMBadRequestException("Invalid users: empty"); } Map userIds = new HashMap<>(); - for (String email: users) { - userIds.put(email, verifyUserByEmail(email, groupId).getId()); + for (String user: users) { + UserDto userDto = verifyUserByEmail(user, groupId); + userIds.put(userDto.getEmailOrThrow(), userDto.getId()); } return userIds; } @@ -288,19 +290,33 @@ protected static String validateEmailRequest(String email) { if (StringUtils.isBlank(email)) { throw new DSMBadRequestException("Invalid user email: blank"); } - return email; + return email.trim(); } public void addAndRemoveUsers(UserRequest req) { + int groupId = verifyStudyGroup(studyGroup); + Map studyRoles = getAdminRoles(groupId); + List removeUsers = req.getRemoveUsers(); List addUsers = req.getAddUsers(); boolean hasAddUsers = !CollectionUtils.isEmpty(addUsers); boolean hasRemoveUsers = !CollectionUtils.isEmpty(removeUsers); + // verify request data + Map emailToAddUser = null; + if (hasAddUsers) { + emailToAddUser = verifyAddUser(addUsers, groupId, studyRoles.keySet()); + } + + List removeUserDto = new ArrayList<>(); if (hasRemoveUsers) { + for (String user: removeUsers) { + removeUserDto.add(verifyUserByEmail(user, groupId)); + } + // ensure no union of add and remove users - if (hasAddUsers && CollectionUtils.containsAny(removeUsers, - addUsers.stream().map(UserRequest.User::getEmail).collect(Collectors.toList()))) { + if (hasAddUsers && CollectionUtils.containsAny(emailToAddUser.keySet(), + removeUserDto.stream().map(UserDto::getEmailOrThrow).collect(Collectors.toList()))) { throw new DSMBadRequestException("Invalid user request: Cannot add and remove the same user"); } } else if (!hasAddUsers) { @@ -308,24 +324,19 @@ public void addAndRemoveUsers(UserRequest req) { } if (hasRemoveUsers) { - removeUser(removeUsers); + removeUser(removeUserDto); } if (hasAddUsers) { - addUser(addUsers); + addUser(addUsers, emailToAddUser, groupId, studyRoles); } } /** * Add user to study group. User request must include at least one role */ - protected void addUser(List users) { - int groupId = verifyStudyGroup(studyGroup); - Map studyRoles = getAdminRoles(groupId); - - // pre-check to lessen likelihood of partial operation - Map emailToUser = verifyAddUser(users, groupId, studyRoles.keySet()); - + protected void addUser(List users, Map emailToUser, int groupId, + Map studyRoles) { UserDao userDao = new UserDao(); for (var user: users) { UserDto userDto = emailToUser.get(user.getEmail()); @@ -333,7 +344,7 @@ protected void addUser(List users) { boolean hasUserSettings = false; if (userId != 0) { UserDao.update(userId, userDto); - hasUserSettings = UserSettings.getUserSettings(user.getEmail()) != null; + hasUserSettings = UserSettings.getUserSettings(userDto.getEmailOrThrow()) != null; } else { userId = userDao.create(userDto); } @@ -348,6 +359,9 @@ protected Map verifyAddUser(List users, int g Map emailToUser = new HashMap<>(); for (var user: users) { String email = validateEmailRequest(user.getEmail()); + validateEmailFormat(email); + // update email in request since we use it as a key + user.setEmail(email); UserDto userDto = getUserByEmail(email, groupId); if (userDto != null) { @@ -415,15 +429,7 @@ public void updateUser(UpdateUserRequest req) { /** * Remove one or more users and their associated roles */ - protected void removeUser(List users) { - int groupId = validateOperatorAdmin(); - - // pre-check all users to decrease likelihood of incomplete operation - List userDto = new ArrayList<>(); - for (String email: users) { - userDto.add(verifyUserByEmail(email, groupId)); - } - + protected void removeUser(List userDto) { for (UserDto user: userDto) { deleteUserRoles(user.getId()); user.setIsActive(0); @@ -476,11 +482,11 @@ protected static Map getStudyUsers(int groupId, UserRoleReque return allStudyUsers; } - Map emailToId = allStudyUsers.entrySet().stream().collect(Collectors.toMap(e -> e.getValue().getEmail(), - Map.Entry::getKey)); + Map emailToId = allStudyUsers.entrySet().stream().collect(Collectors.toMap(e -> + e.getValue().getEmail().toUpperCase(), Map.Entry::getKey)); Map users = new HashMap<>(); for (String email: req.getUsers()) { - Integer id = emailToId.get(email); + Integer id = emailToId.get(email.trim().toUpperCase()); if (id != null) { users.put(id, allStudyUsers.get(id)); } else { @@ -728,6 +734,12 @@ protected static UserDto verifyUserByEmail(String email, int groupId) { return user; } + protected static void validateEmailFormat(String email) { + if (!EmailValidator.getInstance().isValid(email)) { + throw new DSMBadRequestException("Invalid email address format: " + email); + } + } + protected static UserDto getUserByEmail(String email, int groupId) { // TODO: Currently we do not track users for a group, but get by groupId once we do -DC UserDao userDao = new UserDao(); diff --git a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java index 7bd6fde7f2..6389dad548 100644 --- a/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java +++ b/pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/admin/UserRequest.java @@ -17,7 +17,7 @@ public class UserRequest { @Data public static class User { - private final String email; + private String email; private final String name; private final String phone; private final List roles; diff --git a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java index ccc690ce00..eae9f44db3 100644 --- a/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java +++ b/pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminServiceTest.java @@ -500,10 +500,20 @@ public void testAddAndRemoveUser() { String user = "test_user6@study.org"; String userName = "test_user6"; + + UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); + UserRequest badUserRequest = new UserRequest(List.of(new UserRequest.User(user, userName, null, + List.of("bad_role"))), null); + try { + service.addAndRemoveUsers(badUserRequest); + Assert.fail("UserAdminService.addUser should fail with bad role names"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid roles for study group")); + } + UserRequest userRequest = new UserRequest(List.of(new UserRequest.User(user, userName, null, List.of(role1))), null); - UserAdminService service = new UserAdminService(Integer.toString(operatorId), TEST_GROUP); try { service.addAndRemoveUsers(userRequest); } catch (Exception e) { @@ -564,12 +574,12 @@ public void testAddAndRemoveUser() { try { service.addAndRemoveUsers(removeReq); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } try { UserAdminService.verifyUserByEmail(user, groupId); - Assert.fail("UserAdminService.removeUser failed to remove user"); + Assert.fail("UserAdminService.addAndRemoveUsers failed to remove user"); } catch (Exception e) { Assert.assertTrue(e.getMessage().contains("Invalid user")); } @@ -598,6 +608,7 @@ public void testAddExistingUser() { int operatorId = setupAdmin("test_admin5@study.org", new ArrayList<>(rolesToId.values()), groupId); String user = "test_user7@study.org"; + String userVariation = "Test_User7@study.org"; String userName = "test_user7"; UserRequest addUserRequest = new UserRequest(List.of(new UserRequest.User(user, userName, null, roles)), null); @@ -606,27 +617,27 @@ public void testAddExistingUser() { try { service.addAndRemoveUsers(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } UserRequest removeReq = new UserRequest(null, List.of(user)); try { service.addAndRemoveUsers(removeReq); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } // add user back to test the inactive to active transition try { service.addAndRemoveUsers(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } // try to add again try { service.addAndRemoveUsers(addUserRequest); - Assert.fail("UserAdminService.addUser should fail to add an existing study user"); + Assert.fail("UserAdminService.addAndRemoveUsers should fail to add an existing study user"); } catch (Exception e) { Assert.assertTrue(e.getMessage().contains("Already has roles in study")); } @@ -640,14 +651,22 @@ public void testAddExistingUser() { try { service.addAndRemoveUsers(addUserRequest); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.addUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); } - // cleanup + // cleanup (with case variation on email address) + UserRequest removeReq2 = new UserRequest(null, List.of(userVariation)); try { - service.addAndRemoveUsers(removeReq); + service.addAndRemoveUsers(removeReq2); } catch (Exception e) { - Assert.fail("Exception from UserAdminService.removeUser: " + getStackTrace(e)); + Assert.fail("Exception from UserAdminService.addAndRemoveUsers: " + getStackTrace(e)); + } + + try { + service.addAndRemoveUsers(removeReq2); + Assert.fail("UserAdminService.addAndRemoveUsers should fail to a remove study user that does not exist"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid user for study group")); } } @@ -693,6 +712,27 @@ public void testGetStudyGroup() { } } + @Test + public void testValidateEmailFormat() { + try { + UserAdminService.validateEmailFormat("joe@gmail.com"); + } catch (Exception e) { + Assert.fail("Exception from UserAdminService.validateEmailFormat: " + e.toString()); + } + try { + UserAdminService.validateEmailFormat("justjoe"); + Assert.fail("UserAdminService.validateEmailFormat should fail with bad email address: justjoe"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid email address format")); + } + try { + UserAdminService.validateEmailFormat("joe@gmail"); + Assert.fail("UserAdminService.validateEmailFormat should fail with bad email address: joe@gmail"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Invalid email address format")); + } + } + private int createAdminUser(String email, int adminRoleId) { int userId = createTestUser(email, adminRoleId); int groupId = UserAdminService.verifyStudyGroup(TEST_GROUP);