Authentication – Refresh, persistence & API

[mohannad-hassan]

This PR should complete all the functionalities needed to handle the infrastructure of authentication, bar the logout API. This should be handled when adding the details of the login and profile UI.

Main Changes

• Handling refreshing, persistence and authenticating APIs
• Restructuring the client to ease testing the public APIs.

Some Design Points

• I've decided to maintain the configurations setup, at least for now. 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.
• I expect that the we might need to run startup actions (eagerly, or lazily) related to data synchronization. Currently, one action is done to restore and refresh the authentication state.
• This is one point where design might evolve noticeably. The two previous points are expected to be revisited.

[codecov]

Codecov Report

Attention: Patch coverage is 60.09174% with 174 lines in your changes missing coverage. Please review.

Project coverage is 40.64%. Comparing base (d9fc366) to head (f813565).
Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
...enticationClient/Sources/AppAuthOAuthService.swift	0.00%	89 Missing ⚠️
...ata/AuthenticationClient/Sources/Persistence.swift	0.00%	58 Missing ⚠️
...tionClient/Sources/AuthentincationClientImpl.swift	81.60%	16 Missing ⚠️
...cationClient/Tests/AuthenticationClientTests.swift	97.38%	5 Missing ⚠️
...anProfileService/Sources/QuranProfileService.swift	0.00%	3 Missing ⚠️
...nticationClient/Sources/AuthenticationClient.swift	71.42%	2 Missing ⚠️
Features/SettingsFeature/SettingsBuilder.swift	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #681      +/-   ##
==========================================
- Coverage   40.92%   40.64%   -0.29%     
==========================================
  Files         525      536      +11     
  Lines       20880    21647     +767     
==========================================
+ Hits         8546     8799     +253     
- Misses      12334    12848     +514     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mohamede1945]

Jazak Allah khair, Mohannad!

I believe we need to refine the design of a few components to make them more reusable and minimize the chances of failure. Let me know if you have any questions or need clarification.

Thank you!

[mohamede1945]

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.

[mohannad-hassan]

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.

[mohamede1945]

The public state of the client needs to indicate two non-usable states (not-configured and not-authenticate)

Do you mind explaining why is that please?

[mohannad-hassan]

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.

[mohannad-hassan]

To be honest, a lot of the vagueness here will be cleared once we get to integrating the new APIs along the normal flow.

[mohamede1945]

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.

[mohamede1945]

I'm not sure what this method does. Can you explain please?

[mohamede1945]

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.

[mohannad-hassan]

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.

[mohamede1945]

What I’ve observed in other auth libraries is that they typically provide functionality to check isAuthenticated and perform login, similar to your implementation. In addition to that they often include either an explicit start method or implicitly initiate the auth process during the initialization of the service/client class. So, that would be my recommendation.

[mohamede1945]

I suggest creating a general SecurePersistence API (under Core/ directory) rather than one specifically for authentication.

A recommended pattern can be seen in this directory, where Preferences is implemented as a service, and consumers define PreferenceKey objects to describe the data that needs to be persisted. I think we can do something similar with SecurePersistence and SecurePersistenceKey.

Also please add a couple of tests for this class.

[mohamede1945]

I recommend minimizing the use of mocks, as they can make tests more dependent on the mock behavior than the actual application code. Instead, I recommend focusing on mocking only the third-party API for authentication and avoid creating mocks for the code we own. This approach ensures tests remain closely aligned with the application's real behavior.

[mohannad-hassan]

I'm actually of the opinion that one should avoid mocking 3rd party code. Not often that these would be open for subclassing or easy for subclassing. The class OIDAuthState of the library we're using proved to be so. It was open, but I'd have to mock four other types. Same issue prevented from stubbing it.

Moreover, wrapping 3rd party code under abstractions allows to define the interactions between the main logic and the kind of responsibility we seek from the 3rd party code. In other words, we're adapting the 3rd party code to our needs - as much as the situation allows without awkward design.

[mohannad-hassan]

As for depending on mocks, I understand that this is a concern, however, I try to counter that by keeping mocks as small as possible, and relying only on providing ready responses (call succeeds or fails, call returns something, ...)

I use the term 'mock' liberally, what I usually use are mostly stubs and spies.

I expounded on that, because these are key techniques I use (dealing with 3rd party and testing doubles strategies.) We can discuss that face-to-face if there's something to be aligned.

[mohamede1945]

Apologies, I’m not sure I fully understand your point. You mentioned wanting to avoid mocking but also referred to implementing small mocks.

To clarify my perspective: tests should avoid using mocks/fakes/spies/stubs unless absolutely necessary. These cases include scenarios where the actual dependency cannot be used in tests (e.g., backend APIs or time-based APIs) or where using a mock provides a significant benefit (e.g., file system operations that could slow tests due to extensive reading and writing).

If a mock is needed, it should be added to the following directory: Core/SystemDependencies. Additionally, a corresponding fake implementation should be placed in: Core/SystemDependenciesFake. This approach ensures that we explicitly allowlist the APIs for which their actual implementation cannot be used.

[mohannad-hassan]

You mentioned wanting to avoid mocking but also referred to implementing small mocks.

I avoid mocking 3rd party code; in our case, that would be keychain access functions and the AppAuth-iOS library. My solution for this would be wrapping such code behind an abstraction and providing a mock that extends that.

tests should avoid using mocks/fakes/spies/stubs unless absolutely necessary

I see. I largely agree. I usually mock things that represent complex logic and I'd think of mocking first for cross-package/cross-layer dependencies. But since this is the practice we're taking here, I'll adjust it.

For our case though, we have:

• Keychain access, wrapped by Persistence. I can see this fits well with SystemDependencies.
• The OAuth library, wrapped by OAuthCaller. Do you that sitting there?

[mohannad-hassan]

@mohamede1945
Here's an update:

• The most noticeable update is the redesign of the OAuth client part, that is used by AuthenticationClient. The OAuthService is now responsible for executing actions and updating state. The new data, or OAuthStateData should be a simple struct now.
• Extracted that part to a separate Core module. However, I'd like your input if anything is missing from the current mocks to be the kind of fakes we're using, before this is extracted out.
• Didn't extract Persistence out. It's now an internal type, so the API is fitted for that. I preferred not to put time into making it more general. Let me know what you think.

There is one or two points that I've put off for now:

• Defining an enum for the available scopes Quran.com has.
• Currently, OAuth-flow errors revert the client to a non-authenticated state. It might be needed to differentiate server failures and networking issues and other classes. I think it'll be better, in terms of its UX, not to request the user to login again in the first case.