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(#80): AuthInterceptor implementation is incomplete #91

Conversation

nivisi
Copy link
Contributor

@nivisi nivisi commented Jan 6, 2023

Resolves #80


Introduced:

  • A new Authenticator class that is responsible for:
    • Making the tokens accessible by other classes such as interceptors. This class is the source of truth
    • Trying to reauthenticate the user. The new interceptor, ApiAuthInterceptor, is using Authenticator to reauth if needed.
  • A new ApiAuthInterceptor class. It is a rewritten version of the old interceptor — more readable and straightforward. This interceptor is a QueuedInterceptor now — meaning that, if we await for something, other request calls will wait until this call is resolved.

Both of the classes are covered with tests. If you think that not all the scenarios are covered — please let me know, I'll add those.

Also, we now use FlutterSecureStorage for storing the tokens. It is safer. BUT! This storage survives app uninstallation IIRC. So we should consider creating something that will detect fresh installation and clear the storage. Something like storing a value with the freshInstallation key in the shared prefs of the app.


Changes with build context and response errors are related to the freezed generator failing to build with material library imports..

@nivisi nivisi requested a review from johsoe January 6, 2023 09:31
@nivisi nivisi changed the title fix(/#80): AuthInterceptor implementation is incomplete fix(#80): AuthInterceptor implementation is incomplete Jan 6, 2023
lib/data/api/authenticator/authenticator.dart Outdated Show resolved Hide resolved
@@ -11,9 +12,21 @@ class AuthTokens with _$AuthTokens {
required String refreshToken,
required String tokenType,
required double expiresIn,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, do we ever get this tbh? Shouldn't it just be expiresAt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johsoe in all the projects I've worked on we used expiresIn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we use expiresAt on other projects?

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, we should probably remove expiresAt - we shouldn't have both

@utpal-barman utpal-barman marked this pull request as draft October 4, 2024 16:54
@utpal-barman utpal-barman deleted the branch ml-opensource:master October 22, 2024 12:55
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.

AuthInterceptor implementation is incomplete
3 participants