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: use grails configuration for CORS settings #15457

Merged
merged 1 commit into from Jan 23, 2023

Conversation

schrd
Copy link
Collaborator

@schrd schrd commented Jul 27, 2022

What does this PR do?

It removes quirks from bbb-web nginx config and uses grails config options to properly support CORS requests. CORS requests are only required if you run a cluster proxy setup as described in https://docs.bigbluebutton.org/admin/clusterproxy.html

Closes Issue(s)

Closes #15436

Motivation

Grails can handle CORS on its own. It just has to be configured in
/etc/bigbluebutton/bbb-web.properties:

grails.cors.enabled=true
grails.cors.allowedOrigins=https://bbb-proxy.example.com
grails.cors.allowCredentials=true

This is a breaking change of the nginx config if (and only if) you run a
cluster setup as described in
https://docs.bigbluebutton.org/admin/clusterproxy.html

If you run such a setup, you need to change
/etc/bigbluebutton/bbb-web.properties. Otherwise users won't be able
to join meetings, upload slides etc.

The change in PresentationController.groovy fixes the handling of
OPTIONS requests in the /bigbluebutton/presentation/checkPresentation
handler.

More

⚠️ If you merge this into 2.5 operators must be warned that if they run a cluster proxy setup they must tweak their configuration

Grails can handle CORS on its own. It just has to be configured in
`/etc/bigbluebutton/bbb-web.properties`:

~~~
grails.cors.enabled=true
grails.cors.allowedOrigins=https://bbb-proxy.example.org
grails.cors.allowCredentials=true
~~~

This is a breaking change of the nginx config if (and only if) you run a
cluster setup as described in
https://docs.bigbluebutton.org/admin/clusterproxy.html

**If** you run such a setup, you **need** to change
`/etc/bigbluebutton/bbb-web.properties`. Otherwise users won't be able
to join meetings, upload slides etc.

The change in `PresentationController.groovy` fixes the handling of
`OPTIONS` requests in the `/bigbluebutton/presentation/checkPresentation`
handler.
@sonarcloud
Copy link

sonarcloud bot commented Jul 27, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@antobinary antobinary added this to the Release 2.6 milestone Aug 18, 2022
@antobinary
Copy link
Member

Thanks for this contribution @schrd !

We chatted about this internally and it looks like a very welcome change! We'd be looking into including this on BBB 2.6
I am setting the milestone and am tweaking the target branch. Likely we'll need to do a [force]push or rebase but this is something that can wait until testing+final review stage (unless you do it yourself meanwhile).

I just wanted to keep you posted on the positive feedback. Thanks again!

@antobinary antobinary changed the base branch from v2.5.x-release to v2.6.x-release August 18, 2022 21:02
@danimo
Copy link
Contributor

danimo commented Aug 29, 2022

What's the status here?

@GuiLeme
Copy link
Collaborator

GuiLeme commented Sep 29, 2022

Hello, guys, sorry for the delayed response.

So, I tested this PR with a reverse proxy and only one instance of BBB, and there is something I want you to add in the configuration part, adjusting @schrd's comment: in /etc/bigbluebutton/bbb-web.properties, we must insert the BBB instance URL in the grails.cors.allowedOrigins property too, so it would be something like this:

grails.cors.enabled=true
grails.cors.allowedOrigins=https://bbb-proxy.example.com,https://bbb.example.com
grails.cors.allowCredentials=true

This fixes a connection problem with etherpad. What was happening is that bbb-web was allowed to receive requests from the proxy, but not the instance itself as it was calling the checkAuthorization endpoint in the API, so it ended up with a 403 error, see prints ahead:

image

image

And shared notes keeps loading:

image

With the solution mentioned, all goes as expected!

@schrd
Copy link
Collaborator Author

schrd commented Sep 29, 2022

I think this is something else. Pad requests never should go through the proxy. Maybe someone is generating a URL /pad/... somewhere which would lead the browser to the proxy. I think instead direct connection to the BBB node should be made. I'll have a look into this.

@GuiLeme
Copy link
Collaborator

GuiLeme commented Sep 29, 2022

Yeah, it seems that for the client to connect to the pad, it has to get the session for the etherpad component, so, I guess this request to /pad/socket.io... would make sense, and also, there is a request made to bbb-web to /checkAuthorization endpoint which gave us this CORS problem.

@GuiLeme
Copy link
Collaborator

GuiLeme commented Sep 29, 2022

As I already commented on #15753 there are some fixes to the etherpad's files for nginx to avoid other CORS problem and resolve item B on #15436, so in #15753 we have a simple solution that could address it.

The problem happens when the user clicks the move notes to whiteboard button, although it answers with a 200 status, we also face a CORS problem, which would mean that in the response that etherpad gave us, nginx didn't properly set the headers, and thus the browser doesn't match with the expected, logging an error and crashing the fetch call in

const sharedNotesAsFile = await fetch(exportUrl, { credentials: 'include' });

Bearing this in mind, as @schrd pointed out in #15753 (comment) there may be a better solution for that, so we will hold this PR a bit.

@GuiLeme
Copy link
Collaborator

GuiLeme commented Oct 5, 2022

I think this is something else. Pad requests never should go through the proxy. Maybe someone is generating a URL /pad/... somewhere which would lead the browser to the proxy. I think instead direct connection to the BBB node should be made. I'll have a look into this.

@schrd, when you find it, please, respond back in this PR, so that we can properly merge it, because outside of the thing I mentioned before, it is working just fine!!

@schrd
Copy link
Collaborator Author

schrd commented Nov 2, 2022

@GuiLeme sorry for being so late. With alangecker/bbb-etherpad-plugin#7 CORS problems in shared notes should be gone.

@GuiLeme
Copy link
Collaborator

GuiLeme commented Jan 10, 2023

@GuiLeme sorry for being so late. With alangecker/bbb-etherpad-plugin#7 CORS problems in shared notes should be gone.

Hey @schrd, so, can we close this PR? Or is it still to be merged?

@schrd
Copy link
Collaborator Author

schrd commented Jan 11, 2023

Hey @schrd, so, can we close this PR? Or is it still to be merged?

I think it would be still useful to have this merged. BBB works without this merge but needs ugly quirks in the nginx config which do not always work as expected (see #15436). So the purpose of this MR is pure cleanup.

Copy link
Collaborator

@GuiLeme GuiLeme left a comment

Choose a reason for hiding this comment

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


@GuiLeme
Copy link
Collaborator

GuiLeme commented Jan 23, 2023

Ok, so after a thorough review, it looks very good, and I don't seem to find any trouble regarding this PR.
But, I've been having some trouble while creating the cluster configured server, and so I will open an issue giving some details and if anyone wants to comment out, please do so!! Thank you!

Copy link
Collaborator

@GuiLeme GuiLeme left a comment

Choose a reason for hiding this comment

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

LGTM!

@antobinary antobinary merged commit 5ea43ff into bigbluebutton:v2.6.x-release Jan 23, 2023
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