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

refactor: Add readOnlyID to PadCreatedEvtMsg #34

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

gustavotrott
Copy link
Contributor

BBB used to employ Etherpad's read-only view for users who didn't have writing privileges. This functionality was modified by bigbluebutton/bigbluebutton#13916.

After some tests, I found that bbb-pads automatically invalidates a user's session once they lose writing rights. As a result, even if the user tries to restore their previous session, they will no longer have the ability to modify the note.

For the upcoming BBB 2.8 release, I plan to conduct tests utilizing Etherpad's read-only mode. This is because the read-only mode offers a significantly improved user experience.

This particular PR essentially fetches the read-only ID of the pad and transmits it via the PadCreatedEvtMsg message. This ID will then be stored in the database and accessible through GraphQL.

@pedrobmarin
Copy link
Collaborator

Hey @gustavotrott . Do you mean using Etherpads read only view to replace the current iframe one? If that's the case, be aware that this was the source of a major vulnerability with using Etherpad in BigBlueButton.

cc @antobinary @prlanzarin

@gustavotrott
Copy link
Contributor Author

Hey @gustavotrott . Do you mean using Etherpads read only view to replace the current iframe one? If that's the case, be aware that this was the source of a major vulnerability with using Etherpad in BigBlueButton.

cc @antobinary @prlanzarin

Exactly! The idea is to revisit this topic to see which are the vulnerabilities and if we are able to tackle it someway.
Thanks for the heads up!

@pedrobmarin
Copy link
Collaborator

pedrobmarin commented Aug 29, 2023

The problem is that, to be able to view the read only pad, you'll need a valid sesion for that pad. Using that session the user can access the editable mode of the pad and change the pads data.

@pedrobmarin
Copy link
Collaborator

After some tests, I found that bbb-pads automatically invalidates a user's session once they lose writing rights. As a result, even if the user tries to restore their previous session, they will no longer have the ability to modify the note.

Reading this one again. What you mean an user tries to restore the previous session? You mean a force restore to try to re-access a content that should not be available anymore or you saying a valid scenario where the user should regain access?

This module was designed to regenerate the users session everytime a permission is granted. For that, there are multiple events being listened and data checks like promotions, demotions, locks, unlocks... if that's not working anymore something changed someplace else and broke the behavior.

@gustavotrott
Copy link
Contributor Author

After some tests, I found that bbb-pads automatically invalidates a user's session once they lose writing rights. As a result, even if the user tries to restore their previous session, they will no longer have the ability to modify the note.

Reading this one again. What you mean an user tries to restore the previous session? You mean a force restore to try to re-access a content that should not be available anymore or you saying a valid scenario where the user should regain access?

This module was designed to regenerate the users session everytime a permission is granted. For that, there are multiple events being listened and data checks like promotions, demotions, locks, unlocks... if that's not working anymore something changed someplace else and broke the behavior.

Yes, actually I just wanted to confirm that I tested and it is working properly!

@gustavotrott gustavotrott marked this pull request as draft August 29, 2023 19:42
@gustavotrott
Copy link
Contributor Author

I converted this PR to Draft because I'll hold on this task for a week.
After further tests I will update here what I found trying some different approaches (being careful to not backtrack on security aspects).

Thanks for your feedback @pedrobmarin, your review will be really important since you are the one who dealt with this issues.

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