Skip to content

Compass App: WIP Auth logic refactor #2394

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

Merged
merged 10 commits into from
Aug 27, 2024
Merged

Conversation

miquelbeltran
Copy link
Member

@miquelbeltran miquelbeltran commented Aug 22, 2024

This PR follows up with the conversation we had via email and cleans up the auth implementation.

New AuthRepository

The AuthTokenRepository, AuthLoginComponent and AuthLogoutComponent have been refactored into a single AuthRepository.

This AuthRepository exposes the isAuthenticated boolean, which is used by go_router to handle redirects.

Also, has two methods, login(email, pass) and logout(), similar to what we had in the components.

The implementation uses the new SharedPreferencesService, which is a service to access the shared_preferences plugin to store the String token.

New AuthApiClient

The ApiClient has been split in two. The /login REST call is moved to the AuthApiClient, while the other calls that require an auth token remain in the ApiClient.

ApiClient credentials

The main open question is, what is the best way to pass the auth token to the api client?

I've found several alternatives:

  1. ApiClient has a token setter which the AuthRepository calls to set the token
  • Implemented in this PR.
  • Follows our architecture design.
  • AuthRepository should be the source of truth, but the ApiClient.token could be set externally by mistake, so the single source of truth principle gets blurry.
  1. ApiClient calls to the AuthRepository to obtain the token.
  • This is what the code was doing before.
  • AuthRepository is the source of truth.
  • But the dependency "Service to Repository" breaks our architecture design principle.
  1. ApiClient calls to the SharedPreferencesService to obtain the token (
  • SharedPreferencesService is the source of truth, as the ApiClient doesn't need the AuthRepository.
  • We see a "Service calling a Service" which maybe breaks our architecture design principle?
  1. The ApiClient doesn't hold the token at all, intead, each call requires us to pass a token.
  • Tried to implement it but becomes very complex.
  • Makes other repositories depend on the AuthRepository or the SharedPreferencesService.

Personal take: I have implemented 1 in this PR because it is the one that follows best the architecture recommendations. However I see some pitfalls, e.g. the AuthRepository needs to fetch the token from the SharedPreferencesService at least once on app start and pass it to the ApiClient. While on the solution 2 that's never a problem, since the AuthRepository is being called from the ApiClient and so it is always the source of truth for authentication.

I am looking forward for your thoughts @ericwindmill

Pre-launch Checklist

  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I read the Contributors Guide.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-devrel channel on Discord.

@ericwindmill
Copy link
Contributor

This is interesting. I see the value of options 1, 2 and 3. Of those three, I like option 1 and 3 the best, but I'm not sure which is better.

Another option would be to have the ApiClient fetch the token from SharedPreferences itself when _authHeaders is called. I'm not sure how that affects something like performance, but if that is a consideration, the token could be cached for later uses. This option bend the rules of what a service is allowed to do, but it does seem to break the rules less than options the other options. What do you think?

No matter what, it seems like we have to decide which rules should be broken, which is fine. This is proof that these are just guidelines :)

@miquelbeltran
Copy link
Member Author

Another option would be to have the ApiClient fetch the token from SharedPreferences itself

In terms of performance would be like 3 indeed. The thing with it is that the shared_preferences becomes the source of truth.

One issue with making shared_preferences or SharedPreferencesServices the source of truth is that in a real life scenario, we would have to handle cases like expired token renewal and similar. So you have to ensure whatever caching you have in place, it needs to be handled correctly. That's where a Repository makes sense, and why I think the AuthRepository is the right place to handle tokens.


Rethinking a bit the current implementation in the PR, what if instead of calling to setToken() from AuthRepository, it sets an "auth provider" into the ApiClient, and the ApiClient calls to that auth provider (that internally is managed by the AuthRepository).

That way, we don't have a direct dependency ApiClient to AuthRepository but the AuthRepository is the source of truth.

That would be similar to what we can see here: https://learn.microsoft.com/en-us/entra/identity-platform/scenario-web-app-call-api-acquire-token?tabs=aspnetcore with the authorizationHeaderProvider.

@miquelbeltran
Copy link
Member Author

I pushed a change implementing an AuthHeaderProvider, this is I think my favorite solution, keeps the AuthRepository as the source of truth and most importantly doesn't break the dependency tree.

@miquelbeltran miquelbeltran marked this pull request as ready for review August 26, 2024 05:36
@ericwindmill
Copy link
Contributor

One issue with making shared_preferences or SharedPreferencesServices the source of truth is that in a real life scenario, we would have to handle cases like expired token renewal and similar. So you have to ensure whatever caching you have in place, it needs to be handled correctly. That's where a Repository makes sense, and why I think the AuthRepository is the right place to handle tokens.

You're right, SharedPref shouldn't be the source of truth.

I pushed a change implementing an AuthHeaderProvider, this is I think my favorite solution, keeps the AuthRepository as the source of truth and most importantly doesn't break the dependency tree.

I like this solution, I'm happy to call this complete.

@miquelbeltran miquelbeltran merged commit 3be6873 into compass-app Aug 27, 2024
1 check passed
@miquelbeltran miquelbeltran deleted the mb-refactor-auth branch August 27, 2024 11:24
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