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 - Apply filter-based auth for all API endpoints (2/2) #9360

Merged
merged 33 commits into from Feb 22, 2023

Conversation

GPortas
Copy link
Contributor

@GPortas GPortas commented Feb 2, 2023

What this PR does / why we need it:

This is a continuation of the refactoring work done for PR #9303, containing the remaining changes to close issue #9293. It is necessary to review and merge PR #9303 before reviewing this PR.

In particular, this PR replaces the calls to the deprecated findUserOrDie and findAuthenticatedUserOrDie methods with the filter-based design and removes the deprecated methods, no longer used by any API endpoint.

Which issue(s) this PR closes:

Closes #9293

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

…ted AbstractApiBean method 'response' removed
… from AbstractApiBean in addition to its associated methods
@GPortas GPortas added Feature: API NIH OTA: 1.7.1 (reArchitecture) 7 | 1.7.1 | Research & architecture for separating backend and frontend to enable a flexible, sca... User Role: API User Makes use of APIs labels Feb 2, 2023
@GPortas GPortas added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Feb 2, 2023
@GPortas GPortas changed the title 9293 - Apply filter-based auth for all API endpoints 9293 - Apply filter-based auth for all API endpoints (2/2) Feb 2, 2023
@GPortas GPortas moved this from Ready for Review ⏩ to IQSS Team - In Progress 💻 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Feb 2, 2023
@GPortas GPortas self-assigned this Feb 2, 2023
@coveralls
Copy link

coveralls commented Feb 2, 2023

Coverage Status

Coverage: 20.116% (+0.03%) from 20.091% when pulling 5f52321 on GPortas:9293-filter-api-auth-final-refactor into b0c9283 on IQSS:develop.

@GPortas GPortas moved this from IQSS Team - In Progress 💻 to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Feb 3, 2023
@GPortas GPortas removed their assignment Feb 3, 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
@mreekie
Copy link

mreekie commented Feb 8, 2023

Kickoff resizing

  • size 10 based on the issue being sized at 33

@mreekie mreekie added the Size: 10 A percentage of a sprint. 7 hours. label Feb 8, 2023
@qqmyers
Copy link
Member

qqmyers commented Feb 8, 2023

FWIW - In another conversation, it was just pointed out to me that the API calls in the mydata package all appear to use

private AuthenticatedUser getUserFromIdentifier(String userIdentifier){
instead of findUser/findAUthenticatedUserOrDie. They should probably be updated to use the filter as well, either here or as a new issue/PR.
These may be the only api calls outside the api package - I didn't find any @path annotations anywhere else.

@GPortas
Copy link
Contributor Author

GPortas commented Feb 9, 2023

FWIW - In another conversation, it was just pointed out to me that the API calls in the mydata package all appear to use

private AuthenticatedUser getUserFromIdentifier(String userIdentifier){

instead of findUser/findAUthenticatedUserOrDie. They should probably be updated to use the filter as well, either here or as a new issue/PR.
These may be the only api calls outside the api package - I didn't find any @path annotations anywhere else.

@qqmyers

The only call to the getUserFromIdentifier method used to authenticate a user is wrapped in a DEBUG condition (which is always false and it might be interesting to remove it, since it never gets called), please take a look at the call I'm referring to:

authUser = getUserFromIdentifier(userIdentifier);

The rest of the calls to getUserFromIdentifier are made once the user has been authenticated through another mechanism. These mechanisms are checked separately, not via the old findUserOrDie or findAuthenticatedUserOrDie methods, as you mention.

In effect, that endpoint is a candidate for the Auth Filter to be applied on it. By having already identified this, I think it's a good time to also refactor that endpoint in this same PR.

FWIW - I see that session authentication is also enabled for such endpoint (See

} else if ((session.getUser() != null)&&(session.getUser().isAuthenticated())){
), I don't know if there is any special reason. Perhaps the endpoint is called or has been called in the past from JSF, so it requires to authenticate by Session Cookie. In any case, the Auth Filter can be applied while also maintaining session authentication for this API resource, if necessary.

@qqmyers
Copy link
Member

qqmyers commented Feb 9, 2023

@GPortas - Thanks! Sorry for the bad code reference. The real issue that was found is that signedURLs don't work with these calls. It's because this class is checking for a session or api key (I think at

authUser = findUserByApiToken(apiToken);
) by itself so it therefore isn't seeing the newer methods like signedURLs and workflow tokens.

MyData pre-dates me, but I think you are probably right about the session being accepted - the MyData page apparently uses different technologies and probably calls that API directly. I would guess that use is current and should be kept, but I haven't checked.

It does look like removing that if DEBUG statement that is always false would be good code clean-up.

@GPortas
Copy link
Contributor Author

GPortas commented Feb 9, 2023

@GPortas - Thanks! Sorry for the bad code reference. The real issue that was found is that signedURLs don't work with these calls. It's because this class is checking for a session or api key (I think at

authUser = findUserByApiToken(apiToken);

) by itself so it therefore isn't seeing the newer methods like signedURLs and workflow tokens.
MyData pre-dates me, but I think you are probably right about the session being accepted - the MyData page apparently uses different technologies and probably calls that API directly. I would guess that use is current and should be kept, but I haven't checked.

It does look like removing that if DEBUG statement that is always false would be good code clean-up.

@qqmyers

Great, thank you for the clarification. That issue will be solved by applying the Auth Filter to the endpoint, which includes the newer methods you mention.

I'm already working on these changes, to add them to this PR.

…lter to allow more authentication mechanisms
@GPortas GPortas removed their assignment Feb 9, 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.

status codes, messages, more auth, tests

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

kcondon commented Feb 21, 2023

@GPortas This pr has the same signedurl issue as part one -encoding? Would a refresh of this branch from develop solve the problem?

@GPortas GPortas self-assigned this Feb 22, 2023
@GPortas GPortas removed their assignment Feb 22, 2023
@kcondon kcondon merged commit 1a79717 into IQSS:develop Feb 22, 2023
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Feb 22, 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: 10 A percentage of a sprint. 7 hours. User Role: API User Makes use of APIs
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

New filter-based design for the API authentication mechanisms
7 participants