fief-dev / fief

Users and authentication management SaaS
https://www.fief.dev
Other
538 stars 44 forks source link

Fix for https://github.com/fief-dev/fief/issues/194 #211

Closed Kh-Oleg closed 1 year ago

Kh-Oleg commented 1 year ago

This commit adds 1 second tolerance to the fief_authorization_codes.expires_at value. In the logs I see that the value expires_at, stored in fief_authorization_codes table, is sometimes a few milliseconds behind 'now'. Example:

Kh-Oleg commented 1 year ago

With this fix, the authentication in Dashboard goes successfully, but the request from web service to /api/token returns a JWT, which fails timestamp check in

m: "exp" claim timestamp check failed
    at ye (https://unpkg.com/@fief/fief@0.12.3/build/index.umd.js:1:29498)
    at we (https://unpkg.com/@fief/fief@0.12.3/build/index.umd.js:1:30106)
Kh-Oleg commented 1 year ago

I think, the better fix would be to modify Fief in a way, that it would write into fief_authorization_codes.expires_at the value datetime.now+1 second or datetime.now+5 seconds. Right now it writes simply datetime.now. This should fix expiration time of authorization codes in all use cases. I tried to achieve so, by adding attribute __lifetime_seconds__ to fief.models.AuthorizationCode class, but without success.

frankie567 commented 1 year ago

Hi @Kh-Oleg 👋

Thank you for the investigation, but I'm not sure I follow you entirely.

The authorization code is generated by default with a lifetime of 600 seconds (~10 minutes). Unless you customized this setting with a very short lifetime (like 1 second), I don't really see how you could end up in such a situation?

Kh-Oleg commented 1 year ago

Hmm... I found the root cause. I needed to add an URI to the client, and I used for that purpose PATCH /clients/{client_id}. I took the payload from the documentation, (see the picture), modified fields which I needed and left the others untouched.

This did affect the lifetimes, although was not intended. Maybe it worth to change the API in a way that default values in API will not change the existing lifetime setting?

I will close the bug and the PR.

image

frankie567 commented 1 year ago

Ha, that explains things!

Yep, the default value in OpenAPI is misleading here. I'll see how to improve this!

BTW, the update API supports partial payload, so you can just pass the properties you need to change and leave the others.