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

rtc: Handle non-MXID call member event state keys #3836

Merged
merged 11 commits into from
Aug 14, 2024

Conversation

AndrewFerr
Copy link
Member

Update Ruma dependency to expect call membership state events with state keys that are arbitrary strings, not just pure MXIDs.

When a call membership state key does not exactly match the format of an MXID, treat it as a valid state key if it starts with an MXID followed by an underscore, with that MXID designating the owner of the event.

(The state key may also be optionally prefixed with an underscore, which is permitted as a way to bypass pre-MSC3757 authorization rules against sending state events with state keys that do not exactly match the sender's MXID.)

  • Public API changes documented in changelogs (optional)

Signed-off-by: Andrew Ferrazzutti [email protected]

Update Ruma dependency to expect call membership state events with state
keys that are arbitrary strings, not just pure MXIDs.

When a call membership state key does not exactly match the format of an
MXID, treat it as a valid state key if it starts with an MXID followed
by an underscore, with that MXID designating the owner of the event.

(The state key may also be optionally prefixed with an underscore, which
is permitted as a way to bypass pre-MSC3757 authorization rules against
sending state events with state keys that do not exactly match the
sender's MXID.)
@AndrewFerr AndrewFerr requested a review from a team as a code owner August 13, 2024 03:04
@AndrewFerr AndrewFerr requested review from bnjbvr and removed request for a team August 13, 2024 03:04
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.13%. Comparing base (35b62a1) to head (ca47e8a).
Report is 17 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk-base/src/rooms/mod.rs 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3836      +/-   ##
==========================================
+ Coverage   84.12%   84.13%   +0.01%     
==========================================
  Files         263      263              
  Lines       27592    27605      +13     
==========================================
+ Hits        23211    23225      +14     
+ Misses       4381     4380       -1     

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

@stefanceriu stefanceriu requested review from stefanceriu and removed request for bnjbvr August 13, 2024 05:00
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Thanks, looks sane but it'd be nice to have some basic unit tests (which would also show what behavior is expected and what isn't).

crates/matrix-sdk-base/src/rooms/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/mod.rs Outdated Show resolved Hide resolved
@bnjbvr bnjbvr removed the request for review from stefanceriu August 13, 2024 06:19
@bnjbvr
Copy link
Member

bnjbvr commented Aug 13, 2024

(Sorry @stefanceriu, I hadn't seen you removed my review request, and the PR is simple enough that I could take a look — feel free to re-take over if you prefer!)

Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

I hadn't seen you removed my review request

Oh no worries, I only removed you because I can test it end to end, but it doesn't really matter.

It seems to work just fine on both call.element.io and call.element.dev so from my point of view you can merge it after adressing benji's concerns. Nicely done! 👍

crates/matrix-sdk-base/src/rooms/mod.rs Show resolved Hide resolved
@AndrewFerr AndrewFerr requested a review from bnjbvr August 13, 2024 14:21
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Getting closer, thanks for making a separate function and writing tests!

crates/matrix-sdk-base/src/rooms/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/mod.rs Outdated Show resolved Hide resolved
@bnjbvr bnjbvr changed the title Handle non-MXID call member event state keys rtc: Handle non-MXID call member event state keys Aug 13, 2024
@AndrewFerr AndrewFerr requested a review from bnjbvr August 13, 2024 16:46
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Looks good to me, modulo test nits, thanks! Please push another commit with the test changes, and then no need to ask for another review, I'll merge it 👍

crates/matrix-sdk-base/src/rooms/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/mod.rs Outdated Show resolved Hide resolved
@bnjbvr
Copy link
Member

bnjbvr commented Aug 14, 2024

I've addressed the nits for you actually, so we can merge :)

@bnjbvr bnjbvr enabled auto-merge (squash) August 14, 2024 08:28
@bnjbvr bnjbvr merged commit 3803792 into matrix-org:main Aug 14, 2024
40 checks passed
@AndrewFerr AndrewFerr deleted the call-member-state-key-type branch August 14, 2024 12:06
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.

3 participants