Skip to content

Conversation

zecakeh
Copy link
Collaborator

@zecakeh zecakeh commented Jul 30, 2022

Refresh tokens are for when access tokens have an expiry date. It allows to request a new access token without logging again. It's part of the spec since v1.3 and is a prerequisite for #859.

Some refactoring of Session was necessary to be able to make the tokens mutable. We could use a single method to expose the tokens as ReadOnlyMutable in the client, but I thought it would be easier to grasp it with separate methods for users that don't know how the futures-signals crate works.

We give the option for the Client to refresh the tokens automatically. It's not enabled by default to be conservative and avoid the user holding an obsolete refresh token.

@codecov
Copy link

codecov bot commented Jul 30, 2022

Codecov Report

Merging #892 (deae3f7) into main (ae261c2) will increase coverage by 0.45%.
The diff coverage is 96.79%.

@@            Coverage Diff             @@
##             main     #892      +/-   ##
==========================================
+ Coverage   79.38%   79.83%   +0.45%     
==========================================
  Files         106      107       +1     
  Lines       14662    15052     +390     
==========================================
+ Hits        11639    12017     +378     
- Misses       3023     3035      +12     
Impacted Files Coverage Δ
crates/matrix-sdk-appservice/src/virtual_user.rs 48.97% <0.00%> (-1.03%) ⬇️
crates/matrix-sdk/src/error.rs 25.92% <ø> (ø)
crates/matrix-sdk/tests/integration/main.rs 100.00% <ø> (ø)
crates/matrix-sdk/src/client/login_builder.rs 27.10% <50.00%> (+1.85%) ⬆️
crates/matrix-sdk/src/client/mod.rs 76.01% <88.33%> (+1.55%) ⬆️
crates/matrix-sdk-base/src/session.rs 66.66% <90.90%> (+66.66%) ⬆️
crates/matrix-sdk-base/src/client.rs 79.47% <100.00%> (+0.41%) ⬆️
crates/matrix-sdk-base/src/store/mod.rs 88.88% <100.00%> (+1.38%) ⬆️
crates/matrix-sdk/src/client/builder.rs 81.73% <100.00%> (+0.92%) ⬆️
crates/matrix-sdk/tests/integration/client.rs 94.75% <100.00%> (+0.48%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae261c2...deae3f7. Read the comment docs.

@zecakeh zecakeh force-pushed the refresh-token branch 3 times, most recently from f05095b to c67c04c Compare July 30, 2022 11:45
Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't fully review, just a small note

@jplatte jplatte requested a review from poljar August 1, 2022 09:54
@jplatte
Copy link
Collaborator

jplatte commented Aug 1, 2022

I think poljar had some thoughts on this, though let me know if I should do a full review.

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks heaps. In particular appreciate all these tests!

Two things need to change though:

  1. naming things: I find SessionIds confusing, maybe we can opt of SessionInfo or SessionMeta instead?
  2. changing Session means we need to add migrations for the stores to prevent a forced login when coming with an old. Would be great to add an integration test for it. You think you'll be able to add migrations for the stores?

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore my migration request, only the error passing missing, the this is ready for merge IMHO.

@zecakeh
Copy link
Collaborator Author

zecakeh commented Aug 2, 2022

This force push was to rebase on main because GitHub complained there was a conflict.

@jplatte jplatte merged commit 9064e7b into matrix-org:main Aug 3, 2022
@zecakeh zecakeh deleted the refresh-token branch August 3, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants