fastapi-sso icon indicating copy to clipboard operation
fastapi-sso copied to clipboard

Add documentation about redirect_uri

Open mluogh opened this issue 1 year ago • 5 comments

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

mluogh avatar Sep 14 '23 06:09 mluogh

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.

tomasvotava avatar Sep 14 '23 20:09 tomasvotava

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

mluogh avatar Sep 16 '23 23:09 mluogh

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.

tomasvotava avatar Sep 20 '23 19:09 tomasvotava

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.

mluogh avatar Sep 20 '23 19:09 mluogh

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 avatar Sep 20 '23 19:09 tomasvotava