Skip to content

Conversation

@nickvergessen
Copy link
Member

No description provided.

@nickvergessen nickvergessen added 3. to review enhancement feature: api 🛠️ OCS API for conversations, chats and participants labels Jun 30, 2022
@nickvergessen nickvergessen added this to the 💚 Next Major (25) milestone Jun 30, 2022
Copy link
Contributor

@vitormattos vitormattos left a comment

Choose a reason for hiding this comment

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

I think will be good to implement behat tests to check if it is working fine.

@nickvergessen
Copy link
Member Author

I think will be good to implement behat tests to check if it is working fine.

It is intentionally disables even for the login and stuff:
https://github.com/nextcloud/spreed/blob/master/tests/integration/run.sh#L69-L70
So not sure that is possible.
Similarly we can't really test the password policy feature in #7473 as the used passwords violate the rules.

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen
Copy link
Member Author

/backport to stable24

@nickvergessen
Copy link
Member Author

/backport to stable23

@nickvergessen
Copy link
Member Author

/backport to stable22

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

The delay in HPB requests needs to be reset.

@nickvergessen nickvergessen requested a review from danxuliu July 1, 2022 22:02
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

If I am not mistaken there are some other endpoints that need to be protected:

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen
Copy link
Member Author

getRoomByFileId

Not sure what to throttle there. First thing that is done is checking if you have access to the fileId. That is outside of the scope of Talk and only gives you tokens which you can access.

Other things are fixed

@nickvergessen nickvergessen requested a review from danxuliu July 4, 2022 10:59
@danxuliu
Copy link
Member

danxuliu commented Jul 4, 2022

getRoomByFileId

Not sure what to throttle there. First thing that is done is checking if you have access to the fileId. That is outside of the scope of Talk and only gives you tokens which you can access.

I thought that public shared files were seen as being accessible by the user when checked with the file id, but the code says that they are not. I think that I mixed up the tests for the shared token and the file id, sorry for the noise.

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Tested and (mostly, see comments) works 👍

What about resetting the delay for the signaling server IP when the secret is changed? In my experience it is not uncommon to set a wrong secret, and if it is not reset and no hint is given about the throttling I would expect some puzzled admins :-)

Besides that, similarly to getParticipantByDialInPin and createGuestByDialIn, getSingleRoom needs to be protected for the SIP bridge secret and the talk room token. However, getParticipantByDialInPin and createGuestByDialIn have an explicit annotation for talkSipBridgeSecret and an implicit protection for talkRoomToken due to using @RequireRoom, but getSingleRoom only has an explicit annotation for talkRoomToken.

It would be more correct if both actions have an explicit handling, for example by calling sleepDelay and registerAttempt instead of throttle when the SIP bridge secret can not be validated. Right now it probably makes no difference in practice, as those actions are never reset, but it could have unexpected behaviours if at some point they are. As it is just a "future-proofness" thing it might be enough to add a comment mentioning it.

@nickvergessen nickvergessen requested a review from danxuliu July 5, 2022 09:34
Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen nickvergessen merged commit 6ec9552 into master Jul 7, 2022
@nickvergessen nickvergessen deleted the bugfix/noid/add-brute-force-protection-to-conversation-passwords branch July 7, 2022 07:54
@backportbot-nextcloud
Copy link

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

@nickvergessen
Copy link
Member Author

22 in #7537

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

Labels

4. to release enhancement feature: api 🛠️ OCS API for conversations, chats and participants

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants