Pulls/2.4/alternate acs #60
Conversation
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.
|
Thanks, but why did you alter the commits? |
|
Not sure what you mean by alter commits. I just merged pr |
|
The merge has created new commits that list you as the creator (although I'm listed as the author).
It might be nice to use either a fast-forward or a normal merge then, instead of unnecessarily rebasing. Rebasing messes with the history and looks like you've touched up everyone's commits. GitHub is an increasingly important tool used in recruitment - it can give negative impressions if it looks like someone's work has had to be redressed. My commits: Merged commits (made by you): It's less of a merge and more of a cherry-pick all commits onto the target head. |
|
This was likely caused by using the rebase merge option. I've disabled it for the repo now. @satrun77 please make sure to always use the merge commit option for merging PRs to retain the history and the original author's commit without changes. Thank you! |
|
For future, it also would be good to add documentation for the two new features added in this commit:
To be fair, the docs for the module are hardly complete, but there is a fair amount of info regarding the YML configuration: https://github.com/silverstripe/silverstripe-saml/blob/main/docs/en/developer.md#yaml-configuration |
Assertion Consumer Service is not an "only one" predetermined arrangement. It is only one at a time, of course, but e.g. subdomains are allowed for with SAML by defining alternative ACS urls at the identity provider, and provided with the authentication request from the service provider.
The Silverstripe SAML module however did not support providing alternative ACS urls as part of the request, and made assumptions based no this when trying to authenticate an authentication response from the IdP - always checking against the SP entity ID, defined as the base URL including protocol per the documentation.
The ability to define alternative ACS urls is now allowed for with a predefined list (via Silverstripe config) - although identity providers should reject undefined URLs this allows us to catch misconfiguration earlier and provides a more defined behaviour to developers.
In tandem to allowing other ACS urls, we must also account for then when assessing a response - the use of the SP.EntityID has been changed to the absolute base URL as found by Silverstripe, where mismatches in "allowed domains" should be caught by nature of differing from the response (causing a rejection) or not being selected from the list that forms the request.
A hook is introduced to allow for further or reconfiguration of the ACS base URL on a project level if it is needed.
The diff appears larger than changes due to the renaming of a variable to remove confusion between the SAML configuration that is being built, and the Silverstripe Config API that is used to obtain the data for it.