Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fixup shutdown room API #4904

Merged
merged 14 commits into from
Mar 21, 2019
Merged

Fixup shutdown room API #4904

merged 14 commits into from
Mar 21, 2019

Conversation

erikjohnston
Copy link
Member

No description provided.

There are a number of instances where a server or admin may puppet a
user to join/leave rooms, which we don't want to fail if the user has
not consented to the privacy policy. We fix this by adding a check to
test if the requester has an associated access_token, which is used as a
proxy to answer the question of whether the action is being done on
behalf of a real request from the user.
@erikjohnston erikjohnston requested a review from a team March 20, 2019 16:57
@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #4904 into develop will decrease coverage by 0.05%.
The diff coverage is 7.69%.

@@             Coverage Diff             @@
##           develop    #4904      +/-   ##
===========================================
- Coverage    59.85%   59.79%   -0.06%     
===========================================
  Files          326      326              
  Lines        34025    34026       +1     
  Branches      5613     5612       -1     
===========================================
- Hits         20366    20347      -19     
- Misses       12235    12254      +19     
- Partials      1424     1425       +1

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #4904 into develop will decrease coverage by 0.33%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           develop   #4904      +/-   ##
==========================================
- Coverage    77.94%   77.6%   -0.34%     
==========================================
  Files          326     326              
  Lines        34025   34278     +253     
  Branches      5613    5716     +103     
==========================================
+ Hits         26521   26603      +82     
- Misses        5878    6033     +155     
- Partials      1626    1642      +16

synapse/handlers/message.py Outdated Show resolved Hide resolved
synapse/storage/room.py Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

the absence of a test makes me sad

@@ -388,17 +391,6 @@ def assert_accepted_privacy_policy(self, requester):
if self._block_events_without_consent_error is None:
return

# exempt AS users from needing consent
Copy link
Member

Choose a reason for hiding this comment

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

do we not still need this? is it covered by the is_exempt clause?

@erikjohnston erikjohnston requested a review from richvdh March 21, 2019 10:59
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

couple of nits in the tests


other_user = self.register_user("user", "pass")
other_user_token = self.login("user", "pass")
def test_shutdown_room_conset(self):
Copy link
Member

Choose a reason for hiding this comment

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

conset


other_user = self.register_user("user", "pass")
other_user_token = self.login("user", "pass")
def test_shutdown_room_conset(self):
Copy link
Member

Choose a reason for hiding this comment

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

can you add a docstring that explains in slightly more words what you are testing for here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants