ueberauth / guardian

Elixir Authentication
MIT License
3.43k stars 382 forks source link

Issuing different jwt tokesn not possible #599

Closed Sgoettschkes closed 5 years ago

Sgoettschkes commented 5 years ago

I'm trying to use guardian to generate multiple jwt tokens. The use case is that we have an existing authentication provider we want to deprecate so the new one should generate a new jwt token and a legacy one which will be removed after all applications are using the new token.

The new token will be using "RS512" with a public/private key and it's working as expected. The legacy token uses "HS256". This is the code:

AuthenticationService.Guardian.encode_and_sign(user, %{},
  ttl: {42, :minutes},
  secret_fetcher: Guardian.Token.Jwt.SecretFetcher,
  secret: "secret",
  allowed_algos: ["HS256"]
)

As shown, I'm overwriting the fetcher to not use the public/private key fetcher, use our old secret and also changed the allowed algorithm.

I'm getting the the error(ErlangError) Erlang error: {:not_supported, [:HS256]} and it seems that the optional arguments are overwritten by config values.

I'm under the impression that I'm doing something wrong but I can't seem to figure it out!

Sgoettschkes commented 5 years ago

As a follow up, if I exchange the config to support the "HS256" algorithm, everything works fine. I looked through the source and from what I can see the fetch_secret_fetcher function https://github.com/ueberauth/guardian/blob/5663cfa94bdabe33474accb7b49052ca12ea4ea5/lib/guardian/token/jwt.ex#L510-L512 only reads from the config, never from the opts, ignoring the option.

Could this be the issue?

yordis commented 5 years ago

@Sgoettschkes make a PR with your proposal on how to fix the current issue. it will help us to understand exactly what needs to change.

Hanspagh commented 5 years ago

@Sgoettschkes I am not sure it was ever the intention to be able to pass the secret_fetcher as a optional param to decode_and_sign. I was thinking that creating two Guardian config would fix this issue for you. Basicly would have one config what is your old token issuer and one for the new private public key one. Having them in two modules also makes it a bit more easy to maintain and document. What do you think?

Sgoettschkes commented 5 years ago

Maybe I misunderstood the docs. From the docblocks I got

opts - The options to pass to the token module and callbacks

and assumed you could overwrite the config options.

I can see how two different configs would solve my problem and will try that. Would you be interested in me adding a few sentences of documentation for this use case?

Hanspagh commented 5 years ago

Yea, I can see why I can be a bit misleading. Feel very welcome contribute to make it more clear

On Tue, 16 Jul 2019, 10.37 Sebastian, notifications@github.com wrote:

Maybe I misunderstood the docs. From the docblocks I got

opts - The options to pass to the token module and callbacks

and assumed you could overwrite the config options.

I can see how two different configs would solve my problem and will try that. Would you be interested in me adding a few sentences of documentation for this use case?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ueberauth/guardian/issues/599?email_source=notifications&email_token=AAH2DIANVS6HB7TC3NFONG3P7WCEVA5CNFSM4ICKSNTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2AEGVA#issuecomment-511722324, or mute the thread https://github.com/notifications/unsubscribe-auth/AAH2DIDADT6EUADB67YWMKDP7WCEVANCNFSM4ICKSNTA .

Sgoettschkes commented 5 years ago

Just to clarify: Would you recommend to have one config and Guardian implementation for each token, even if they share the key? I'm asking because technically, we have 3 different tokens:

I just looked at guardian_db and it seems that in order to use the lib for the refresh token but not the access token a different implementation of the guardian module is needed.

Hanspagh commented 5 years ago

Sorry for the late reply. To keep things simple I would have one config per secret or secret fetcher, no need to make a additional config for your refresh token.

Hanspagh commented 5 years ago

@Sgoettschkes any update on this?

yordis commented 5 years ago

Feel free to reopen the issue, if you would like to continue the discussion