Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elisa/7492 public device endpoint #8341

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
package gov.cdc.usds.simplereport.api.devicetype;

import com.fasterxml.jackson.annotation.JsonView;
import gov.cdc.usds.simplereport.api.model.errors.DryRunException;
import gov.cdc.usds.simplereport.db.model.DeviceType;
import gov.cdc.usds.simplereport.service.DeviceTypeProdSyncService;
import gov.cdc.usds.simplereport.service.DeviceTypeService;
import gov.cdc.usds.simplereport.service.DeviceTypeSyncService;
import jakarta.servlet.http.HttpServletRequest;
import java.util.List;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
Expand All @@ -12,6 +21,8 @@
@Slf4j
public class DeviceTypeController {
@Autowired private DeviceTypeSyncService deviceTypeSyncService;
@Autowired private DeviceTypeProdSyncService deviceTypeProdSyncService;
@Autowired private DeviceTypeService deviceTypeService;
Comment on lines 23 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBD I may or may not combine this with the existing DeviceTypeSyncService or just rename this service to make it clear it is for the LIVD sync will depend on how the sync is implemented in part III

I like this idea! I got tripped up w/ these names


@GetMapping("/devices/sync")
public void syncDevices(@RequestParam boolean dryRun) {
Expand All @@ -21,4 +32,17 @@ public void syncDevices(@RequestParam boolean dryRun) {
log.info("Dry run");
}
}

@GetMapping("/devices")
@JsonView(PublicDeviceType.class)
public ResponseEntity<Object> getDevices(HttpServletRequest request) {
try {
String headerToken = request.getHeader("Sr-Prod-Devices-Token");
deviceTypeProdSyncService.validateToken(headerToken);
List<DeviceType> devices = deviceTypeService.fetchDeviceTypes();
return ResponseEntity.status(HttpStatus.OK).body(devices);
} catch (AccessDeniedException e) {
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(null);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package gov.cdc.usds.simplereport.api.devicetype;

public interface PublicDeviceType {}
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
.permitAll()
.requestMatchers(HttpMethod.GET, WebConfiguration.USER_ACCOUNT_REQUEST + "/**")
.permitAll()
// Devices endpoint authorization is handled at the service or controller level
.requestMatchers(HttpMethod.GET, WebConfiguration.DEVICES + "/**")
.permitAll()
// Anything else goes through Okta
.anyRequest()
.authenticated())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ public class WebConfiguration implements WebMvcConfigurer {
public static final String PATIENT_UPLOAD = "/upload/patients";
public static final String RESULT_UPLOAD = "/upload/results";
public static final String CONDITION_AGNOSTIC_RESULT_UPLOAD = "/upload/condition-agnostic";

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting nit - extra newline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh thank you! I'll update that in the part 3 PR 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo oops -- I'll remove this in my next PR 😅 Thank you for catching this 🎣

public static final String DEVICES = "/devices";
public static final String GRAPH_QL = "/graphql";

@Autowired private RestLoggingInterceptor _loggingInterceptor;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package gov.cdc.usds.simplereport.db.model;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonView;
import gov.cdc.usds.simplereport.api.devicetype.PublicDeviceType;
import jakarta.persistence.CascadeType;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
Expand All @@ -23,26 +24,31 @@
public class DeviceType extends EternalAuditedEntity {

@Column(nullable = false)
@JsonView(PublicDeviceType.class)
private String name;

@Column(nullable = false)
@JsonView(PublicDeviceType.class)
private String manufacturer;

@Column(nullable = false)
@JsonView(PublicDeviceType.class)
private String model;

@JoinTable(
name = "device_specimen_type",
joinColumns = @JoinColumn(name = "device_type_id"),
inverseJoinColumns = @JoinColumn(name = "specimen_type_id"))
@OneToMany(fetch = FetchType.LAZY)
@JsonView(PublicDeviceType.class)
private List<SpecimenType> swabTypes;

@Column(nullable = false)
@JsonView(PublicDeviceType.class)
private int testLength;

@JsonIgnore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emyl3 Have you looked into our concerns that removing these ignore tags will cause an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I apologize because I just now realized I never added that detail to the ticket:

#7492 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had previously deployed this branch both with and without the @JsonIgnore -- is this what you were thinking in terms of adding a timer and logging to see if there's a difference with and without the @JsonIgnore

This is with the JsonIgnore tag
Screenshot 2024-11-15 at 12 21 54

This is without
Screenshot 2024-11-15 at 13 40 56

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this is not what you had in mind and I can adjust the testing plan and retest 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me thinks that the issue is no longer relevant because the audit log was moved out of our db table. The other part of me thinks if it is somehow still an issue, we aren't going to see it until we send this to prod.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to @mehansen about this a bit and I loved her idea of just trying to replicate the original issue as a means of confirming that its no longer a concern. But again, the lack of an audit log in the db might make that a futile effort.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing with @DanielSass @mehansen and @mpbrown after standup, we will deploy this after the cert renewal.

Once merged in, I will keep an eye on the following:

If there are any slowdowns noticeable from Azure following the deploy, we will revert this PR and see if there are ways we can investigate this further.

In the meantime, I will look into if there's a way to view DB request times in Azure or check-in with Shanice after cert renewal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing the our "Azure Database for PostgreSQL - Flexible Server" view for our prod DB and there is a way to view both the Read and Write Throughput Bytes/Sec (Avg) -- Would this be sufficient? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shanice-skylight Are you familiar with any Azure tools to monitor avg DB request times? I saw there's this but we don't seem to have that enabled. 🤔

@OneToMany(mappedBy = "deviceTypeId", cascade = CascadeType.ALL, orphanRemoval = true)
@JsonView(PublicDeviceType.class)
List<DeviceTypeDisease> supportedDiseaseTestPerformed = new ArrayList<>();

protected DeviceType() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package gov.cdc.usds.simplereport.db.model;

import com.fasterxml.jackson.annotation.JsonView;
import gov.cdc.usds.simplereport.api.devicetype.PublicDeviceType;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.JoinColumn;
Expand All @@ -25,13 +27,33 @@ public class DeviceTypeDisease extends IdentifiedEntity {
@JoinColumn(name = "supported_disease_id")
private SupportedDisease supportedDisease;

@Column private String testPerformedLoincCode;
@Column private String testPerformedLoincLongName;
@Column private String equipmentUid;
@Column private String equipmentUidType;
@Column private String testkitNameId;
@Column private String testOrderedLoincCode;
@Column private String testOrderedLoincLongName;
@Column
@JsonView(PublicDeviceType.class)
private String testPerformedLoincCode;

@Column
@JsonView(PublicDeviceType.class)
private String testPerformedLoincLongName;

@Column
@JsonView(PublicDeviceType.class)
private String equipmentUid;

@Column
@JsonView(PublicDeviceType.class)
private String equipmentUidType;

@Column
@JsonView(PublicDeviceType.class)
private String testkitNameId;

@Column
@JsonView(PublicDeviceType.class)
private String testOrderedLoincCode;

@Column
@JsonView(PublicDeviceType.class)
private String testOrderedLoincLongName;

@Override
public boolean equals(Object o) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package gov.cdc.usds.simplereport.db.model;

import com.fasterxml.jackson.annotation.JsonView;
import gov.cdc.usds.simplereport.api.devicetype.PublicDeviceType;
import gov.cdc.usds.simplereport.validators.NumericCode;
import gov.cdc.usds.simplereport.validators.RequiredNumericCode;
import jakarta.persistence.Column;
Expand All @@ -12,16 +14,23 @@
public class SpecimenType extends EternalAuditedEntity {

@Column(nullable = false)
@JsonView(PublicDeviceType.class)
private String name;

@Column(nullable = false, updatable = false)
@RequiredNumericCode
@NaturalId
@JsonView(PublicDeviceType.class)
private String typeCode;

@Column private String collectionLocationName;
@Column
@JsonView(PublicDeviceType.class)
private String collectionLocationName;

@Column @NumericCode private String collectionLocationCode;
@Column
@NumericCode
@JsonView(PublicDeviceType.class)
private String collectionLocationCode;

protected SpecimenType() {} // for hibernate

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package gov.cdc.usds.simplereport.service;

import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

/** Service to fetch and save DeviceTypes from our prod env */
@Service
@Slf4j
@Transactional(readOnly = true)
public class DeviceTypeProdSyncService {
@Value("${simple-report.production.devices-token}")
private String token;

public boolean validateToken(String headerToken) throws AccessDeniedException {
if (token.equals(headerToken)) {
return true;
}
throw new AccessDeniedException("Access denied");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package gov.cdc.usds.simplereport.api;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;

import gov.cdc.usds.simplereport.service.DeviceTypeProdSyncService;
import gov.cdc.usds.simplereport.test_util.TestUserIdentities;
import org.json.JSONArray;
import org.json.JSONObject;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.http.MediaType;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.MvcResult;
import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder;

class DeviceTypeControllerTest extends BaseFullStackTest {

@Autowired private MockMvc _mockMvc;
@MockBean private DeviceTypeProdSyncService _mockDeviceTypeProdSyncService;

@BeforeEach
void init() {
TestUserIdentities.withStandardUser(
() -> {
_dataFactory.initGenericDeviceTypeAndSpecimenType();
});
}

@Test
void getDevices_withValidateToken_success() throws Exception {
when(_mockDeviceTypeProdSyncService.validateToken(any())).thenReturn(true);
MockHttpServletRequestBuilder builder =
get(ResourceLinks.DEVICES)
.contentType(MediaType.valueOf(MediaType.APPLICATION_JSON_VALUE))
.accept(MediaType.APPLICATION_JSON)
.characterEncoding("UTF-8");

MvcResult result = this._mockMvc.perform(builder).andReturn();
MockHttpServletResponse res = result.getResponse();

assertThat(res.getStatus()).isEqualTo(200);

JSONArray jsonRes = new JSONArray(res.getContentAsString());
assertThat(jsonRes.length()).isEqualTo(1);

JSONObject deviceType = jsonRes.getJSONObject(0);
assertThat(deviceType.getString("manufacturer")).isEqualTo("Acme");
assertThat(deviceType.getString("model")).isEqualTo("SFN");
assertThat(deviceType.getString("name")).isEqualTo("Acme SuperFine");
assertThat(deviceType.getInt("testLength")).isEqualTo(15);
assertThat(deviceType.getJSONArray("supportedDiseaseTestPerformed")).isEmpty();
// ensure deviceType internalId is not returned
assertTrue(deviceType.isNull("internalId"));

JSONArray swabTypes = deviceType.getJSONArray("swabTypes");
assertThat(swabTypes.length()).isEqualTo(1);
JSONObject swabType = swabTypes.getJSONObject(0);
assertThat(swabType.getString("collectionLocationCode")).isEqualTo("986543321");
assertThat(swabType.getString("collectionLocationName")).isEqualTo("Da Nose");
assertThat(swabType.getString("name")).isEqualTo("Nasal swab");
assertThat(swabType.getString("typeCode")).isEqualTo("000111222");
// ensure swabType internalId is not returned
assertTrue(swabType.isNull("internalId"));
}

@Test
void getDevices_withValidateToken_failure() throws Exception {
when(_mockDeviceTypeProdSyncService.validateToken(any()))
.thenThrow(new AccessDeniedException("Bad token"));
MockHttpServletRequestBuilder builder =
get(ResourceLinks.DEVICES)
.contentType(MediaType.valueOf(MediaType.APPLICATION_JSON_VALUE))
.accept(MediaType.APPLICATION_JSON)
.characterEncoding("UTF-8");

MvcResult result = this._mockMvc.perform(builder).andReturn();
MockHttpServletResponse res = result.getResponse();

assertThat(res.getStatus()).isEqualTo(401);
assertThat(res.getContentAsString()).isEmpty();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

/** Container class for test constants related to REST handler testing */
public final class ResourceLinks {
public static final String DEVICES = "/devices";
public static final String VERIFY_LINK_V2 = "/pxp/link/verify/v2";

public static final String SELF_REGISTER = "/pxp/register";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package gov.cdc.usds.simplereport.service;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.security.access.AccessDeniedException;

class DeviceTypeProdSyncServiceTest extends BaseServiceTest<DeviceTypeProdSyncService> {
@Value("${simple-report.production.devices-token}")
private String token;

@Test
void validateToken_withValidToken_success() {
assertThat(_service.validateToken(token)).isTrue();
}

@Test
void validateToken_withInvalidToken_throwsException() {
assertThrows(AccessDeniedException.class, () -> _service.validateToken("foo"));
}
}
Loading