Feature/inf 158 by ctucker3 · Pull Request #17 · Breeding-Insight/bi-api

ctucker3 · 2020-05-14T14:54:59Z

This features adds the user system roles and the user id into JWT claims. It also adds a means for the controllers to easily access those roles and id in the claims.

This feature also includes some code contributed by Tim that will be used when program roles enforcement is implemented. The code is active, but won't be called if the @ProgramSecured annotation is not used on any controllers. Which it is not yet.

Tests were added for the rare case that a user has a valid JWT, but not a user account. This case can occur if the user is valid, gets their JWT, but then their user account gets deleted (by a dev directly in the database), but there JWT hasn't expired yet. We don't check for this in the controller, but will be caught in the database because there is a foreign key constraint on the updated_by, created_by in the database.

Dependencies

This feature modifies the login flow to bi-web. There is a matching feature card in bi-web, INF-158 that should be reviewed alongside this.

Review and Tests

Do the bad authentication cases work?
- Logging in with an orcid that doesn't exist in bi-api
- Logging in with an orcid whose user account is deactivate
If a non-admin user tries to access an ADMIN endpoint, do they get a 403
Have all controllers been updated to use the new SecurityService
General code review

ctucker3 · 2020-05-14T16:41:35Z

INF-158 - Add user object to JWT in bi-api

nickpalladino · 2020-05-14T18:01:12Z

A database migration was added for this so make sure you do a flyway migration and generate the jooq sources.

nickpalladino · 2020-05-14T18:48:40Z

bi-web uses a health endpoint that is provided by micronaut which is not currently in the api docs. Even though we didn't write it, it's still part of the api for the system so I think it should be added to the api docs.

nickpalladino · 2020-05-15T15:54:55Z

Do the bad authentication cases work?
- Logging in with an orcid that doesn't exist in bi-api (works)
- Logging in with an orcid whose user account is deactivate (works)
Does it matter if the orcid is changed after the user has the JWT that they can still perform operations? Technically the orcid logged in with doesn't exist anymore but the user still does. Not sure that's really possible outside of test cases.
If a non-admin user tries to access an ADMIN endpoint, do they get a 403 (works)
Have all controllers been updated to use the new SecurityService (yes)

nickpalladino · 2020-05-14T20:39:24Z

src/main/java/org/breedinginsight/api/v1/controller/ProgramController.java

@@ -107,18 +102,13 @@ public HttpResponse<Response<Program>> createProgram(Principal principal, @Valid
    @Produces(MediaType.APPLICATION_JSON)
    @AddMetadata
    @Secured(SecurityRule.IS_AUTHENTICATED)


I see that the edit endpoints are not set to ADMIN which looks consistent with the requirements flow charts (admin & breeder) but I'm wondering about the front end. Currently you can only edit a program on the admin page. Not for this card but do we need to add editing capability to the user program management page if you have a breeder program role? Or would a breeder get pieces of admin functionality on the sidebar? Just discussing here because this permission thing made me think of it. Thoughts @eawoods?

I believe that permission scheme is an intentional decision -> program metadata (name and species) is not editable by anyone except the admin role.

nickpalladino · 2020-05-15T15:49:20Z

src/main/java/org/breedinginsight/api/v1/controller/UserController.java

    @Produces(MediaType.APPLICATION_JSON)
-    @Secured(SecurityRule.IS_AUTHENTICATED)
+    @Secured({"ADMIN"})
    public HttpResponse deleteUser(@PathVariable UUID userId){


Maybe not this card but eventually we're going to have deactivating users I think so this would be using the securityService to get the acting user. I see we have the active flag in the database now so may be a good time to change this?

After talking with Tim, I think the thought right now is to shorten the JWT expiration time and add a refresh token capability so that the user will get their refreshed info frequently. Not checking the user on the incoming call does leave a gap equal to the duration of the JWT expiration where a bad actor with an account can still have access though. But, I think the thought was that the case would be rare and they would have to be in the system to start anyway, so the case may not be worth making checks for it. It does avoid having to query the user on every single call. I'm down to discuss this again though if you had thoughts?

Also, for the deactivate user endpoint, there is a card made for switching that endpoint over on the UI side,

https://trello.com/c/ij6pPrRN/127-pro-56-ui-update-deactivate-modal-text-to-replace-remove-with-deactivate

We could just mix the controller in on that too. I want to save it for later since changing it over is outside of scope for this card.

I'm happy to discuss this further too

Posting this thought before I forget it. We could create a security rule that will check if the method being requested has the @Secured annotation, and if it's anything but a GET, and if so, then check in the db to make sure the user that the JWT was issued for is still active, therefore preventing a bad actor from making destructive changes the instant their account is deactivated.

I like that idea! It seems like a good balance between cutting down the queries, but giving us some added protection as well. @nickpalladino thoughts about this?

sounds good

@timparsons @nickpalladino A new security role has been added. A lot of the new tests were removed that checked for an active user that didn't exist in the database because the security rule will stop the flow before they get to the controller in that case. New tests were added for the security rule cases.

I put a TODO in ProgramSecuredAnnotationRule class to add tests when the time comes.

ctucker3 · 2020-05-18T12:22:06Z

Does it matter if the orcid is changed after the user has the JWT that they can still perform operations? Technically the orcid logged in with doesn't exist anymore but the user still does. Not sure that's really possible outside of test cases.

Yeah that is a good point. My thought would be that it doesn't matter to us if their orcid account was removed. Since the orcid is used so we can know they are who they are, once we know who they are we can decide whether they should continue to have access to our system on our side and just check to see if they are who they are when they need a new JWT.

ctucker3 · 2020-05-18T13:32:29Z

bi-web uses a health endpoint that is provided by micronaut which is not currently in the api docs. Even though we didn't write it, it's still part of the api for the system so I think it should be added to the api docs.

changed and pushed

eawoods

Looks good to me, and I got all the possible error states to trigger at the correct times.
I defer to @nickpalladino and @timparsons on technical endpoints/permissions/token/timeout questions.

timparsons

Looks good. One comment on one test

src/test/java/org/breedinginsight/api/v1/controller/ProgramControllerIntegrationTest.java

…th flow

timparsons

A few small comments/quick discussion points

src/main/java/org/breedinginsight/api/auth/AuthServiceLoginHandler.java

src/main/java/org/breedinginsight/api/auth/SecurityService.java

src/main/java/org/breedinginsight/api/v1/controller/ProgramController.java

src/main/java/org/breedinginsight/model/ProgramLocation.java

ctucker3 requested review from dmeidlin, eawoods, nickpalladino and timparsons May 14, 2020 16:07

ctucker3 marked this pull request as ready for review May 14, 2020 16:08

ctucker3 force-pushed the feature/INF-158 branch from f8b9429 to e10d979 Compare May 14, 2020 16:32

nickpalladino requested changes May 15, 2020

View reviewed changes

eawoods approved these changes May 18, 2020

View reviewed changes

timparsons requested changes May 18, 2020

View reviewed changes

src/test/java/org/breedinginsight/api/v1/controller/ProgramControllerIntegrationTest.java Outdated Show resolved Hide resolved

ctucker3 requested review from nickpalladino and timparsons May 19, 2020 21:08

Chris T added 12 commits May 20, 2020 09:08

Check user exists in DB for login

cbe04db

first pass at new JWT claims

887871d

converted controllers to use security service. Added tests for new au…

7de2945

…th flow

converted controllers to use security service. Added tests

b4a437b

cleanup fixes

c0e03a2

integrate program locations to new auth flow

e7669d6

add health check to docs

d75184a

add health check to docs

4f7e4d5

fix mistake changes in api docs

2271700

change login failed redirect path

e8cf297

change login failed route

471cfe6

add valid user check for non GET calls + tests

99241c1

ctucker3 force-pushed the feature/INF-158 branch from d2808a3 to 99241c1 Compare May 20, 2020 13:08

fix broken oauth login

3b6e1ed

timparsons requested changes May 21, 2020

View reviewed changes

Chris T added 2 commits May 21, 2020 12:38

change authentication failure reason

324a48a

throw exception in security service

45f1481

timparsons approved these changes May 21, 2020

View reviewed changes

nickpalladino approved these changes May 21, 2020

View reviewed changes

ctucker3 merged commit 3ce61c9 into develop May 21, 2020

ctucker3 deleted the feature/INF-158 branch May 21, 2020 18:30

nickpalladino added the feature label Aug 24, 2020

Conversation

ctucker3 commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependencies

Review and Tests

Uh oh!

ctucker3 commented May 14, 2020

Uh oh!

nickpalladino commented May 14, 2020

Uh oh!

nickpalladino commented May 14, 2020

Uh oh!

nickpalladino commented May 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eawoods May 18, 2020 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

ctucker3 commented May 18, 2020

Uh oh!

ctucker3 commented May 18, 2020

Uh oh!

eawoods left a comment

Choose a reason for hiding this comment

Uh oh!

timparsons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timparsons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ctucker3 commented May 14, 2020 •

edited

Loading

eawoods May 18, 2020 •

edited

Loading