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

docs: Oauth_improve #17546

Closed
wants to merge 2 commits into from
Closed

Conversation

alejandroparra
Copy link

@alejandroparra alejandroparra commented Nov 24, 2021

SUMMARY

Hi there!
I was trying to set up the Custom OAuth2 Configuration but found out that one very important and necessary import is missing from the documentation in order to make the sample code work without any problem.

the import missing is with the library OAuth, after several hours of searching and reading found that importing only that library doesn't work, what it works is to import the full OAuth library from flask_appbuilder.security.manager.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Documentation update for OAuth configuration #17544
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Hi there!
I was trying to set up the Custom OAuth2 Configuration but found out that one very important and necessary import is missing from the documentation in order to make the sample code work without any problem.

the import missing is with the library OAuth, after several hours of searching and reading found that importing only that library doesn't work, what it works is to import the full OAuth library from flask_appbuilder.security.manager.
@villebro
Copy link
Member

Thanks for the contribution @alejandroparra - Looks good, but I tagged Daniel for a review, as he has the most context on FAB.

@villebro villebro changed the title docs:Oauth_improve docs: Oauth_improve Nov 25, 2021
@alejandroparra
Copy link
Author

thank you @villebro, is there something else that needs to be done on my end?

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, this looks good to me. left a non blocking comment, also would you be so kind to update the rest of the example code regarding single quoted and double quotes?

AUTH_REMOTE_USER,
AUTH_DB,
AUTH_LDAP,
AUTH_OAUTH,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can just import this one AUTH_OAUTH

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @dpgaspar thanks for your answer, actually this is exactly the point of my request, I've spent an entire day trying to make the SSO with Google work and all I had to do was add the rest of the libraries in order to make it work, so as is, the current documentation will fail in step to step implementation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not make sense to me, why does it fail? do you have the crash log?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't give a crash log, it just doesn't work, I was trying the OAUTH with google, and didn't authenticate until I added the rest of the dependencies. I've spent about 10 days on that

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final pending question

@rusackas
Copy link
Member

If I'm understanding correctly, this PR is hung up over a nit. Can we just resolve the conflicts and merge, or does approval need to be rescinded and more actions taken?

@dpgaspar
Copy link
Member

dpgaspar commented Jul 7, 2022

@alejandroparra thank you for the PR, I'm salvaging your contribution on #20640

Just doubled checked FAB by just importing AUTH_OAUTH and found no problems authenticating with Google for example. Feel free to open an issue on FAB or Superset if you need any help

@dpgaspar dpgaspar closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants