ID-807 Get tos for user v2 #1238

Ghost-in-a-Jar · 2023-11-01T15:40:41Z

Ticket: https://broadworkbench.atlassian.net/browse/ID-807

What:

Endpoints for getting a user's ToS status/details for themselves or for another user if they are an admin

PR checklist

I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
I've filled out the Security Risk Assessment (requires Broad Internal network access) and attached the result to the JIRA ticket

…eDetails`. Introduce new `TermsOfServiceDetails`

… because of PostgresDirectoryDAOSpec, so adding the correct `assume` statements back into all the tests

…icated and unauthenticated routes both work

… for users are returned or denied properly

…ng AND requested user so that we can properly authorize requests

…are under /api

# Conflicts: # src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala # src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala # src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala # src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/MockSamRoutes.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala

sjkobori

Left some comments, but otherwise lgtm!

src/main/resources/swagger/api-docs.yaml

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRouteSpec.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala

sonarqubecloud · 2023-11-09T17:00:48Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
1.5% Duplication

tlangs

Thanks for all the work on this! Just one question.

tlangs · 2023-11-09T18:42:08Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala

-              }
-            } ~
-            pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
+            validate(samUserIdPattern.matches(userId), "User ID must be alpha numeric") {


Do we need this check?

Greg added it, idk that we NEED it but I think extra validation and user feedback is usually a good thing 🤷

Greg Polumbo and others added 26 commits October 23, 2023 15:32

ID-807 Rename deprecated TermsOfServiceDetails to `OldTermsOfServic…

5afdb4a

…eDetails`. Introduce new `TermsOfServiceDetails`

Merge branch 'develop' into ID-807-Get-ToS-for-user

e46846e

ID-807 Adding route tests and some custom matchers

e34e2f0

ID-807 Not sure if others had this problem, but my tests were failing…

323ff18

… because of PostgresDirectoryDAOSpec, so adding the correct `assume` statements back into all the tests

ID-807 adding tests for routes and rearranging things so that authent…

b01b76c

…icated and unauthenticated routes both work

ID-807 routing comment cleanup

0c9b1d3

ID-807 validate user_id in url

8fc08d5

ID-807 additional routing tests to make sure Terms of Service Details…

545b776

… for users are returned or denied properly

ID-807 Convert flatspec to funspec. Make sure we pass around requesti…

2410427

…ng AND requested user so that we can properly authorize requests

ID-807 test tidying

27224e8

ID-807 some validation rules

9190d50

ID-807 scalafmt

41b3163

ID-807 clarifying comment on new directive

221c8a9

ID-807 route format

96291d6

ID-807 moving the route around slightly so that even the public docs …

f955c07

…are under /api

ID-807 comments

91923e2

ID-807 fixing failing tests from new directive

edf6957

ID-807 fixing failing tests from new directive

b220a66

ID-807 fixing unused import

598bdd3

Merge branch 'develop' into ID-807-Get-ToS-for-user

00436cf

Merge branch 'develop' into ID-807-Get-ToS-for-user

9202dce

ID-807 fixing stuff

c77dd06

ID-807 Making the GET TOS Details endpoints return the right objects

5d8c235

ID-807 scalafmt

c0c9466

Merge branch 'develop' into ID-807-Get-ToS-for-user

2a01d39

Ghost-in-a-Jar force-pushed the ID-807-Get-ToS-for-user-v2 branch from a1d1ba3 to 526cc4d Compare November 1, 2023 15:51

merge develop

6f77d68

Ghost-in-a-Jar force-pushed the ID-807-Get-ToS-for-user-v2 branch from 526cc4d to 6f77d68 Compare November 1, 2023 16:57

Ghost-in-a-Jar marked this pull request as ready for review November 1, 2023 16:58

Ghost-in-a-Jar changed the title ~~Id 807 get to s for user v2~~ ID-807 Get tos for user v2 Nov 1, 2023

Add swagger docs

70038ad

Ghost-in-a-Jar force-pushed the ID-807-Get-ToS-for-user-v2 branch from f5dca60 to 70038ad Compare November 2, 2023 19:16

tlangs suggested changes Nov 6, 2023

View reviewed changes

Ghost-in-a-Jar force-pushed the ID-807-Get-ToS-for-user-v2 branch 2 times, most recently from 98c7d65 to 7c91f66 Compare November 6, 2023 19:34

Address tlang's comments

35776ed

Ghost-in-a-Jar force-pushed the ID-807-Get-ToS-for-user-v2 branch from 7c91f66 to 35776ed Compare November 6, 2023 19:36

tlangs suggested changes Nov 6, 2023

View reviewed changes

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala Outdated Show resolved Hide resolved

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala Outdated Show resolved Hide resolved

sjkobori approved these changes Nov 7, 2023

View reviewed changes

Moved isAdmin check into TosService

75df08b

Ghost-in-a-Jar force-pushed the ID-807-Get-ToS-for-user-v2 branch from 15529cd to 75df08b Compare November 8, 2023 21:45

Ghost-in-a-Jar added 2 commits November 9, 2023 10:47

Addressing comments

07ad62e

Merge branch 'develop' into ID-807-Get-ToS-for-user-v2

b7da745

tlangs suggested changes Nov 9, 2023

View reviewed changes

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala Outdated Show resolved Hide resolved

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala Outdated Show resolved Hide resolved

Ghost-in-a-Jar added 2 commits November 9, 2023 11:46

Refactoring getTermsOfServiceDetailsForUser

ead139e

Moved ensureAdminIfNeeded to new trait SupportsAdmin

76bd86b

Ghost-in-a-Jar force-pushed the ID-807-Get-ToS-for-user-v2 branch from 51686ed to 76bd86b Compare November 9, 2023 17:00

tlangs approved these changes Nov 9, 2023

View reviewed changes

Ghost-in-a-Jar merged commit d992455 into develop Nov 9, 2023
17 of 22 checks passed

Ghost-in-a-Jar deleted the ID-807-Get-ToS-for-user-v2 branch November 9, 2023 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ID-807 Get tos for user v2 #1238

ID-807 Get tos for user v2 #1238

Ghost-in-a-Jar commented Nov 1, 2023 •

edited

Loading

sjkobori left a comment

sonarqubecloud bot commented Nov 9, 2023

tlangs left a comment

tlangs Nov 9, 2023

Ghost-in-a-Jar Nov 9, 2023

ID-807 Get tos for user v2 #1238

ID-807 Get tos for user v2 #1238

Conversation

Ghost-in-a-Jar commented Nov 1, 2023 • edited Loading

Endpoints for getting a user's ToS status/details for themselves or for another user if they are an admin

sjkobori left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 9, 2023

tlangs left a comment

Choose a reason for hiding this comment

tlangs Nov 9, 2023

Choose a reason for hiding this comment

Ghost-in-a-Jar Nov 9, 2023

Choose a reason for hiding this comment

Ghost-in-a-Jar commented Nov 1, 2023 •

edited

Loading