-
Notifications
You must be signed in to change notification settings - Fork 531
Login page and Shibboleth auth refactored to work with new InCommon services #11502
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
Conversation
…out relying on discoffeed; wayfinder redirect url is still hard-coded; the overall setup is working on the unc test box. #11404
… new InCommon Wayfinder service. Before I make a pr, I want to put back a DiscoFeed-based login workflow, as a configurable option, just in case some instance out there has a reason to keep using it. #11404
… new mdq and wayfinder configuration #11404
…discofeed will no longer be part of the setup when using InCommon. #11404
resolved conflicts: doc/sphinx-guides/source/installation/shibboleth.rst
...a/edu/harvard/iq/dataverse/authorization/AuthenticationProvidersRegistrationServiceBean.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibServiceBean.java
Dismissed
Show dismissed
Hide dismissed
This comment has been minimized.
This comment has been minimized.
…d w/ MDQ from the documentation #11404
This comment has been minimized.
This comment has been minimized.
pdurbin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some tweaks to the docs in 4ecb090 and I'm leaving a little non-critical feedback in this review. I hope I'm understanding the InCommon vs non-InCommon divide properly! Overall, this is great!
|
|
||
| String baseUrl; | ||
| if (FeatureFlags.SHIBBOLETH_USE_LOCALHOST.enabled()) { | ||
| baseUrl = "http://localhost"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://localhost is hard coded here and in the shibservicebean (within the string http://localhost/Shibboleth.sso/DiscoFeed). Should http://localhost be factored into a common location? Should https://localhost (https instead of http) be an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually needs to be http:, not https: - since that will result in a failure if called from Java, since the ssl certificate is not going to be valid for localhost. And this in turn was the main reason why I chose not to make this the default behavior, since calling http://localhost/Shibboleth.sso/DiscoFeed would not work on an instance that followed our recommendations for the Apache configuration. I.e., things would break for most Shibboleth-using instances without a configuration change - and, seeing how this localhost gimmick is extremely unlikely to be needed by any instance other than ours, I absolutely wanted to avoid that.
...a/edu/harvard/iq/dataverse/authorization/AuthenticationProvidersRegistrationServiceBean.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibServiceBean.java
Dismissed
Show dismissed
Hide dismissed
| * It is kept in the code for now, under the assumption that somebody | ||
| * may still have reasons to keep using the DiscoFeed-based model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * It is kept in the code for now, under the assumption that somebody | |
| * may still have reasons to keep using the DiscoFeed-based model. | |
| * It is kept in the code for now because the majority of Dataverse | |
| * installations using Shib are likely not using InCommon. |
In the release notes we say that if you're not using InCommon you should set dataverse.feature.shibboleth-use-discofeed=true. I'm pretty sure Harvard Dataverse and UNC Dataverse are the only ones using InCommon. All the other shib installations (of which there are many, I think) will operate in this non-InCommon mode (DiscoFeed-mode), as far as I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing out/clarifying this. This is totally on me, I asked which case was more common (it was an in-person conversation by the coffee machine, I think) - but I must have misunderstood your answer as the exact opposite of the above. ... It does make far more sense of course, that our use case is in fact fringe. I changed the feature flag etc. accordingly (to an optional dataverse.feature.shibboleth-use-wayfinder=true). Will update the release note and docs momentarily.
This comment has been minimized.
This comment has been minimized.
|
I'm going to mention this during standup, but note that I removed the 6.7 milestone from this last week. |
|
As of yesterday, the word I got was that we would get the answer on the R&S application "within 2 business days", i.e. by tomorrow. |
|
Oh wow, I haven't heard back from our contacts, but This looks very promising. I will update the "how to test" section in preparation for (maybe) finally being able to test and merge this thing. |
Resolved conflicts: src/main/java/edu/harvard/iq/dataverse/settings/FeatureFlags.java issue #11404
|
Working on fixing one bug I had reported on demo: redirects not working consistently. I.e., the user gets logged in successfully, but they not redirected to the proper destination page necessarily. |
Co-authored-by: Philip Durbin <[email protected]>
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
What this PR does / why we need it:
See the issue. In short, the shibboleth framework had to be reconfigured since InCommon has discontinued the metadata feed around which our current implementation was built. The new implementation is based on the recommended replacement parts - the MDQ protocol for the federation metadata and the WayFinder service for the login page authentication redirect.
Which issue(s) this PR closes:
Special notes for your reviewer:
I preserved the old, DiscoFeed-based implementation, in case an instance has a need to keep using it. I'm guessing a use case for this will be an installation that is not part of InCommon, that got their shib. auth. to work with a static XML metadata file. They cannot use, and have no need for switching to WayFinder, so they will be able to keep their old setup intact by setting one feature flag.
In order to make the WayFinder redirect, Dataverse needs to supply the entityID of itself as a Service Provider. In most cases this will be the
siteUrl + "/sp", but it's not guaranteed. One way of knowing this entityID would be to add a required configuration setting for it. Which I chose not to do in favor of calling shibd to retrieve it. One other option would be to just read it from the config file/etc/shibboleth/shibboleth2.xml, since shibd is almost certainly running on the same server. But I figured there may be some complicated load balancer situation where that is not the case. So I went to some trouble of making Dataverse call/Shibboleth.sso/Metadataonce, on startup, when the authentication provider is initiated, where it is then cached for shibservice to use. Feedback welcome about this implementation.Suggestions on how to test this:
A Dataverse instance with Shibboleth configured and properly registered with InCommon is required to test the functionality. I'm going to proceed with obtaining the registration for dataverse-internal, which hopefully will be done by the time the PR goes into QA. [this is still pending as of 05-27]
I will try to add more info here, on specific parts that need to be tested and confirmed.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation:
Preview at https://dataverse-guide--11502.org.readthedocs.build/en/11502/installation/shibboleth.html#identity-federation