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

Refactor api user check auth0 #84

Merged
merged 36 commits into from
Nov 10, 2020
Merged

Refactor api user check auth0 #84

merged 36 commits into from
Nov 10, 2020

Conversation

br648
Copy link
Contributor

@br648 br648 commented Oct 9, 2020

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

This PR builds on #82 by removing all auth based on an Api user's API key. Instead auth is handled by Auth0. To accomodate this, a new endpoint has been added to allow an Api user to obtain a bearer token from Auth0 by providing their username (email) and password.

For clarity, the checks tieing an Otp user to an Api user based on an Api user's API key remains.

PR also includes an update to cover this issue: #81. I could not strictly replicate the issue because Auth0 would reject creating an account with an existing email address. I could replicate if I create the admin user in just otp-middleware. I've added the suggested userId parameter, but if this isn't provided the conditional checks would still allow all trips to be returned for an Otp user. Therefore, I have updated the isUserAdmin to return true only if the requesting user is an admin (i.e. does not have mutiple user types).

landonreed and others added 6 commits September 22, 2020 15:06
instead of just checking the Auth0 token for the requesting user, we need to permit third party API
users to authenticate with an API token. This begins to make those changes
Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

This looks to be going in the right direction. I think my main comment is to add a method to check if a user isThirdPartyUser, so that we can track better where this is being called and augment it with additional checks if needed in the future.

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #84 (9072f1e) into dev (ed41d0b) will increase coverage by 7.98%.
The diff coverage is 61.53%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev      #84      +/-   ##
============================================
+ Coverage     46.14%   54.13%   +7.98%     
- Complexity      363      446      +83     
============================================
  Files            85       85              
  Lines          2540     2627      +87     
  Branches        289      308      +19     
============================================
+ Hits           1172     1422     +250     
+ Misses         1246     1031     -215     
- Partials        122      174      +52     
Impacted Files Coverage Δ Complexity Δ
...iddleware/controllers/api/AdminUserController.java 66.66% <ø> (ø) 1.00 <0.00> (ø)
...pentripplanner/middleware/models/AbstractUser.java 68.42% <ø> (+15.78%) 6.00 <0.00> (+1.00)
...g/opentripplanner/middleware/models/AdminUser.java 69.23% <0.00%> (+15.38%) 3.00 <0.00> (+1.00)
...g/opentripplanner/middleware/tripMonitor/Main.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...iddleware/tripMonitor/jobs/CheckMonitoredTrip.java 41.71% <ø> (ø) 19.00 <0.00> (ø)
...leware/controllers/api/AbstractUserController.java 56.66% <27.27%> (+2.82%) 5.00 <1.00> (+2.00)
...entripplanner/middleware/models/MonitoredTrip.java 72.44% <33.33%> (+12.87%) 39.00 <0.00> (+7.00)
...entripplanner/middleware/auth/Auth0Connection.java 46.53% <36.00%> (+33.05%) 19.00 <4.00> (+9.00)
...nner/middleware/controllers/api/LogController.java 46.29% <50.00%> (ø) 2.00 <0.00> (ø)
...org/opentripplanner/middleware/models/OtpUser.java 52.00% <50.00%> (+6.54%) 9.00 <2.00> (+3.00)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed41d0b...9072f1e. Read the comment docs.

@binh-dam-ibigroup
Copy link
Collaborator

Nice to see #81 fixed.

@landonreed landonreed mentioned this pull request Nov 6, 2020
5 tasks
@br648 br648 requested a review from landonreed November 9, 2020 18:16
@br648 br648 removed their assignment Nov 9, 2020
*/
//FIXME: Move this check into existing auth checks so it would be carried out automatically prior to any
// business logic. Consider edge cases where a user can be both an API user and OTP user.
public static void linkApiKeyToApiUser(Request req) {
Copy link
Contributor

@landonreed landonreed Nov 9, 2020

Choose a reason for hiding this comment

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

With respect to the method name, maybe just ensureApiUserHasApiKey? Has this logic changed from its previous iteration? I think, as it stands, this check will cause a halt under the following conditions:

  1. I sign up as an API user for [email protected]
  2. I attempt to sign up as an OTP user in the MOD UI using [email protected].
  3. The halt is thrown because the MOD UI uses a different API key than my API user API key.

Perhaps the way to handle this is with token scopes that we provide in the getCompleteAuth0TokenResponse method. Let's discuss on our technical call tomorrow.

Javadoc:

Check that the API key used in the incoming request is associated with the matching {@link ApiUser} (which is determined from the Authorization header).

Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Squirrelly issue with linking API key to user, but perhaps we should address it in a new PR (this one has over 200 comments).

@landonreed landonreed assigned br648 and unassigned landonreed Nov 9, 2020
@binh-dam-ibigroup
Copy link
Collaborator

@br648, please remember to fix conflicts as result of merging #77. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants