Include features currently exclusive to CMS 4 support branch #63
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For e.g. subdomains that are served by the same Silverstripe installation. This could be subsites, translation domains (fluent), etc. Without the ability to accurately return traffic to the correct site after authentication, users are presented with a confusing and frustrating experience. At worst they get locked out of their sites (with SAMLMiddleware), as although AuthN completes successfully, the domain that carries the session is (by default) neither correct nor shared.
As a work-around for a problem where ACS was read as e.g. domain/acs instead of domain/path/acs, explicitly setting the path (via the base url) was done. But we need the domain to match the actual loaded domain, not the service provider entity ID. It is important since there is already another major version of this saml module that no changes that would require existing users to make changes to their code be introduced (requiring a major version). This is the reason we're now relying on Director::absoluteBaseUrl instead of letting the SAML library do what it does (in cases where the workaround is not needed). When using Director::absoluteBaseUrl instead of letting the SAML library build it's own interpretation, the use of other Util class options become redundant - they're not used anyway, so there is no point in having any further/alternative options. A hook point is introduced to further manipulate things like that should anyone have the need.
|
LGTM, and sorry for the divergence @NightJar! As you say in your other issue, it's worth getting fixed up with the retirement of SS4 soon as well, so v2 can become legacy for SSv4, and we move on in the |
As I've just noticed they weren't linked, the other issue was #61 :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ability to set alternative ACS domains was submitted to the CMS 4 support branch with the intention for it to be merged up. However the mainenance has diverged the branches, so this is a cherry-pick for feature parity instead.