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

add PKCE Oauth2 flow #325

Merged
merged 9 commits into from
Feb 11, 2023
Merged

add PKCE Oauth2 flow #325

merged 9 commits into from
Feb 11, 2023

Conversation

rgammans
Copy link
Contributor

@rgammans rgammans commented Mar 7, 2022

I've finally gotten around to cleaning this up and adding some documentation, tests etc.

Fixes: #316

Copy link
Collaborator

@schinckel schinckel left a comment

Choose a reason for hiding this comment

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

I don't know enough about the PCKE auth flow to say whether this is "the best way to do it", but it mostly looks okay.

There are a bunch of flake8/style fixes I'd like to see fixed - spaces, trailing commas and the like.

tests/auth.py Outdated Show resolved Hide resolved
tests/auth.py Outdated Show resolved Hide resolved
tests/auth.py Outdated Show resolved Hide resolved
tests/auth.py Outdated Show resolved Hide resolved
@rgammans
Copy link
Contributor Author

rgammans commented Mar 8, 2022

I've updated it to lint cleanly with flake8.

As to whether it's the best approach, it's hard to say, but there are a good number of places a user can customise it if necessary. And it's easy to use in the simple cases.

But without knowing more use cases it's difficult to write documentation with advice for them.

@rgammans rgammans requested a review from schinckel March 8, 2022 20:39
@rgammans
Copy link
Contributor Author

rgammans commented Mar 9, 2022

Hmm. I'll probably need to run isort, but master isn't clean against isort 5.10.1 either.

@rgammans
Copy link
Contributor Author

rgammans commented Mar 9, 2022

Ah, I misspoke about isort cleanliness it turns out there are some config updates

Copy link
Owner

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review; I've done a light review pass on the docs, but otherwise, seems reasonable to me. Thanks for the contribution!

@freakboy3742 freakboy3742 merged commit e205de5 into freakboy3742:main Feb 11, 2023
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.

PKCE Authentication
3 participants