[RW-7834][risk=low]Require TOS acceptance on login when users are not Terra-TOS compliant #6479

NehaBroad · 2022-03-22T04:01:13Z

PR checklist

This PR meets the Acceptance Criteria in the JIRA story
The JIRA story has been moved to Dev Review
This PR includes appropriate unit tests
I have run and tested this change locally
I have run the E2E tests on ths change against my local UI and/or API server with yarn test-local or yarn test-local-devup
If this includes a UI change, I have taken screen recordings or screenshots of the new behavior and notified the PO and UX designer
If this includes an API change, I have updated the appropriate Swagger definitions and notified API consumers
If this includes a new feature flag, I have created and linked new JIRA tickets to (a) turn on the feature flag and (b) remove it later

api/src/main/java/org/pmiops/workbench/firecloud/FireCloudServiceImpl.java

NehaBroad · 2022-03-22T19:17:31Z

ui/src/app/pages/login/account-creation/account-creation-tos.tsx

  hasAckedTermsOfService: boolean;
 }

+// TODO: Rename this to TermsOfService


Will be creating a follow up PR to rename this component

ui/src/app/routing/app-routing.tsx

ui/src/app/utils/access-utils.tsx

api/src/main/java/org/pmiops/workbench/api/ProfileController.java

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

jmthibault79 · 2022-03-22T22:52:45Z

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

+    boolean termsOfServiceAccepted = false;
+    try {
+      termsOfServiceAccepted = fireCloudService.getUserTermsOfServiceStatus();
+    } catch (ApiException e) {


should we catch this exception, or simply let the underlying exception propagate?

Out of curiosity, where does log.log get stored to? Does it go into GCP somewhere?

I wanted to log the message with userId, was not sure if the underlying exception will do the same
@evrii yes it goes to google cloud console logger

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

api/src/main/java/org/pmiops/workbench/firecloud/FireCloudServiceImpl.java

evrii · 2022-03-23T13:27:59Z

api/src/main/java/org/pmiops/workbench/api/ProfileController.java

+    DbUser loggedInUser = userAuthenticationProvider.get().getUser();
+    userService.submitAouTermsOfService(loggedInUser, termsOfServiceVersion);
+    userService.acceptTerraTermsOfService(loggedInUser);
+    return ResponseEntity.status(HttpStatus.NO_CONTENT).build();


What does a HttpStatus of NO_CONTENT represent? Does it represent success or failure?

It represents Success void return

yes - this is HTTP 204

evrii · 2022-03-23T13:29:54Z

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

+    boolean termsOfServiceAccepted = false;
+    try {
+      termsOfServiceAccepted = fireCloudService.getUserTermsOfServiceStatus();
+    } catch (ApiException e) {


Out of curiosity, where does log.log get stored to? Does it go into GCP somewhere?

evrii · 2022-03-23T13:36:55Z

api/src/test/java/org/pmiops/workbench/api/ProfileControllerTest.java

  }

+  @Test
+  public void testMe_UserHasNotAcceptedTerraTOS()


What does "Me" mean in this function name? I see it in the function below too.

the profilecontroller has a function getMe() that returns the information about logged in user, thats why the test has tests like testMe_success() etc.

NehaBroad · 2022-03-24T16:12:06Z

After the change to call the single endpoint (getUserTerraTOS), rather than throwing error in profileController. The terra TOS check will happen just the first time user logs in, not on every page

api/src/main/resources/workbench-api.yaml

ui/src/app/utils/access-utils.tsx

api/src/test/java/org/pmiops/workbench/firecloud/FireCloudServiceImplTest.java

jmthibault79 · 2022-03-24T17:53:46Z

api/src/main/java/org/pmiops/workbench/api/ProfileController.java

+    DbUser loggedInUser = userAuthenticationProvider.get().getUser();
+    userService.submitAouTermsOfService(loggedInUser, termsOfServiceVersion);
+    userService.acceptTerraTermsOfService(loggedInUser);
+    return ResponseEntity.status(HttpStatus.NO_CONTENT).build();


yes - this is HTTP 204

jmthibault79 · 2022-03-24T17:59:23Z

api/src/main/java/org/pmiops/workbench/api/ProfileController.java

  }

+  @Override
+  public ResponseEntity<Boolean> getUserTerraTermsOfServiceStatus() {


The AC for this story includes checking for AoU TOS compliance here as well. If false, we can exit early: we are not compliant. We can then check Terra only when aou-tos=true.

OK, i did not check that because i assumed the account creation was successful hence the AoU TOS has been accepted.
I'll add the check and some tests

api/src/main/resources/workbench-api.yaml

api/src/test/java/org/pmiops/workbench/api/ProfileControllerTest.java

evrii · 2022-03-24T18:42:24Z

ui/src/app/pages/login/sign-in.tsx

          <AccountCreationTos
            filePath='/aou-tos.html'
-            onComplete={() => {
+            onComplete={(tosVersion) => {


It looks like the associated spec file is not passing a tosVersion.

ui/src/app/routing/app-routing.tsx

evrii · 2022-03-24T20:39:47Z

I am not sure if this is significant or not, but it looks like the TOS page for account creation and non-compliant TerraTOS do look different:
Account Creation:

Old TOS:

NehaBroad · 2022-03-24T20:44:53Z

@evrii Yes i had to update the HTML height for existing user TOS screen. I can tweek it a little more. In case there is something else i am missing do let me know

evrii

In my first pass, the functionality worked fine.

I was trying to see if I could get the TOS screen to show up again if I changed the TOS version. Instead of changing the TOS version, I deleted my record in user_terms_of_service. Now, I cannot seem to get passed the DAR page. I am not sure if this is something to be concerned about or not.

evrii · 2022-03-24T20:58:58Z

@evrii Yes i had to update the HTML height for existing user TOS screen. I can tweek it a little more. In case there is something else i am missing do let me know

No worries. It does not seem like a big deal to me. The only things I noticed were the different background colors and the logo in the top left of the screen.

NehaBroad · 2022-03-24T21:22:18Z

@evrii Hmm i was not able to reproduce this, but would really like to debug it with you tomorrow to confirm whats happening

api/src/main/resources/workbench-api.yaml

jmthibault79 · 2022-03-24T21:15:46Z

ui/src/app/utils/access-utils.tsx

+    boolean | undefined
+  >(false);
+  useEffect(() => {
+    if (


ui/src/app/utils/access-utils.tsx

jmthibault79 · 2022-03-24T21:24:38Z

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java


  @Override
-  public void validateTermsOfService(Integer tosVersion) {
+  public void validateAllOfUsTermsOfService(Integer tosVersion) {


when this was originally written, we only checked AOU TOS when creating the user. If the TOS was wrong, it made sense to throw a Bad Request.

But now we should be more thoughtful. If an existing user has an outdated TOS, that's not really what I would call a Bad Request. So I'd prefer the previous signature with Boolean

I prefer BadRequest (or maybe Unautorized) because it had message that helped identify why exactly the validation failed, is it a bad user who has not selected TOS (AoU or terra) in the first place or just outdated version .

That could also help in showing the user a more appropriate message if we need to change the UX.

I can make it boolean right now , so that we are unblocked and can discuss this further tomorrow

…else just true or false

jmthibault79

Minor changes requested, but we're looking good!

jmthibault79 · 2022-03-28T20:44:48Z

api/src/main/java/org/pmiops/workbench/api/ProfileController.java

-      // by approving the latest AOU Terms of Service, the user also approves the latest Terra TOS
-      try {
-        userService.validateTermsOfService(dbUser);
+      // getUserTermsOfServiceStatus and acceptTos will take care of scenario where user has not


the old comment has important context that we should retain here.

This new comment is good too, but it has some grammar issues that we should fix before merging.

jmthibault79 · 2022-03-28T20:48:36Z

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

    }
    if (tosVersion != LATEST_AOU_TOS_VERSION) {
-      throw new BadRequestException("Terms of Service version is not up to date");
+      throw new BadRequestException("All of Us Terms of Service version is not up to date");


good catch!

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

jmthibault79 · 2022-03-28T20:59:05Z

api/src/test/java/org/pmiops/workbench/db/dao/UserServiceTest.java


    // does not throw
    userService.validateTermsOfService(user);
  }


this test is not valid anymore. need to check for return value true

…ms of serviceaccepted

evrii · 2022-03-29T15:29:21Z

ui/src/app/utils/access-utils.tsx

  return disabled;
 };

+export const useNeedsToAcceptTOS = () => {


Cool, I don't think that I have ever seen useEffect used in a custom hook before.

jmthibault79 · 2022-03-29T15:43:27Z

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

+    try {
+      final int tosVersion =
+          userTermsOfServiceDao.findByUserIdOrThrow(dbUser.getUserId()).getTosVersion();
+      return tosVersion == LATEST_AOU_TOS_VERSION;
+    } catch (BadRequestException ex) {
+      // In case user doesnt have any terms of service
+      return false;
+    }


We don't need to throw an exception and catch it here. This also works:

Suggested change

try {

final int tosVersion =

userTermsOfServiceDao.findByUserIdOrThrow(dbUser.getUserId()).getTosVersion();

return tosVersion == LATEST_AOU_TOS_VERSION;

} catch (BadRequestException ex) {

// In case user doesnt have any terms of service

return false;

}

return userTermsOfServiceDao

.findFirstByUserIdOrderByTosVersionDesc(dbUser.getUserId())

.map(u -> u.getTosVersion() == LATEST_AOU_TOS_VERSION)

.orElse(false);

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java

api/src/test/java/org/pmiops/workbench/db/dao/UserServiceTest.java

… Terra-TOS compliant (#6479)

NehaBroad added 6 commits March 22, 2022 00:00

Redirect User to TOS page

0d4c873

Add log message

f3f4435

Refresh page after clicking next

f92e9e3

Update yaml description

fcaad27

Comments from mobbing

b7e0035

Using consistent naming convention

9fc63b6

NehaBroad commented Mar 22, 2022

View reviewed changes

api/src/main/java/org/pmiops/workbench/firecloud/FireCloudServiceImpl.java Show resolved Hide resolved

NehaBroad commented Mar 22, 2022

View reviewed changes

Fix test

c3f5cfb

NehaBroad marked this pull request as ready for review March 22, 2022 19:25

NehaBroad requested review from aweng98, evrii and jmthibault79 March 22, 2022 20:49

jmthibault79 reviewed Mar 22, 2022

View reviewed changes

evrii reviewed Mar 23, 2022

View reviewed changes

NehaBroad marked this pull request as draft March 23, 2022 14:34

PR Comments

b79fa70

NehaBroad marked this pull request as ready for review March 24, 2022 15:33

Spotless and better return for void

780562e

jmthibault79 reviewed Mar 24, 2022

View reviewed changes

NehaBroad added 2 commits March 24, 2022 14:11

Update swagger description and test method;

836bf7f

Validating TOS status includes checking AoU as well

2da9cb9

evrii reviewed Mar 24, 2022

View reviewed changes

NehaBroad and others added 2 commits March 24, 2022 16:30

Use enviornment flag in UI

9dd7faa

Merge branch 'main' into nsaxena/RW7860TOS

874616b

evrii reviewed Mar 24, 2022

View reviewed changes

jmthibault79 reviewed Mar 24, 2022

View reviewed changes

NehaBroad added 6 commits March 24, 2022 19:06

Return boolean instead of throwing badRequestException

c9597ca

rebase

8ecf4a7

Split validation method to throw exception in case of create account …

f3ff541

…else just true or false

Rebase

1a1bb83

Fix test

d1b5bfe

Spotless

a986c38

jmthibault79 approved these changes Mar 28, 2022

View reviewed changes

Pr Comments and also catch exception in case user doesnt have any ter…

db91b32

…ms of serviceaccepted

evrii reviewed Mar 29, 2022

View reviewed changes

Test fix

dcef920

jmthibault79 reviewed Mar 29, 2022

View reviewed changes

NehaBroad merged commit 29e1a34 into main Mar 29, 2022

NehaBroad deleted the nsaxena/RW7860TOS branch March 29, 2022 17:15

evrii approved these changes Mar 29, 2022

View reviewed changes

api/src/main/java/org/pmiops/workbench/db/dao/UserServiceImpl.java Show resolved Hide resolved

api/src/test/java/org/pmiops/workbench/db/dao/UserServiceTest.java Show resolved Hide resolved

chenchals pushed a commit that referenced this pull request Mar 31, 2022

[RW-7834][risk=low]Require TOS acceptance on login when users are not…

ac3433d

… Terra-TOS compliant (#6479)

[RW-7834][risk=low]Require TOS acceptance on login when users are not Terra-TOS compliant #6479

[RW-7834][risk=low]Require TOS acceptance on login when users are not Terra-TOS compliant #6479

Uh oh!

Conversation

NehaBroad commented Mar 22, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NehaBroad commented Mar 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evrii commented Mar 24, 2022

Uh oh!

NehaBroad commented Mar 24, 2022

Uh oh!

evrii left a comment

Choose a reason for hiding this comment

Uh oh!

evrii commented Mar 24, 2022

Uh oh!

NehaBroad commented Mar 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NehaBroad Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmthibault79 left a comment

NehaBroad Mar 24, 2022 •

edited

Loading