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

9293 - New filter-based design for the API authentication mechanisms (1/2) #9303

Merged
merged 37 commits into from Feb 21, 2023

Conversation

GPortas
Copy link
Contributor

@GPortas GPortas commented Jan 19, 2023

What this PR does / why we need it:

This PR includes the refactoring changes for transitioning to the new filter-based design described in #9293 for the current API authentication mechanisms.

Due to the magnitude of the changes and in order not to overextend the revision work of this PR, I have decided to limit the scope of this PR in relation to issue #9293.

In particular, this PR covers the following points:

  • The new authentication model is fully implemented, including all current authentication mechanisms (previously handled in the findUserOrDie method).
  • Only Datasets API methods have been evolved to use the new filter-based design. The rest of the API resources (Files, Dataverses, etc) still use the old model (findUserOrDie).
  • The findUserOrDie method has been marked as @Deprecated, as well as any other methods in the AbstractApiBean class related to the old auth design.

My idea for a next iteration is to evolve the rest of the API methods to use the new auth design so we can remove the old design logic (methods marked as @Deprecated in this PR).

Which issue(s) this PR closes:

Partially completes #9293 but does not close it.

Special notes for your reviewer:

Not yet

Suggestions on how to test this:

The behavior of the API must be the same as before, so in addition to the automated IT tests, curl calls to the API using different types of credentials can be used to test the different auth mechanisms.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No

Is there a release notes update needed for this change?:

Not sure

Additional documentation:

Not yet / Not sure

@GPortas GPortas added Feature: API User Role: API User Makes use of APIs NIH OTA: 1.7.1 (reArchitecture) 7 | 1.7.1 | Research & architecture for separating backend and frontend to enable a flexible, sca... labels Jan 19, 2023
@GPortas GPortas self-assigned this Jan 19, 2023
@coveralls
Copy link

coveralls commented Jan 19, 2023

Coverage Status

Coverage: 20.091% (+0.08%) from 20.009% when pulling fdab1b3 on GPortas:9293-filter-api-auth into 1aabf69 on IQSS:develop.

@mreekie
Copy link

mreekie commented Jan 23, 2023

Sizing: Still in Draft so no sizing yet.

@GPortas GPortas added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Jan 30, 2023
@GPortas GPortas added the Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) label Jan 30, 2023
@GPortas GPortas changed the title 9293 - New filter-based design for the API authentication mechanisms 9293 - New filter-based design for the API authentication mechanisms (1/2) Feb 2, 2023
@scolapasta scolapasta moved this from Ready for Review ⏩ to Review 🔎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Feb 8, 2023
Copy link
Contributor

@rtreacy rtreacy left a comment

Choose a reason for hiding this comment

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

examined all implementations in the auth package, new methods in AbstractApiBean such as getRequestUser(...) and getRequestAuthenticatedUserOrDie(...) which use ContainerRequestContext for parameters and are designed to replace findUserOrDie() and findAuthenticatedUserOrDie() and their use in DataSets API. Looks good

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🔎 to Ready for QA ⏩ Feb 10, 2023
@rtreacy rtreacy removed their assignment Feb 10, 2023
@kcondon kcondon self-assigned this Feb 14, 2023
@kcondon
Copy link
Contributor

kcondon commented Feb 14, 2023

@GPortas Please refresh from develop due to version numbers changing.

@GPortas
Copy link
Contributor Author

GPortas commented Feb 15, 2023

@GPortas Please refresh from develop due to version numbers changing.

Done, @kcondon. Thanks!

@kcondon
Copy link
Contributor

kcondon commented Feb 16, 2023

@GPortas So far, have tested 3 endpoints, gets JSON representation of dataset, publish dataset, create/get private url. They use getRequestUser and getRequestAuthenticatedUserOrDie.

They continue to work with api token and workflow token but so far do not work with signed url, returns {"status":"ERROR","message":"Bad signed URL"}. There is no server log error. It works on develop branch with the same exact signed url.

What I did to test signed url:
Pass Get JSON Representation of Dataset url into Request Signed URL endpoint for username who created the unpublished dataset, using api token of dataverseAdmin (super user) to execute the request.

Posted resulting url into a different browser, got metadata on develop branch, bad signed url on this pr.

@GPortas GPortas self-assigned this Feb 17, 2023
Copy link
Contributor

@rtreacy rtreacy left a comment

Choose a reason for hiding this comment

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

Decoding issue causing signed url authentication to fail

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Ready for QA ⏩ Feb 17, 2023
@kcondon kcondon merged commit b0c9283 into IQSS:develop Feb 21, 2023
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Feb 21, 2023
@pdurbin pdurbin added this to the 5.14 milestone Feb 27, 2023
@mreekie mreekie added the pm.GREI-d-1.7.1 NIH, yr1, aim7, task1: Research & architecture for separating backend and frontend label Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: API NIH OTA: 1.7.1 (reArchitecture) 7 | 1.7.1 | Research & architecture for separating backend and frontend to enable a flexible, sca... pm.GREI-d-1.7.1 NIH, yr1, aim7, task1: Research & architecture for separating backend and frontend Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) User Role: API User Makes use of APIs
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

7 participants