search

Sage / omniauth-cognito-idp

OmniAuth Strategy for AWS Cognito in Ruby
Apache License 2.0
35 stars 13 forks source link

`build_access_token` clears all query params #5

Closed mkarklinsScout closed 4 years ago

mkarklinsScout commented 4 years ago

Hello!

First of all - thanks for open-sourcing this gem!

One thing I stumbled upon is this line. It seems that it removes all query params, but I suppose the intention was to only remove the code param?

If that is the case, would it be OK if I submitted a PR which removes just the code param?

For our use-case we need to have other query params preserved.

Offtopic: Did you plan on adding /userinfo endpoint in this gem, so that the Oauth2 flow is able to retrieve user attributes? Or is that not feasible due dependency on aws-sdk?

Thanks in advance!

tosch commented 4 years ago

Hi @mkarklinsScout , thanks for considering this gem and opening the first issue, that's very much appreciated!

Actually, the intention was to just use the path but no query params at all, as Cognito matches the full redirect url to the registered ones, so you cannot really use dynamic query params (apart from state anyway. You could, of course, register all the possible values of your query param(s), but then you could use a different sub-path as well. So, I'm not really against accepting the PR you suggest, but that would require the users to be extra careful about the URLs they register in Cognito as allowed callbacks. It would be really great if you could update the README with a section about the callback url configuration in that PR.

On the /userinfo topic: It would certainly possible to fetch information from there without having a hard dependency on the aws-sdk, but I'd still prefer to not implement that here. There already is some information in the JWT returned from Cognito and that is being made available. An additional API call is additional latency and I'd rather avoid that. So the short answer is: No, there are currently no plans to add this (but I also wouldn't reject a pull request adding this feature as long as it is optional). Do you think my concerns are valid?

mkarklinsScout commented 4 years ago

Thanks for you reply!

Our use-case changed a bit, so we no longer need to have additional query params in redirect url. Also, yes it seems that all of the necessary information is already present in the JWT tokens and additional call to /userinfo isn't necessary.

Anyway, thanks for taking the time to prepare your answer - it was helpful. Since there's nothing else to discuss, I'll close this issue.