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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

Kh-Oleg
Copy link

@Kh-Oleg Kh-Oleg commented Jun 6, 2023

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:

  • expires_at: 2023-06-06 14:33:29.591862
  • now: 2023-06-06 14:33:29.824840
    Because of that get_valid_by_code() returns None and authorization fails.

I don't know why, but this is reproducible on my side only after I access /clients/{client_id} REST API.

In the logs I see that the value expires_at, stored in fief_authorization_codes table, sometimes is a few milliseconds behind 'now'. Because of that get_valid_by_code returns None and authorization fails.
@Kh-Oleg Kh-Oleg marked this pull request as draft June 6, 2023 15:43
@Kh-Oleg
Copy link
Author

Kh-Oleg commented Jun 6, 2023

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/[email protected]/build/index.umd.js:1:29498)
    at we (https://unpkg.com/@fief/[email protected]/build/index.umd.js:1:30106)

@Kh-Oleg
Copy link
Author

Kh-Oleg commented Jun 6, 2023

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
Copy link
Member

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
Copy link
Author

Kh-Oleg commented Jun 7, 2023

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

@Kh-Oleg Kh-Oleg closed this Jun 7, 2023
@frankie567
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants