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

Authorization Refresh #3

Merged
merged 1 commit into from
Oct 12, 2024
Merged

Authorization Refresh #3

merged 1 commit into from
Oct 12, 2024

Conversation

0xifarouk
Copy link
Contributor

Currently if the token is expired, every request after it fails and there is no way of refreshing it since it's being done on the client init.
this PR introduces better authorization handling, with the following changes:

  • token is no longer being fetched on init, because you usually create the client on the server start, you want that to be as fast as possible (you don't want to make the server start waits for a network call to finish).
  • token is fetched when needed if it's not there.
  • token is refreshed automatically if it's expired 10 seconds before it's actual expiry, so the client requests don't get un-authorized).
  • only one token refresh happens at the same time, if multiple requests are trying to access the token while it's being refreshed, all of them wait for that single refresh and take it's result.

The refresh flow is inspired by this article:
Building a token refresh flow with async/await and Swift Concurrency

Copy link
Owner

@fpseverino fpseverino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again! This was something that was on my todo list

I haven't set up CI yet, so could you execute

swift format --in-place --recursive --parallel .

to format the files?

extension TokenResponse {

/// A boolean indicates weather to token is valid 10 seconds before it's actual expiry time.
var isValid: Bool {
Copy link
Owner

@fpseverino fpseverino Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't ever become false, as the Date.now is new every time and expiresInSeconds doesn't get updated since the response is received.

Maybe we can add a private expirationDate property to TokenResponse to which we assign Date.now.addingTimeInterval(TimeInterval(expiresInSeconds)) when the TokenResponse gets initialized

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's a very good point, sorry for that.
I will push a fix and write a test for it.

@0xifarouk
Copy link
Contributor Author

@fpseverino I fixed it, and added a test against it.
But now tests takes 12 seconds to run.
I tried to use a TimeTravel concept to adjust the date using a date generator, but it made the code not understandable from TokenResponse side.

if you have some idea on how to improve it let me know.

@0xifarouk
Copy link
Contributor Author

I improved it, and now tests take 2 seconds.

Copy link
Owner

@fpseverino fpseverino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I'll add CI, run swift-format and update the DocC, then I'll release a new version.
Thanks again!

@fpseverino fpseverino merged commit 2114c3e into fpseverino:main Oct 12, 2024
@0xifarouk
Copy link
Contributor Author

Thank you 🙏🏻

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