Audit Log Service - /v1/events endpoint implementation #540

dhanyak-btc · 2020-06-22T14:07:15Z

Moved HealthController class to common-service module
Used everit-org JSON Schema validator (Apache 2.0 license) for event request validation.
Used JSoup (MIT license) to extract the error response details from RestClientResponseException thrown by tomcat server
Added Spring Boot Actuator to view the log file when deployed to DEV/QA environment and the mapping details. This will help during deployment and while integrating the APIs.
Implemented /v1/events endpoint
Added token introspection filter and common exception handler for filters in common-service.

Moved filters and health controller to common-service, added actuator, added JSoup to extract error message from Html etc

convert xpath to json path

minor refactoring and removed unused getEncodedAuthorization() from BaseServiceImpl

Added getUriTemplateAndHttpMethodsMap() abstract method to BaseTokenIntrospectionFilter

Removed unused method: getEventInfo() from AuditLogEventEntity

...va/com/google/cloud/healthcare/fdamystudies/auditlog/controller/AuditLogEventController.java

...java/com/google/cloud/healthcare/fdamystudies/auditlog/validator/AuditLogEventValidator.java

zohrehj · 2020-06-22T14:56:04Z

audit-log-module/audit-log-service/src/main/resources/audit-log-event-schema.json

+	"properties": {
+		"alert": {
+			"type": "boolean"
+		},


what is alert used for? and why is it required.

This stores the value of Alert flag. I'll request Shanthala to connect with you to discuss on Audit Log Event fields & requirements.

This file no longer in use. Replaced json schema with validation annotations.

audit-log-module/audit-log-service/src/main/resources/audit-log-event-schema.json

audit-log-module/audit-log-service/src/main/resources/application-local.properties

.../src/main/java/com/google/cloud/healthcare/fdamystudies/filter/TokenIntrospectionFilter.java

audit-log-module/audit-log-service/audit-log-service.postman_collection.json

.../java/com/google/cloud/healthcare/fdamystudies/auditlog/filter/TokenIntrospectionFilter.java

Fixed PR#540 comments

nikklassen

This PR could've been split into OAuth code, followed by AuditLog code, to make it easier to review.

...java/com/google/cloud/healthcare/fdamystudies/auditlog/validator/AuditLogEventValidator.java

...ain/java/com/google/cloud/healthcare/fdamystudies/auditlog/service/AuditLogEventService.java

...java/com/google/cloud/healthcare/fdamystudies/auditlog/service/AuditLogEventServiceImpl.java

...c/main/java/com/google/cloud/healthcare/fdamystudies/auditlog/model/AuditLogEventEntity.java

.../main/java/com/google/cloud/healthcare/fdamystudies/filter/BaseTokenIntrospectionFilter.java

...c/main/java/com/google/cloud/healthcare/fdamystudies/filter/FilterChainExceptionHandler.java

nikklassen · 2020-06-22T18:53:12Z

...-service/src/main/java/com/google/cloud/healthcare/fdamystudies/service/BaseServiceImpl.java

-      ErrorResponse err = new ErrorResponse(url, e);
-      return ResponseEntity.status(e.getRawStatusCode()).body(err);
-    }
+    return restTemplate.exchange(url, method, requestEntity, JsonNode.class, uriVariables);


avoid wrapping one-liners.

I agree with you. Initially this method had try..catch block but now exception handling moved to RestExceptionHandler and FilterChainExceptionHandler so this method became one-liners.

In that case that mean you don't need BaseServiceImpl anymore. The AuditLogEventService isn't doing anything with the methods anyways.

I agree with you, but I've already implemented the Service classes using these methods. We'll keep this class.

...-service/src/main/java/com/google/cloud/healthcare/fdamystudies/service/BaseServiceImpl.java

...vice/src/main/java/com/google/cloud/healthcare/fdamystudies/controller/HealthController.java

Fixed PR review comments

...vice/src/main/java/com/google/cloud/healthcare/fdamystudies/common/RestExceptionHandler.java

Removed JSoup, Json Schema validator dependencies. Added server-side validation using validation annotations, Added ErrorController, GlobalExceptionHandler, ValidationErrorResponse and AuditLogEventRequest to common-service module Added jsonassert dependency to assert json values.

Added logger statements to GlobalExceptionHandler

Moved AuditLogEventResponse to common-service module

Deleted postman collection

Added @ToString.Exclude to entity class

Removed params from logger.entry

audit-log-module/audit-log-service/src/main/resources/application-local.properties

nikklassen · 2020-06-24T17:39:14Z

Fixes #551

nikklassen

Looks good. Just a few small things.

.../main/java/com/google/cloud/healthcare/fdamystudies/auditlog/mapper/AuditLogEventMapper.java

...om/google/cloud/healthcare/fdamystudies/auditlog/controller/AuditLogEventControllerTest.java

...rc/main/java/com/google/cloud/healthcare/fdamystudies/exceptions/GlobalExceptionHandler.java

Fixed Imports and Removed beans, request/response from logger statements

Changed the column definition to TIMESTAMP for eventTimestamp and createdTimestamp in AuditLogEventEntity class. Added javdocs to eventTimestamp in AuditLogEventRequest

@SuppressWarnings

Removed @SuppressWarnings("rawtypes") Changed log level to trace in handleConstraintValidationException() method

zohrehj

Looks good, added some suggestions; some need to be addressed at some point but nothing immediately concerning.

zohrehj · 2020-06-25T20:31:49Z

...c/main/java/com/google/cloud/healthcare/fdamystudies/auditlog/model/AuditLogEventEntity.java

+      insertable = false,
+      updatable = false,
+      columnDefinition = "TIMESTAMP DEFAULT CURRENT_TIMESTAMP")
+  private Timestamp createdTimestamp;


Google API guideline suggests calling these timestamp columns simply:
created, modified etc

This link has different naming convention. Please share the Google API guideline link to refer. Renamed both column and property name to 'created', it'll be pushed for review in next PR.

zohrehj · 2020-06-25T20:32:04Z

...c/main/java/com/google/cloud/healthcare/fdamystudies/auditlog/model/AuditLogEventEntity.java

+      nullable = false,
+      updatable = false,
+      columnDefinition = "TIMESTAMP")
+  private Timestamp eventTimestamp;


could be renamed to occured maybe?

Renamed both column and property name to 'occured', it'll be pushed for review in next PR.

zohrehj · 2020-06-25T20:37:14Z

...service/src/test/java/com/google/cloud/healthcare/fdamystudies/auditlog/ApplicationTest.java


 class ApplicationTest extends BaseMockIT {

-  @Autowired HealthController controller;
+  @Autowired AuditLogEventController controller;


please add back the healthcheck test as well. It should assert that the response status is 200

I've added assertNotNull(healthController); in this Test. There is already a HealthControllerTest that assert response status is 200. This will be pushed for review in next PR.

zohrehj · 2020-06-25T20:49:00Z

...vice/src/main/java/com/google/cloud/healthcare/fdamystudies/controller/HealthController.java

-            "status=%d and response=%s",
-            healthResponse.getStatusCodeValue(), healthResponse.getBody()));
-    return healthResponse;
+    return oauthService.health();


why is this method calling oauthservice.health()?

Ideally it should just return a 200 status.

I referred Monitoring Microservices With Health Checks, all services in our project requires token introspection so added downstream services in /healthCheck endpoint. I've changed to code to returns always OK. This will be pushed for review in next PR. Thanks a lot for approving the PR #540.

zohrehj · 2020-06-25T21:03:02Z

...modules/common-tests/src/main/resources/mappings/hydra/introspect_invalid_token_request.json

@@ -0,0 +1,18 @@
+{
+	"request": {


nit: indentation should probably be 2 spaces to be consistent with other files.

dhanyak-btc added 5 commits June 20, 2020 14:12

/v1/events endpoint implmentation

4ec17dc

Moved filters and health controller to common-service, added actuator, added JSoup to extract error message from Html etc

Refactored AuditLogEventValidator

8fe9275

convert xpath to json path

minor refactoring and removed unused method

e3b4ade

minor refactoring and removed unused getEncodedAuthorization() from BaseServiceImpl

Added postman collection & BaseTokenIntrospectionFilter

6bfb2a6

Added getUriTemplateAndHttpMethodsMap() abstract method to BaseTokenIntrospectionFilter

Removed unused method

78db1c6

Removed unused method: getEventInfo() from AuditLogEventEntity

googlebot added the cla: yes Do not use - reserved for devops label Jun 22, 2020

zohrehj reviewed Jun 22, 2020

View reviewed changes

Fixed PR#540 comments

8742363

Fixed PR#540 comments

nikklassen suggested changes Jun 22, 2020

View reviewed changes

Fixed PR review comments

5d8f03e

Fixed PR review comments

nikklassen mentioned this pull request Jun 23, 2020

Refactor audit-log to use Javax validators #551

Closed

nikklassen reviewed Jun 23, 2020

View reviewed changes

...vice/src/main/java/com/google/cloud/healthcare/fdamystudies/common/RestExceptionHandler.java Outdated Show resolved Hide resolved

dhanyak-btc added 6 commits June 24, 2020 19:43

Added logger statements to GlobalExceptionHandler

57fb701

Added logger statements to GlobalExceptionHandler

Moved AuditLogEventResponse to common-service module

12eea07

Moved AuditLogEventResponse to common-service module

Deleted postman collection

55b5f41

Deleted postman collection

Added @ToString.Exclude to entity class

4a8c9f5

Added @ToString.Exclude to entity class

Removed params from logger.entry

68c94de

Removed params from logger.entry

zohrehj suggested changes Jun 24, 2020

View reviewed changes

audit-log-module/audit-log-service/src/main/resources/application-local.properties Outdated Show resolved Hide resolved

nikklassen reviewed Jun 24, 2020

View reviewed changes

dhanyak-btc added 3 commits June 25, 2020 12:32

Fixed PR comments - imports and logger statements

1cab43d

Fixed Imports and Removed beans, request/response from logger statements

Changed the column definition to TIMESTAMP

e822804

Changed the column definition to TIMESTAMP for eventTimestamp and createdTimestamp in AuditLogEventEntity class. Added javdocs to eventTimestamp in AuditLogEventRequest

Removed @SuppressWarnings and changed log level to trace

ac8a1ec

Removed @SuppressWarnings("rawtypes") Changed log level to trace in handleConstraintValidationException() method

nikklassen approved these changes Jun 25, 2020

View reviewed changes

zohrehj approved these changes Jun 25, 2020

View reviewed changes

Merge branch 'early-access' into auditlog-events-endpoint

2920902

dhanyak-btc merged commit 7e74896 into early-access Jun 26, 2020

dhanyak-btc mentioned this pull request Jun 29, 2020

Fixed PR#540 comments and added Copyright text to java files #574

Merged

zohrehj deleted the auditlog-events-endpoint branch July 7, 2020 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit Log Service - /v1/events endpoint implementation #540

Audit Log Service - /v1/events endpoint implementation #540

dhanyak-btc commented Jun 22, 2020 •

edited

Loading

zohrehj Jun 22, 2020

dhanyak-btc Jun 22, 2020 •

edited

Loading

dhanyak-btc Jun 24, 2020

nikklassen left a comment

nikklassen Jun 22, 2020

dhanyak-btc Jun 23, 2020

nikklassen Jun 23, 2020

dhanyak-btc Jun 24, 2020

nikklassen commented Jun 24, 2020

nikklassen left a comment

zohrehj left a comment

zohrehj Jun 25, 2020

dhanyak-btc Jun 26, 2020 •

edited

Loading

zohrehj Jun 25, 2020

dhanyak-btc Jun 26, 2020

zohrehj Jun 25, 2020

dhanyak-btc Jun 26, 2020

zohrehj Jun 25, 2020

dhanyak-btc Jun 26, 2020

zohrehj Jun 25, 2020

Audit Log Service - /v1/events endpoint implementation #540

Audit Log Service - /v1/events endpoint implementation #540

Conversation

dhanyak-btc commented Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

dhanyak-btc Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikklassen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikklassen commented Jun 24, 2020

nikklassen left a comment

Choose a reason for hiding this comment

zohrehj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhanyak-btc Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhanyak-btc commented Jun 22, 2020 •

edited

Loading

dhanyak-btc Jun 22, 2020 •

edited

Loading

dhanyak-btc Jun 26, 2020 •

edited

Loading