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

Errors in documentation, no migration path for pre v1.0 users #43

Open
jrial opened this issue Jan 22, 2021 · 2 comments
Open

Errors in documentation, no migration path for pre v1.0 users #43

jrial opened this issue Jan 22, 2021 · 2 comments

Comments

@jrial
Copy link

jrial commented Jan 22, 2021

The documentation (the README in the project root) contains a few errors and omissions:

  • It doesn't mention that one needs to add saml2_pro_auth to the INSTALLED_APPS list
  • It refers to the function_urls, which no longer exist

This second item is causing an issue for people migrating from pre 1.0 to post 1.0: the class-based views have changed the SAML URLs, which means the SSO provider must update their settings to match the new URLs. In some organisations this can be a bit problematic due to slow formal ITIL procedures.

Not sure if this is a bug, or simply not or badly documented, but I would have guessed that setting the SAML_ROUTE setting, and including the URLs for ACS and SLS under the SAML_PROVIDERS dictionary, would result in the system using the same URLs as previously under the function-based views. This doesn't seem to work. I already had them in my settings, and they remain unchanged. What's worse: the new URLs don't seem to work either. They either result in a 404 or a 500. Before I file a separate bug for that, can someone from the team enlighten me as to whether these settings should in fact override the URLs for the class-based views as well?

@shepdelacreme
Copy link
Member

Apologies for the documentation errors, SAML_ROUTE is no longer used since it makes more sense for users of this package to just specify a route prefix in their own URL patterns.

path("sso/saml/", include((saml_urls), namespace="saml")),

or more simply:

path("sso/saml/", include("saml2_pro_auth.urls")),

Unfortunately, the old function_urls didn't fit with the substantial changes we needed to make in v1.0 to make this package work for our use cases internally. The rewrite of the views and some of the other fundamental changes we needed to make are why this was cut as a new major release. If you need support for the old function_urls or you would prefer querystring parameters for specifying provider or acs those could likely be implemented by inheriting and overriding some of the class-based view set up with a different set of URLs in your app.

I would also accept and consider any PR that mimics the old URL scheme as long as it doesn't impact complexity much further.

Lastly, if you are receiving 404 or 500's for the new views, please submit a ticket with as much information as you can provide and we will take a look, but it does sound like it is likely a configuration mismatch in your app or the IdP.

@jrial
Copy link
Author

jrial commented Jan 28, 2021

Hey, thanks for the response. No need to assist me with anything; as long as you're aware of the errors in the documentation, I'm good.

On my end, I won't be debugging this further. For now I'm going to stick with the previous version in use, as that one worked. It just lacked one feature of 1.0 which I can achieve through different means.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants