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: Fix original presentation download link in cluster setups #18150

Merged

Conversation

danielpetri1
Copy link
Collaborator

What does this PR do?

This pull request fixes the link in the "Download" button for original presentation files (i.e, no annotations) when they are sent to chat in cluster setups.

It also removes any cluster-specific settings from akka-bbb-apps and bbb-export-annotations for downloads, exports, and captures. From now on, links will be created without the need for the bbbWebBase information, letting Meteor alone handle this on the client side. The goal of this change is to decrease the coupling between components, making it easier for system admins to maintain and configure them.

Motivation

Only the link in the toast notification was working properly when downloading files without annotations in cluster setups. This issue was reported by @schrd. His feedback on this pull request would be appreciated.

@antobinary antobinary added this to the Release 2.6 milestone Jun 12, 2023
@schrd
Copy link
Collaborator

schrd commented Jun 13, 2023

I tried it and it works. There is a small glitch in the way URIs are constructed:

  • for documents without annotations the URI is /bigbluebutton//presentation/download ← look at the //
  • for documents with annotations the URI is /bigbluebutton/presentation/abcdefg...

Why did you remove the option to read in operator configuration? Are bbb-export-annotation settings meant to be immutable?

@sonarcloud
Copy link

sonarcloud bot commented Jun 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@danielpetri1
Copy link
Collaborator Author

Thanks for trying it out @schrd. The double // has been addressed.

The idea behind removing the option to read in operator configuration was to make maintenance and setup easier. I don't see why bbb-html5, bbb-export-annotations and akka-bbb-apps should all have to be coupled to BBB-Web's public API when it suffices to handle this in the client alone. This is what was broken in the first place: link generation for files with no annotations had the /bigbluebutton hardcoded in when this is not valid in cluster setups.

Is there any downside to this approach?

@schrd
Copy link
Collaborator

schrd commented Jun 14, 2023

IMHO there is nothing wrong with constructing the final API URL in the client.

The idea of separating operator config is to keep operator settings untouched if packages are upgraded.
Configuration from operators and default config should be in different files. That opens up the opportunity for packages to ship updated defaults without interaction (dpkg asking for "do you want to install packagers config or keep yours). Additionally there are tools like debsums which compare the md5sums of files shipped by packages with the entry in the dpkg database. If operators modify files which are not marked as config files in the dpkgdatabase, debsums will report an error. debsums can help detecting broken storage or otherwise modified binaries (for example by attackers).

That's why I was asking if the values in /usr/local/bigbluebutton/bbb-export-annotations/config/settings.json are meant to be constants and not to be changed by operators in any case. If they are meant to be immutable, its perfectly fine to remove the support for operator config. If they are not meant to be immutable then please keep the option to have separate operator config file.

@danielpetri1
Copy link
Collaborator Author

Thanks for the insightful response. The settings were indeed meant to be immutable.

@danimo
Copy link
Contributor

danimo commented Jun 23, 2023

It seems all issues are resolved, so can this be merged? I was hoping it would make it in time for 2.6.10, but somehow this was lost.

@antobinary
Copy link
Member

It seems all issues are resolved, so can this be merged? I was hoping it would make it in time for 2.6.10, but somehow this was lost.

I just checked with @danielpetri1 and we're good to go!

@antobinary antobinary merged commit 9a6c8f4 into bigbluebutton:v2.6.x-release Jun 23, 2023
4 of 5 checks passed
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.

None yet

4 participants