Refresh Tokens #5023

langleyd · 2022-01-21T13:33:49Z

Resolves #4943

What's in the PR

Hardcodes refresh_tokens on for the moment
Refresh tokens requested during login
Refresh tokens and access token expiry parsed in login response
Access Token provider synchronises refresh so that we have a leader refresh requests while other requests are blocked and received the new access token when the leader succeeds.
Adds refresh task for unauthenticated refresh api.
Added interceptor logic may be trigger a refresh before a request if it is deemed the toke is no longer valid(e.g. expired). But also triggers a refresh on the a 401 UNKOWN_TOKEN from the server(the source of truth on token validity).
Handles cases of both access token auth/refresh token auth becoming unauthenticated and logs to analytics for monitoring.

#TODO

Verify we don't need to handle multiprocess
Add config to enable/disable refresh tokens and test both enabled/disabled
Test sequential login/logouts
That soft/hard logouts work and that clients gracefully logs out when either refresh token or access token based auth become unauthenticated.
Add analytics event for UnauthenticatedError
Open issue for separate bug found: Soft Logout when LoginMode is SSOAndPassword results in an empty space where a login UI component should be displayed. #5192

- Hardcodes refresh_tokens on for the moment - Refresh tokens requested during login - Refresh tokens and access token expiry parsed in login response - Access Token provider synchronises refresh so that we have a leader refresh requests while other requests are blocked and recieve the new access token when the leader succeeds. - Adds refresh wizard to authentication service for unauthed refresh api.

github-actions · 2022-01-21T14:28:33Z

Unit Test Results

  84 files ±0   84 suites ±0 1m 0s ⏱️ -1s
157 tests ±0 157 ✔️ ±0 0 💤 ±0 0 ❌ ±0
504 runs ±0 504 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 1d5b19e. ± Comparison against base commit 79b511b.

♻️ This comment has been updated with latest results.

bmarty

Nice work. I've made a few remarks.

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/AuthenticationService.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/data/Credentials.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/#/#Wizard.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/refresh/RefreshWizard.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/AuthAPI.kt

.../src/main/java/org/matrix/android/sdk/internal/network/parsing/CredentialsResponseAdapter.kt

...src/main/java/org/matrix/android/sdk/internal/network/token/HomeserverAccessTokenProvider.kt

github-actions · 2022-01-21T14:44:05Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed=
[org.matrix.android.sdk.account]
passed=
[org.matrix.android.sdk.internal]
passed=
[org.matrix.android.sdk.ordering]
passed=
[org.matrix.android.sdk.PermalinkParserTest]
passed=

…eature/dla/refresh_tokens

…ase. enable refresh tokens.

…identity.

vector/src/main/java/im/vector/app/features/signout/soft/SoftLogoutActivity.kt

bmarty

Some more comments, please let me know what you think.
Sorry this is a very sensitive subject, I want to be sure that there is no issue and the implementation is crystal clear.

...x-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/#/DefaultLoginWizard.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/data/RefreshResult.kt

bmarty · 2022-02-14T14:16:12Z

.../src/main/java/org/matrix/android/sdk/internal/network/parsing/CredentialsResponseAdapter.kt

+            return responseCredentials
+        }
+        // We have received a credential response from the server, estimate the expiry datetime.
+        return responseCredentials.copy(expiryTs = System.currentTimeMillis() + responseCredentials.expiresInMs)


I understand reading this code again that expiryTs does not come from the server, do it?
This a bit weird to create a customized Json adapter to handle this.
Not sure how to implement it in another way, I will think more about it.

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/SessionModule.kt

...rc/main/java/org/matrix/android/sdk/internal/session/identity/IdentityAccessTokenProvider.kt

...sdk-android/src/main/java/org/matrix/android/sdk/internal/session/identity/IdentityModule.kt

vector/src/main/java/im/vector/app/features/signout/soft/SoftLogoutActivity.kt

…eature/dla/refresh_tokens

github-actions · 2022-02-21T12:05:47Z

Ktlint Results

👎Failed
matrix-sdk-android 🔸 internal/auth/#/DefaultLoginWizard.kt:20:1: Unused import (no-unused-imports)

bmarty

Some more remarks.

@ganfra can you have a quick look on this PR please? especially the refresh token management itself.

...x-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/#/DefaultLoginWizard.kt

bmarty · 2022-02-21T16:41:33Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/SessionModule.kt

                @Unauthenticated okHttpClient: Lazy<OkHttpClient>,
                retrofitFactory: RetrofitFactory,
                homeServerConnectionConfig: HomeServerConnectionConfig
-        ): AuthAPI {
+        ): RefreshTokenAPI {


Can you have retrofit: Retrofit as the only parameter?

The okHttpClient here is unauthenticated. Would I have to provide another unauthenticated version of Retrofit? is there any example of where we do that?

Is it a problem to send the current (and outdated) access token in the request header to refresh the token?

I think this is not a problem. (but I have done no test)

To me, it should even be mandatory server side to provide the previous access token to be able to get a new token.

If this is not a problem, I would remove this change on the SessionModule

Well according to the spec

This new API should be rate-limited and does not require authentication since only the refresh_token parameter is needed.

So it's not needed but I don't imagine the access token itself would cause a problem.
However the authenticated requests(which /refresh would bcome) run through the AccessTokenInterceptor and HomeserverAccessTokenProvider which is itself making the request to refresh. I think you might have to add additional defensive code there to check that the current request is not /refresh. Seems a more sensible design to keep the /refresh endpoint outside of that code flow.

I see your (good) point about potential endless loop

Yeah you can keep it like that it's ok

…eature/dla/refresh_tokens

bmarty · 2022-03-10T16:24:20Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/data/RefreshResult.kt

+import com.squareup.moshi.JsonClass
+
+@JsonClass(generateAdapter = true)
+data class RefreshResult(


Could be internal

ganfra

Some thoughts

ganfra · 2022-03-10T17:05:59Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/auth/AuthenticationService.kt

     */
-    fun getLoginWizard(): LoginWizard
+    fun getLoginWizard(enableRefreshTokenAuth: Boolean = false): LoginWizard


Should this flag enabled on MatrixConfiguration instead? It would avoid passing this everywhere and directly be injected in the few places where it makes sense.

I actually had it that way initially. @bmarty felt it would be better to be a more explicit part of the api on the AuthenticationService vs a config people might not know about. I don't really have a strong opinion on it. :)

ganfra · 2022-03-10T17:36:42Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/SessionModule.kt

                @Unauthenticated okHttpClient: Lazy<OkHttpClient>,
                retrofitFactory: RetrofitFactory,
                homeServerConnectionConfig: HomeServerConnectionConfig
-        ): AuthAPI {
+        ): RefreshTokenAPI {


Yeah you can keep it like that it's ok

ganfra · 2022-03-10T17:51:12Z

...src/main/java/org/matrix/android/sdk/internal/network/token/HomeserverAccessTokenProvider.kt

-        private val sessionParamsStore: SessionParamsStore
+        private val sessionParamsStore: SessionParamsStore,
+        private val refreshTokenTask: RefreshTokenTask,
+        private val globalErrorReceiver: GlobalErrorReceiver
 ) : AccessTokenProvider {


Okhttp has an API to handle accessToken and refreshToken when needed, might be the opportunity to use it instead of a custom interceptor/provider ?
See this article: https://www.lordcodes.com/articles/authorization-of-web-requests-for-okhttp-and-retrofit for example

bmarty · 2022-10-04T13:35:38Z

Will have to be done again, if we want to support this using https://www.lordcodes.com/articles/authorization-of-web-requests-for-okhttp-and-retrofit.

I guess this is probably better to create another PR.

langleyd added 3 commits January 21, 2022 13:01

Fix copyright and add changelog

c8d2682

lint

e3ad5ff

langleyd marked this pull request as draft January 21, 2022 13:52

Moar lint. Local lint not working so well :/

cb31c2c

bmarty requested changes Jan 21, 2022

View reviewed changes

langleyd added 8 commits February 3, 2022 12:13

Merge branch 'develop' of github.com:vector-im/element-android into f…

6651120

…eature/dla/refresh_tokens

Address comments, add MatrixConfig, add global error handling.

c0e4edf

Merge branch 'develop' of github.com:vector-im/element-android into f…

74669da

…eature/dla/refresh_tokens

Peek error body in interceptor to not consume it. Fix refresh catch c…

d086a32

…ase. enable refresh tokens.

Add analytics for UnauthenticatedError and BuildConfig

e6f6883

Rename refreshToken boolean to enableRefreshTokenAuth

9fa5722

lint

d29a0dc

lint

b3ccbdf

langleyd self-assigned this Feb 8, 2022

langleyd added 2 commits February 9, 2022 09:39

Cleanup and only use interceptor retry logic for homeserver auth not …

4b1a3dc

…identity.

Add explicit conversion to Long.

b03d1a6

langleyd marked this pull request as ready for review February 9, 2022 13:16

langleyd requested a review from bmarty February 9, 2022 13:16

langleyd added 2 commits February 9, 2022 14:04

Remove error testing code.

8831748

Specify user is logged out when clearing data from SoftLgoutActivity.

87201cc

langleyd commented Feb 9, 2022

View reviewed changes

vector/src/main/java/im/vector/app/features/signout/soft/SoftLogoutActivity.kt Show resolved Hide resolved

bmarty reviewed Feb 14, 2022

View reviewed changes

Add refreshTokenAuth api to public interface. Cleanup.

ed4606b

langleyd requested a review from bmarty February 18, 2022 16:21

Merge branch 'develop' of github.com:vector-im/element-android into f…

dc43479

…eature/dla/refresh_tokens

bmarty requested changes Feb 21, 2022

View reviewed changes

bmarty requested a review from ganfra February 21, 2022 16:42

langleyd added 3 commits February 22, 2022 16:01

lint

a34a7be

Merge branch 'develop' of github.com:vector-im/element-android into f…

de24d73

…eature/dla/refresh_tokens

fix copyright

1d5b19e

bmarty reviewed Mar 10, 2022

View reviewed changes

ganfra reviewed Mar 10, 2022

View reviewed changes

ouchadam mentioned this pull request Apr 12, 2022

PoC - Staleness action for Element team PRs #5742

Closed

bmarty closed this Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh Tokens #5023

Refresh Tokens #5023

langleyd commented Jan 21, 2022 •

edited

Loading

github-actions bot commented Jan 21, 2022 •

edited

Loading

bmarty left a comment

github-actions bot commented Jan 21, 2022 •

edited

Loading

bmarty left a comment

bmarty Feb 14, 2022

github-actions bot commented Feb 21, 2022 •

edited

Loading

bmarty left a comment

bmarty Feb 21, 2022

langleyd Feb 21, 2022

bmarty Mar 10, 2022

langleyd Mar 10, 2022

bmarty Mar 10, 2022

ganfra Mar 10, 2022

bmarty Mar 10, 2022

ganfra left a comment

ganfra Mar 10, 2022

langleyd Mar 11, 2022

ganfra Mar 10, 2022

ganfra Mar 10, 2022

bmarty commented Oct 4, 2022

Refresh Tokens #5023

Refresh Tokens #5023

Conversation

langleyd commented Jan 21, 2022 • edited Loading

What's in the PR

github-actions bot commented Jan 21, 2022 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2022 • edited Loading

Matrix SDK

Integration Tests Results:

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2022 • edited Loading

Ktlint Results

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganfra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Oct 4, 2022

langleyd commented Jan 21, 2022 •

edited

Loading

github-actions bot commented Jan 21, 2022 •

edited

Loading

github-actions bot commented Jan 21, 2022 •

edited

Loading

github-actions bot commented Feb 21, 2022 •

edited

Loading