OAuth Support #680
OAuth Support #680
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #680 +/- ##
==========================================
- Coverage 40.92% 40.12% -0.81%
==========================================
Files 525 533 +8
Lines 20880 21282 +402
==========================================
- Hits 8546 8539 -7
- Misses 12334 12743 +409 ☔ View full report in Codecov by Sentry. |
| let client = AppAuthOAuthClient() | ||
| if let config = Constant.QuranOAuthAppConfigurations { | ||
| client.set(appConfiguration: config) | ||
| } |
There was a problem hiding this comment.
@mohamede1945 I thought of StartupLauncher for this.
There was a problem hiding this comment.
Actually, this approach makes more sense to me. However, I’d prefer not to create an OAuthClient at all, rather than creating one with a nil configuration. This way, it’s clear to the consumer that the app doesn’t have OAuth configured. I would also propagate this to the Profile service, ensuring the feature recognizes that there’s no Profile service and doesn’t display login controls.
|
|
||
| // MARK: Public | ||
|
|
||
| public func set(appConfiguration: OAuthAppConfiguration) { |
There was a problem hiding this comment.
Is it possible to remove this method and just have the configuration set in the init? This way we can make appConfiguration non-optional and remove the need for oauthClientHasNotBeenSet error.
| let client = AppAuthOAuthClient() | ||
| if let config = Constant.QuranOAuthAppConfigurations { | ||
| client.set(appConfiguration: config) | ||
| } |
There was a problem hiding this comment.
Actually, this approach makes more sense to me. However, I’d prefer not to create an OAuthClient at all, rather than creating one with a nil configuration. This way, it’s clear to the consumer that the app doesn’t have OAuth configured. I would also propagate this to the Profile service, ensuring the feature recognizes that there’s no Profile service and doesn’t display login controls.
| @@ -0,0 +1,123 @@ | |||
| // | |||
There was a problem hiding this comment.
It would be really really great to have some tests to validate the success cases and a couple of failure cases as well, please. This would not only help ensure the code works but also ensure it is testable as we add additional logic in the future, such as the state machine for automatically logging in a user at startup.
There was a problem hiding this comment.
Definitely this will be handled next inshaa Allah. I usually go test-first, but for this PR the full responsibilities of the auth client weren't clear yet. As things stand, AppAuthOAuthClient is just a wrapper for the library.
We have the following:
- Token refresh & persistance
- Signaling API availability
- Authenticating the API requests
- Signaling if OAuth is available (disables/removes the login option)
This structure is going to be the subject of the next PR, and with it, I'll resume the standard testing strategy.
There was a problem hiding this comment.
As for the comment on managing the configuration along with the availability of the OAuth client, I'll take that into account while I handle the responsibilities mentioned above.
The Profile service will be facing the features.
The OAuth client will be facing other data clients, which I expect them to be used by the domain services (bookmarks, notes ...).
The question here is where to delegate the responsibility of knowing of the API availability. I think out understanding of this will need to evolve as we handle the cases of data syncing.
Introduces the basic infra for authenticating the user to their Quran.com account, and the basic UI for login. Will be expanded upon in following tasks.
Data/OAuthClienthandles the actual request. This is the starting point to handle authenticating requests and refreshing the tokens. The persistance of the tokens is yet to be handled.Domain/QuranProfileServiceis the service counterpart. Will, probably, only expose the profile information.openid/AppAuth-iOSwas added as a dependency. Cuts down on much nuisance in OpenID communications.