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

Remove the authentication annotation from info API #10131

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

jp-tosca
Copy link
Contributor

@jp-tosca jp-tosca commented Nov 20, 2023

What this PR does / why we need it:

After the changes that happened on #9303 and #9360 the @AuthenticationRequired was present on some API methods that are public and some additional code that is not required, this change:

-Overloads AbstractAPIBean.response requiring only a DataverseRequestHandler and passing a null User.
-Removes the @AuthenticationRequired from the following info endpoints: {version, server and apiTermsOfUse}
-it also removes the @Context ContainerRequestContext from the signature of the method and makes a new call to the new re-introduced signature. This signature was deprecated on #9303 and removed on #9360.

Which issue(s) this PR closes:

Closes #9466

Special notes for your reviewer:
This method signature was marked as deprecated on #9303, probably the intention of the author was to deprecate it while the functionality was migrated to the new authentication mechanism and removed once all the changes were done.

Suggestions on how to test this:
-Run the APIs and integration tests.

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

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

Additional documentation:
N/A

@coveralls
Copy link

coveralls commented Nov 20, 2023

Coverage Status

coverage: 20.006% (+0.003%) from 20.003%
when pulling 8e69c6d on jp-tosca:9466-authentication-annotation-change
into 454e0bb on IQSS:develop.

@jp-tosca jp-tosca force-pushed the 9466-authentication-annotation-change branch 2 times, most recently from dd3db8f to 9a5f523 Compare November 21, 2023 20:42
@GPortas GPortas self-assigned this Nov 22, 2023
* @return HTTP Response appropriate for the way {@code hdl} executed.
*/
protected Response response(DataverseRequestHandler hdl) {
return response(hdl, null);
Copy link
Contributor

@GPortas GPortas Nov 22, 2023

Choose a reason for hiding this comment

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

This passes a null user to the response(DataverseRequestHandler hdl, User user) method which will create the DataverseRequest object with a null user, and the injected HttpServletRequest.

Until now I have always seen that the DataverseRequest class is instantiated with a user, even if it is GuestUser, in order to perform permission checks on the actions to be carried out on authenticated API endpoints.

Without the calling user, the DataverseRequest object would only contain the injected HttpServletRequest, which as far as I know its purpose there is to know the user IP address, which I'm not sure is useful information for unauthenticated operations like those in Info (if I'm not mistaken, these address checks are only used in certain areas and are not common in Dataverse).

If there are no specific reasons that I am not aware of for keeping the HttpServletRequest data and therefore use the DataverseRequest class in unauthenticated scenarios, I would not create a DataverseRequest for unauthenticated operations, and would call the required logic directly like here.

I'm almost certain that null users are never passed when instantiating a DataverseRequest object. Maybe other people can confirm this. I also think that allowing null users can cause future accidental NPE issues.

Copy link
Contributor

@GPortas GPortas Nov 22, 2023

Choose a reason for hiding this comment

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

If you look at the method, which you mention is reintroduced, the one introduced is not the same as the previous one. Please take a look at the old one here: https://github.com/IQSS/dataverse/blob/v5.12/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java#L705

You can see that the old method always passed a non-null user to the DataverseRequest object, coming from the old findUserOrDie method.

Copy link
Contributor Author

@jp-tosca jp-tosca Nov 22, 2023

Choose a reason for hiding this comment

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

as far as I know its purpose there is to know the user IP address, which I'm not sure is useful information for unauthenticated operations like those in Info

Probably we can get more input about this on the next stand-up meeting. Right now, it is not used on these public endpoints, but should we keep it so we can have an IP for log/additional functionality on the future?

If you look at the method, which you mention is reintroduced, the one introduced is not the same as the previous one. Please take a look at the old one here

The new response(DataverseRequestHandler hdl) method is not intended to be the same than the older one, I added it as a way to expose the response method without the need of the user. My initial thought process was that this was not done previously because there were dependencies to response(DataverseRequestHandler hdl) in the past while the other APIs were migrated to the new functionality defined on response(DataverseRequestHandler hdl, User user).

Copy link
Member

Choose a reason for hiding this comment

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

In the auth design, the very first point is that there should always be a user:

## Notes About the Design
* Invariant - there's always a user
    - Required for auditing, logging, etc.
    - Required for the Command Architecture

See https://github.com/IQSS/dataverse/blob/v6.0/doc/Architecture/auth.md

Perhaps we could simply replace null with the Guest user like this:

     protected Response response(DataverseRequestHandler hdl) {
-        return response(hdl, null);
+        return response(hdl, GuestUser.get());
     }

That way, the invariant will hold.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pdurbin will give it a look with this in mind.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could delete the new response() method and rewrite the three methods to not use it.

Copy link
Member

Choose a reason for hiding this comment

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

@GPortas @jp-tosca and I discussed this and decided to remove the new response() method in 8e69c6d.

Copy link
Contributor

@GPortas GPortas left a comment

Choose a reason for hiding this comment

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

Thanks for continuing the auth refactoring!

I just left a few considerations. Please let me know if you have any question.

@scolapasta scolapasta added this to the 6.1 milestone Nov 27, 2023
@pdurbin pdurbin self-assigned this Nov 27, 2023
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Ready to merge. I ran InfoIT locally but ideally we'll wait for a successful Jenkins run.

@pdurbin pdurbin self-assigned this Nov 28, 2023
@pdurbin
Copy link
Member

pdurbin commented Nov 28, 2023

I ran the tests with the API runner and they passed: https://github.com/gdcc/api-test-runner/actions/runs/7021121336

@pdurbin pdurbin merged commit 1ea692b into IQSS:develop Nov 28, 2023
10 of 11 checks passed
@pdurbin pdurbin removed their assignment Nov 28, 2023
@jp-tosca jp-tosca deleted the 9466-authentication-annotation-change branch February 18, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Evolve API authentication to omit it on endpoints intended to be open
5 participants