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 documentation about redirect_uri #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mluogh
Copy link

@mluogh mluogh commented Sep 14, 2023

It was unclear to us that we needed it in verify_and_process and led to some debugging.

@tomasvotava
Copy link
Owner

Hi @mluogh and thanks for you PR, however, I believe it's now quite correct - verify_and_process does not accept redirect_uri. Also, you are passing it the callback uri from within the callback itself.

I believe it should be:

@api.get("/login")
async def login(request: Request):
 return sso.get_login_redirect(request.url_for("callback"))

@api.get("/callback")
async def callback(request: Request):
 ...

Or something similar. Would you be up to updating the PR to reflect the actual usage or should I take care of it?

Either way, thanks again for bringing this up, my understanding is that fastapi-sso's documentation is not very well written nor extensive.

@mluogh
Copy link
Author

mluogh commented Sep 16, 2023

verify_and_process takes redirect_uri here, doesn't it?

https://github.com/tomasvotava/fastapi-sso/blob/master/fastapi_sso/sso/base.py#L195

I believe putting verify_and_process in the callback itself is the correct thing since putting it here means it will eventually be propagated to prepare_token_request here.

https://github.com/tomasvotava/fastapi-sso/blob/master/fastapi_sso/sso/base.py#L271

@tomasvotava
Copy link
Owner

Well, would you believe how little I know my own package?

You are right, but this only applies after you had already passed the same redirect_uri to the get_login_redirect method. Wouldn't it make more sense to remember the redirect uri from get_login_redirect and not require it again in verify_and_process? I could do that.

@mluogh
Copy link
Author

mluogh commented Sep 20, 2023

That would be wonderful and makes the most sense to me.

If you're busy, I could also send you a PR for that since I have a little bit of downtime.

@tomasvotava
Copy link
Owner

It would be awesome if you could do this ❤️ Could you please also wait for #60? I'll merge it in a minute, I'd like the new PR to be already using codecov.

@tomasvotava tomasvotava added the documentation Improvements or additions to documentation label Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants