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

Use *spec.SenderID for QuerySenderIDForUser #3164

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

swedgwood
Copy link
Contributor

@swedgwood swedgwood commented Aug 1, 2023

There are cases where a dendrite instance is unaware of a pseudo ID for a user, the user is not a member of that room. To represent this case, we currently use the 'zero' value, which is often not checked and so causes errors later down the line. To make this case more explict, and to be consistent with QueryUserIDForSender, this PR changes this to use a pointer (and nil to mean no sender ID).

Signed-off-by: Sam Wedgwood <[email protected]>

Note this PR depends on matrix-org/gomatrixserverlib#405 being merged first

- nil means no sender ID found
- including bumping GMSL to change some types to reflect this
@swedgwood
Copy link
Contributor Author

swedgwood commented Aug 1, 2023

After running pseudo ID tests, I've found that some tests that were previously passing are now failing - so I will look into these.

(I have a feeling that some of these are to do with empty strings being stored in the database, whereas now it's erroring instead of silently passing)

161	ok -> not ok	Users can't delete other's aliases
166	ok -> not ok	Only room members can list aliases of a room
227	ok -> not ok	Users cannot kick users from a room they are not in
235	ok -> not ok	m.room.history_visibility == "world_readable" allows/forbids appropriately for Guest users
236	ok -> not ok	m.room.history_visibility == "shared" allows/forbids appropriately for Guest users
239	ok -> not ok	m.room.history_visibility == "default" allows/forbids appropriately for Guest users
243	ok -> not ok	Guest non-joined users cannot send messages to guest_access rooms if not joined
249	ok -> not ok	m.room.history_visibility == "world_readable" allows/forbids appropriately for Real users
250	ok -> not ok	m.room.history_visibility == "shared" allows/forbids appropriately for Real users
253	ok -> not ok	m.room.history_visibility == "default" allows/forbids appropriately for Real users
257	ok -> not ok	Real non-joined users cannot send messages to guest_access rooms if not joined
268	ok -> not ok	/context/ on non world readable room does not work
699	ok -> not ok	AS can make room aliases

EDIT: will defer these fixes to another PR, and merge this one as it does not affect non-pseudo IDs

Copy link
Contributor

@S7evinK S7evinK left a comment

Choose a reason for hiding this comment

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

Aside from linter being unhappy, LGTM.

clientapi/routing/redaction.go Show resolved Hide resolved
swedgwood added a commit to matrix-org/gomatrixserverlib that referenced this pull request Aug 2, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 17.14% and project coverage change: -0.11% ⚠️

Comparison is base (af13fa1) 64.38% compared to head (ac5b506) 64.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3164      +/-   ##
==========================================
- Coverage   64.38%   64.28%   -0.11%     
==========================================
  Files         506      506              
  Lines       57040    57092      +52     
==========================================
- Hits        36725    36700      -25     
- Misses      16476    16553      +77     
  Partials     3839     3839              
Flag Coverage Δ
unittests 48.98% <17.14%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
clientapi/routing/directory.go 44.22% <0.00%> (-1.34%) ⬇️
clientapi/routing/profile.go 47.98% <0.00%> (-0.73%) ⬇️
clientapi/routing/redaction.go 39.84% <0.00%> (-4.60%) ⬇️
clientapi/routing/sendevent.go 59.23% <0.00%> (-0.77%) ⬇️
clientapi/threepid/invites.go 6.81% <0.00%> (-0.07%) ⬇️
federationapi/internal/perform.go 37.18% <0.00%> (-0.30%) ⬇️
federationapi/routing/join.go 51.09% <0.00%> (-0.94%) ⬇️
federationapi/routing/leave.go 42.71% <0.00%> (-1.24%) ⬇️
roomserver/api/api.go 66.66% <ø> (ø)
roomserver/internal/perform/perform_admin.go 37.79% <0.00%> (-0.30%) ⬇️
... and 9 more

... and 7 files with indirect coverage changes

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

@swedgwood swedgwood merged commit c7193e2 into main Aug 2, 2023
15 of 20 checks passed
@swedgwood swedgwood deleted the swedgwood/query-senderid-ptrs branch August 2, 2023 10:12
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.

2 participants