Skip to content
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

feat(sdk): Add support for refresh tokens #892

Merged
merged 7 commits into from
Aug 3, 2022

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

crates/matrix-sdk-base/src/session.rs Outdated Show resolved Hide resolved
@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?

crates/matrix-sdk-base/src/session.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/session.rs Show resolved Hide resolved
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