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

fix: use userinfo() with okta and don't try decoding a hash as json #2292

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Lewiscowles1986
Copy link

Description

This plainly doesn't work with Okta, and it is not so hard to get it to work with Okta... There is no custom security class, no hacks to rely on identity token, just using the metadata url, to do what it should.

Fixes #2291

ADDITIONAL INFORMATION

  • Has associated issue:
  • Is CRUD MVC related.
  • Is Auth, RBAC security related.
  • Changes the security db schema.
  • Introduces new feature
  • Removes existing feature

I Would love it if someone can cut a release once this merges, so that I can build a superset image based upon this.

@Lewiscowles1986 Lewiscowles1986 changed the title fix: use userinfo() with okta and don't try decoding as json fix: use userinfo() with okta and don't try decoding a hash as json Dec 7, 2024
@jtv8
Copy link

jtv8 commented Dec 16, 2024

Am not a contributor to this repo but by coincidence I've also been trying to trace this issue today.

I believe this PR would fix a non-obvious "gotcha" when configuring a provider with the name "okta" - it requires api_base_url to be supplied as well as server_metadata_url in remote_app. This should not be necessary as all the relevant endpoints are supplied by querying the server metadata.

In fact, it already works just fine if you name your provider something other than "okta"!

To continue down this train of thought - I think flask_appbuilder/security/manager.py could take better advantage of the fact that all these providers support OpenID Connect, which exists to standardize these interactions. It could benefit from some heavy refactoring to extract the common logic for retrieving data from the userinfo endpoint, which is done inconsistently throughout.

The mappings of the retrieved data to flask_appbuilder's user info values needn't be hardcoded either - some organizations might wish to customize them. It might be better to provide them as overridable defaults for each provider. Allowing the values to be extracted from the ID token claims (if present) could also prevent the need for a round trip to the userinfo endpoint altogether.

@Lewiscowles1986
Copy link
Author

@jtv8 while this feels related, the problem I am solving actually relates to the Apache Superset usage of this library, where additional conventions have been setup around this.

We've actually gone old-school and manually pulled and patched the manager.py in that superset to this, and it works. Technically changing from Okta to auth0 can make this work, but not when using the conventions of Apache Superset where client and server are lock-stepped.

If you look at the auth0 just below okta definition, we are essentially borrowing from another of Okta's services (Auth0) conventions, to get this to work.

Server metadata url is not required by the way, you could also define the user info endpoint url, but I like that I can set the base uri and the metadata url and be more or less done.

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

Successfully merging this pull request may close these issues.

Okta login is broken
2 participants