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

Fix user management in password request rooms #4656

Merged
merged 7 commits into from
Nov 27, 2020

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Nov 20, 2020

This fixes a regression introduced in 4afa2d7, as well as an UI fix that I thought that was a regression but seems to have been missing since the beginning 🤦.

The active guests are only those who are currently in a call, and not those who are currently in the conversation. Therefore other guests or users were not prevented from joining a password request conversation if a guest was in the conversation but not in the call.

Fortunately in practice this was not a problem, as the Web UI starts a call immediately after joining the conversation, which made the guest immediately active and thus prevented others from joining.

Besides that it adds integration tests for password request conversations.

Pending:

  • Do not break previous integration tests :-)
  • Do not allow the owner to add more users to password request conversations (as they will not be able to join the conversation anyway), neither in the API nor in the UI

@danxuliu
Copy link
Member Author

/backport to stable20

@danxuliu
Copy link
Member Author

/backport to stable19

@danxuliu
Copy link
Member Author

/backport to stable18

Until now it was possible to verify the room data when getting the full
room list with the "user is participant of the following rooms" step.
Now the same can be optionally done when using "user is participant of
room XXX" too.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This fixes a regression introduced in 4afa2d7.

The active guests are only those who are currently in a call, and not
those who are currently in the conversation. Therefore other guests or
users were not prevented from joining a password request conversation if
a guest was in the conversation but not in the call.

Fortunately in practice this was not a problem, as the Web UI starts a
call immediately after joining the conversation, which made the guest
immediately active and thus prevented others from joining.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Only the owner and another participant will be allowed to join a
password request room, so there is no point in being able to add more
participants to those rooms.

Although throwing the exception in the listener is enough to prevent
adding the participants unhandled exceptions in the endpoint are
returned as error 404, but the expected error would be 400. To minimize
conflicts with other pull requests and backports it is explicitly
checked if the room is a password request room instead of refactoring
the code to handle the exception.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the fix-user-management-in-password-request-rooms branch from 83c138e to 44f3732 Compare November 24, 2020 18:00
@danxuliu
Copy link
Member Author

Sharing integration test failures are unrelated, see nextcloud/server#23820 (comment)

@danxuliu danxuliu marked this pull request as ready for review November 24, 2020 18:01
@danxuliu danxuliu requested a review from PVince81 November 24, 2020 18:02
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member

Tests pass locally:

PHPUnit 8.5.9 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.12 with Xdebug 2.9.8
Configuration: /home/nickv/Nextcloud/21/server/appsbabies/spreed/tests/php/phpunit.xml

...............................................................  63 / 451 ( 13%)
............................................................... 126 / 451 ( 27%)
............................................................... 189 / 451 ( 41%)
............................................................... 252 / 451 ( 55%)
............................................................... 315 / 451 ( 69%)
............................................................... 378 / 451 ( 83%)
............................................................... 441 / 451 ( 97%)
..........                                                      451 / 451 (100%)

Time: 3.51 seconds, Memory: 40.00 MB

OK (451 tests, 1638 assertions)

@nickvergessen nickvergessen merged commit ec14c70 into master Nov 27, 2020
@nickvergessen nickvergessen deleted the fix-user-management-in-password-request-rooms branch November 27, 2020 12:12
@backportbot-nextcloud
Copy link

The backport to stable20 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable19 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable18 failed. Please do this backport manually.

@nickvergessen
Copy link
Member

@danxuliu what about the backports

@danxuliu
Copy link
Member Author

Done.

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

Successfully merging this pull request may close these issues.

2 participants