-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Conversation
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 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 :) |
In terms of performance would be like 3 indeed. The thing with it is that the One issue with making Rethinking a bit the current implementation in the PR, what if instead of calling to That way, we don't have a direct dependency 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 |
I pushed a change implementing an |
You're right, SharedPref shouldn't be the source of truth.
I like this solution, I'm happy to call this complete. |
This PR follows up with the conversation we had via email and cleans up the auth implementation.
New
AuthRepository
The
AuthTokenRepository
,AuthLoginComponent
andAuthLogoutComponent
have been refactored into a singleAuthRepository
.This
AuthRepository
exposes theisAuthenticated
boolean, which is used bygo_router
to handle redirects.Also, has two methods,
login(email, pass)
andlogout()
, similar to what we had in the components.The implementation uses the new
SharedPreferencesService
, which is a service to access theshared_preferences
plugin to store theString
token.New
AuthApiClient
The
ApiClient
has been split in two. The/login
REST call is moved to theAuthApiClient
, while the other calls that require an auth token remain in theApiClient
.ApiClient
credentialsThe main open question is, what is the best way to pass the auth token to the api client?
I've found several alternatives:
ApiClient
has atoken
setter which theAuthRepository
calls to set the tokenAuthRepository
should be the source of truth, but theApiClient.token
could be set externally by mistake, so the single source of truth principle gets blurry.ApiClient
calls to theAuthRepository
to obtain the token.AuthRepository
is the source of truth.ApiClient
calls to theSharedPreferencesService
to obtain the token (SharedPreferencesService
is the source of truth, as theApiClient
doesn't need theAuthRepository
.ApiClient
doesn't hold the token at all, intead, each call requires us to pass a token.AuthRepository
or theSharedPreferencesService
.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 theSharedPreferencesService
at least once on app start and pass it to theApiClient
. While on the solution 2 that's never a problem, since theAuthRepository
is being called from theApiClient
and so it is always the source of truth for authentication.I am looking forward for your thoughts @ericwindmill
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-devrel channel on Discord.