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.

Sign up for GitHub

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

Jump to bottom

Fix adding and removing permissions #13027

Merged
merged 5 commits into from
Aug 22, 2024
Merged

Fix adding and removing permissions #13027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1164db
test: Add integration tests for adding and removing permissions
danxuliu Aug 17, 2024
b1fce9a
fix: Fix adding permissions when attendees have default permissions
danxuliu Aug 17, 2024
a974742
refactor: Extract method to get the base query for setting permissions
danxuliu Aug 17, 2024
1111bd3
fix: Fix adding and removing several attendee permissions at once
danxuliu Aug 17, 2024
7d8ea65
fix: Fix adding or removing permissions without call permissions set
danxuliu Aug 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions lib/Model/AttendeeMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,18 +203,12 @@ public function deleteByIds(array $ids): int {
}

public function modifyPermissions(int $roomId, string $mode, int $newState): void {
$query = $this->db->getQueryBuilder();
$query->update($this->getTableName())
->where($query->expr()->eq('room_id', $query->createNamedParameter($roomId, IQueryBuilder::PARAM_INT)))
->andWhere($query->expr()->notIn('actor_type', $query->createNamedParameter([
Attendee::ACTOR_CIRCLES,
Attendee::ACTOR_GROUPS,
], IQueryBuilder::PARAM_STR_ARRAY)));

if ($mode === Attendee::PERMISSIONS_MODIFY_SET) {
if ($newState !== Attendee::PERMISSIONS_DEFAULT) {
$newState |= Attendee::PERMISSIONS_CUSTOM;
}

$query = $this->getModifyPermissionsBaseQuery($roomId);
$query->set('permissions', $query->createNamedParameter($newState, IQueryBuilder::PARAM_INT));
$query->executeStatement();
} else {
Expand All @@ -227,6 +221,8 @@ public function modifyPermissions(int $roomId, string $mode, int $newState): voi
Attendee::PERMISSIONS_LOBBY_IGNORE,
] as $permission) {
if ($permission & $newState) {
$query = $this->getModifyPermissionsBaseQuery($roomId);
Copy link
Member

Choose a reason for hiding this comment

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

this could be outside the loop? We don't need to create the query builder 7 times?

Copy link
Member Author

Choose a reason for hiding this comment

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

The query builder would be created as many times as individual permissions are added/removed, so it would be created seven times only if the seven permissions were added/removed at once.

A single query object needs to be created for each individual permission because adding or removing a single permission executes the statement. Is there any way to reset the query builder to reuse the same base update query and change the set and andWhere parts?


if ($mode === Attendee::PERMISSIONS_MODIFY_ADD) {
$this->addSinglePermission($query, $permission);
} elseif ($mode === Attendee::PERMISSIONS_MODIFY_REMOVE) {
Expand All @@ -237,6 +233,18 @@ public function modifyPermissions(int $roomId, string $mode, int $newState): voi
}
}

protected function getModifyPermissionsBaseQuery(int $roomId): IQueryBuilder {
$query = $this->db->getQueryBuilder();
$query->update($this->getTableName())
->where($query->expr()->eq('room_id', $query->createNamedParameter($roomId, IQueryBuilder::PARAM_INT)))
->andWhere($query->expr()->notIn('actor_type', $query->createNamedParameter([
Attendee::ACTOR_CIRCLES,
Attendee::ACTOR_GROUPS,
], IQueryBuilder::PARAM_STR_ARRAY)));

return $query;
}

protected function addSinglePermission(IQueryBuilder $query, int $permission): void {
$query->set('permissions', $query->func()->add(
'permissions',
Expand All @@ -255,6 +263,12 @@ protected function addSinglePermission(IQueryBuilder $query, int $permission): v
$query->createNamedParameter($permission, IQueryBuilder::PARAM_INT)
)
);
$query->andWhere(
$query->expr()->neq(
'permissions',
$query->createNamedParameter(Attendee::PERMISSIONS_DEFAULT, IQueryBuilder::PARAM_INT)
)
);

$query->executeStatement();
}
Expand All @@ -277,6 +291,9 @@ protected function removeSinglePermission(IQueryBuilder $query, int $permission)
$query->createNamedParameter($permission, IQueryBuilder::PARAM_INT)
)
);
// Removing permissions does not need to be explicitly prevented when
// the attendee has default permissions, as in that case it will not be
// possible to remove the permissions anyway.

$query->executeStatement();
}
Expand Down
6 changes: 6 additions & 0 deletions lib/Service/RoomService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,12 @@ public function setPermissions(Room $room, string $level, string $method, int $p
} elseif ($level === 'call') {
$property = ARoomModifiedEvent::PROPERTY_CALL_PERMISSIONS;
$oldPermissions = $room->getCallPermissions();

if ($oldPermissions === Attendee::PERMISSIONS_DEFAULT
&& ($method === Attendee::PERMISSIONS_MODIFY_ADD
|| $method === Attendee::PERMISSIONS_MODIFY_REMOVE)) {
$oldPermissions = $room->getDefaultPermissions();
}
} else {
return false;
}
Expand Down
50 changes: 50 additions & 0 deletions tests/integration/features/bootstrap/FeatureContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,21 @@ private function assertRooms(array $rooms, TableNode $formData, bool $shouldOrde
if (isset($expectedRoom['recordingConsent'])) {
$data['recordingConsent'] = (int) $room['recordingConsent'];
}
if (isset($expectedRoom['permissions'])) {
$data['permissions'] = $this->mapPermissionsAPIOutput($room['permissions']);
}
if (isset($expectedRoom['permissions'])) {
$data['permissions'] = $this->mapPermissionsAPIOutput($room['permissions']);
}
if (isset($expectedRoom['attendeePermissions'])) {
$data['attendeePermissions'] = $this->mapPermissionsAPIOutput($room['attendeePermissions']);
}
if (isset($expectedRoom['callPermissions'])) {
$data['callPermissions'] = $this->mapPermissionsAPIOutput($room['callPermissions']);
}
if (isset($expectedRoom['defaultPermissions'])) {
$data['defaultPermissions'] = $this->mapPermissionsAPIOutput($room['defaultPermissions']);
}
if (isset($expectedRoom['participants'])) {
throw new \Exception('participants key needs to be checked via participants endpoint');
}
Expand Down Expand Up @@ -2024,6 +2039,41 @@ public function userSetsPermissionsForInRoomTo(string $user, string $participant
$this->assertStatusCode($this->response, $statusCode);
}

/**
* @When /^user "([^"]*)" (sets|removes|adds) permissions for all attendees in room "([^"]*)" to "([^"]*)" with (\d+) \((v4)\)$/
*
* @param string $user
* @param string $mode
* @param string $identifier
* @param string $permissionsString
* @param int $statusCode
* @param string $apiVersion
*/
public function userSetsRemovesAddsPermissionsForAllAttendeesInRoomTo(string $user, string $method, string $identifier, string $permissionsString, int $statusCode, string $apiVersion): void {
$permissions = $this->mapPermissionsTestInput($permissionsString);

// Convert method from step syntax to what the API expects
if ($method === 'sets') {
$method = 'set';
} elseif ($method === 'removes') {
$method = 'remove';
} else {
$method = 'add';
}

$requestParameters = [
['method', $method],
['permissions', $permissions],
];

$this->setCurrentUser($user);
$this->sendRequest(
'PUT', '/apps/spreed/api/' . $apiVersion . '/room/' . self::$identifierToToken[$identifier] . '/attendees/permissions/all',
new TableNode($requestParameters)
);
$this->assertStatusCode($this->response, $statusCode);
}

/**
* @When /^user "([^"]*)" sets (call|default) permissions for room "([^"]*)" to "([^"]*)" with (\d+) \((v4)\)$/
*
Expand Down
36 changes: 36 additions & 0 deletions tests/integration/features/conversation-5/set-permissions.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,39 @@ Feature: conversation-2/set-publishing-permissions
| actorType | actorId | permissions | attendeePermissions | participantType |
| users | owner | SJLAVPM | D | 1 |
| users | invited user | CLAVPM | CLAVPM | 3 |

Scenario: adding and removing permissions to all attendees do not change default attendee permissions
Given user "invited user2" exists
And user "owner" creates room "group room" (v4)
| roomType | 2 |
| roomName | room |
And user "owner" adds user "invited user" to room "group room" with 200 (v4)
And user "owner" adds user "invited user2" to room "group room" with 200 (v4)
And user "owner" sets call permissions for room "group room" to "LPM" with 200 (v4)
And user "owner" sets permissions for "invited user2" in room "group room" to "LVP" with 200 (v4)
When user "owner" adds permissions for all attendees in room "group room" to "JA" with 200 (v4)
And user "owner" removes permissions for all attendees in room "group room" to "SLP" with 200 (v4)
Then user "owner" sees the following attendees in room "group room" with 200 (v4)
| actorType | actorId | permissions | attendeePermissions |
| users | owner | SJLAVPM | D |
| users | invited user | CJAM | D |
| users | invited user2 | CJAV | CJAV |
And user "owner" is participant of room "group room" (v4)
| callPermissions | defaultPermissions |
| CJAM | D |

Scenario: adding and removing permissions to all attendees customize room permissions if call permissions are not set
Given user "owner" creates room "group room" (v4)
| roomType | 2 |
| roomName | room |
And user "owner" adds user "invited user" to room "group room" with 200 (v4)
And user "owner" sets default permissions for room "group room" to "LPM" with 200 (v4)
When user "owner" adds permissions for all attendees in room "group room" to "JA" with 200 (v4)
And user "owner" removes permissions for all attendees in room "group room" to "SLP" with 200 (v4)
Then user "owner" sees the following attendees in room "group room" with 200 (v4)
| actorType | actorId | permissions | attendeePermissions |
| users | owner | SJLAVPM | D |
| users | invited user | CJAM | D |
And user "owner" is participant of room "group room" (v4)
| callPermissions | defaultPermissions |
| CJAM | CLPM |
Loading