Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Authentication – Refresh, persistence & API #681
base: main
Are you sure you want to change the base?
Authentication – Refresh, persistence & API #681
Changes from 21 commits
1f0adfa
af28e6e
17886fb
6a2ee35
a3f8cca
3f1af6b
a3a700d
b2614b7
986408a
f601c03
1637fc1
b9b5598
008a93e
eaf968a
458d7bb
9b71d61
b8aebcb
a54ceed
9a4fd81
c0f4c25
a5fa1a8
bc85910
2fa50e6
90bd841
8049a8b
3cdc724
f07289d
8f80afa
4f7000a
2731407
87d8060
ef596f9
09a97f1
5b96158
a316eb1
ba45925
6bb436f
bc1e8f9
2e2ebe2
d5a4ecb
d878504
4cfe501
644cef1
7edc942
f813565
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in the previous PR, please move this code to the class initializer. This ensures the class cannot exist without a configuration, making it less prone to invalid states and reducing the number of states the class needs to manage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay then, I moved setting the configurations to the initialization. However, I'm still not comfortable to make it nullable.
As explained in the PR's description: The public state of the client needs to indicate two non-usable states (not-configured and not-authenticate), so I saw that it's better to unify access through a non-optional type for both of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind explaining why is that please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Services using the client need to guard against the state of not being authenticated. They also need to guard against the case of the client not having been configured (if cloned by someone.) These are the two non-usable states of the client.
The client needs to expose the first state, so my first idea is to unify all states of the client (not-configured, non-authenticated & authenticated) through the
authenticationState
property. I favoured this over handling one state through nil checking and the other through a property.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, a lot of the vagueness here will be cleared once we get to integrating the new APIs along the normal flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, a client instance should be configured for a consumer to use it. If the consumer does not configure the client, no client will be available for use. A null client serves as a clear indication that the app is not configured for authentication. This approach aligns with best practices followed by many system APIs, where the API is configured during initialization to ensure it can be used effectively.
There are additional nuances to consider, such as handling invalid configurations. Ideally, verifying the configuration during initialization would be great, but I assume this isn't feasible due to the need for backend API requests. Therefore, late verification of the configuration is acceptable. However, when early validation is possible, we should prioritize it and not defer it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this method does. Can you explain please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the implementation below and got what it does. I believe we should reconsider whether this needs to be a public API. Similar to how saving the auth token is treated as an implementation detail, retrieving the auth token from a persistence store should also remain an implementation detail, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps naming needs to be reconsidered. The purpose of this is to restore and reevaluate the status. The full picture of this API is to be used by sync logic.
But your intuition is on point: this part is still not clear. The main remaining point is synchronization especially on startup, as this API is intended to be the base for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I’ve observed in other auth libraries is that they typically provide functionality to check
isAuthenticated
and performlogin
, similar to your implementation. In addition to that they often include either an explicitstart
method or implicitly initiate the auth process during the initialization of the service/client class. So, that would be my recommendation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is not thread-safe and needs protection. I recommend converting this class into an actor to ensure thread safety.