-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update SAML docs, remove sample XML file #89
Conversation
Related to #87
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.
Tried to explain the proper workflow again, sorry if i wasn't clear before.
docs/saml-2-0.md
Outdated
## Service Provider Metadata XML | ||
|
||
To enable SAML 2.0 support, define the following options in the configuration file, and provide the SAML IdP Metadata in a file per environment: | ||
To generate the Service Provider metadata file from Altis, which is typically needed to setup the Identity provider application for each environment, visit [`/sso/metadata/`](site://sso/metadata/) on your application to download the metadata XML file. Collect these XML files for all environments and pass them to your colleagues responsible for the SSO integration setup. You should expect back an Identity Provider metadata XML file per environment. | ||
|
||
## Identity Provider Metadata XML | ||
|
||
To enable SAML 2.0 support, add the IdP metadata XML files to your project's `.config/sso/` directory (you may need to create the directory first). | ||
|
||
By default, Altis looks for `.config/sso/saml-idp-metadata-%ENVIRONMENT%.xml` where `%ENVIRONMENT%` is one of `local`, `development`, `staging`, or `production`, and falls back to `.config/sso/saml-idp-metadata.xml`. Make sure there are no XML formatting errors or leading whitespace. | ||
|
||
Lastly define the following option in your Altis configuration: |
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.
@roborourke IdP generation needs values from the application, in this case Altis, to be able to generate the IdP application on the provider. Previously, and why we actually have this bug, we added a sample IdP config ( dummy / local / testing ) so we can generate the SP file ( otherwise php-saml won't be able to generate the SP file and chokes with an error ), then replaced the dummy IdP file with the proper one from the provider. But in retrospective, this is not the correct way to do it, and was only because the provider manager (individual) at the time of development of the plugin knew what to put in while creating the IdP application, which is not the case for our clients.
What needs to be changed now is a way to surface those values, that the provider needed from the SP file, somewhere else, so we don't have to go through using the dummy IdP file and the process explained above. Then the process would be:
- Altis spits out the values needed somewhere, or the client follows docs that explain how the values are formed, eg: https://sites.com/sso/auth , etc.
- Client uses these info to create a new application profile at the provider, eg OKTA.
- Client takes the IdP file generated by the provider for that application profile, adds it to Altis.
Which means the SP file is not used at all in the standard setup flow.
That's what seems to be the proper flow from https://developer.okta.com/docs/guides/sign-into-web-app/aspnet/create-okta-application/
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.
Bit of a circular requirement! I see what we need now.
We could document all the endpoints here for one thing.
Additionally we could catch the Invalid SSO settings error in the wp-simple-saml plugin and provide the required output in some other format or show the endpoints in another format.
It does look like the /sso/metadata
output could be derived without having an existing IdP metadata XML file though. Is that 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.
Joe recently needed the abstract values, so I think just the documentation would be enough, no need to mock the SP output, wouldn't be future proof.
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.
Looks like we could avoid returning early here if it's the metadata endpoint:
https://github.com/humanmade/wp-simple-saml/blob/master/inc/admin/namespace.php#L71-L75
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.
Ok no we couldn't 🤦♂️
Thanks for explaining it fully - I get the process better now. I've just documented the endpoints and simplified it down.
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.
That looks a lot better! Concerned that the SLS endpoint would lead people to think the feature is currently working, but it isn't AFAIK 🤔
Ah, I see there's this issue humanmade/wp-simple-saml#5 I could remove that for now I suppose, or if it comes up in support use some support time to fix it. |
Related to #87