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

External SPA applications allow origin #42

Open
t-book opened this issue May 13, 2024 · 14 comments
Open

External SPA applications allow origin #42

t-book opened this issue May 13, 2024 · 14 comments
Labels
question Further information is requested

Comments

@t-book
Copy link
Contributor

t-book commented May 13, 2024

Hi,

we have some SPA apps that use oauth2 to auth against geonode. In past I've needed to update my local build

if ($request_method = OPTIONS) {
add_header Access-Control-Allow-Methods "GET, POST, PUT, PATCH, OPTIONS";
add_header Access-Control-Allow-Headers "Authorization, Content-Type, Accept";
add_header Access-Control-Allow-Credentials true;
add_header Content-Length 0;
add_header Content-Type text/plain;
add_header Access-Control-Max-Age 1728000;
return 200;
}

@@ -116,6 +116,7 @@ location / {
   set $upstream django:8000;

   if ($request_method = OPTIONS) {
+      add_header Access-Control-Allow-Origin "*";

To not run into a CORS error. I wonder how other devs deal with ACAO. I do not see a way to set Access-Control-Allow-Origin dynamicully but would try to avoid using a forked version of this image.

Thanks,

Toni

@t-book t-book added the question Further information is requested label May 13, 2024
@ridoo
Copy link

ridoo commented May 14, 2024

@t-book I agree .. having static CORS configuration is a limitation which could be improved by using Django based CORS configuration. It seems that the django-cors-header app is exactly for that. Funny enough, that it is a GeoNode dependency already. Perhaps we can remove the CORS configuration from the nginx config.

@giohappy what is your take on this?

@t-book
Copy link
Contributor Author

t-book commented May 15, 2024

Thanks for your reply @ridoo . I was puzzled if I oversee something and guessed for sure others have to deal with cors as well. django-cors-header would be a good option in my opinion.
Another appraoch might be to just use envsubset as the current nginx configuration does?

Something like

if ($request_method = OPTIONS) {
    add_header Access-Control-Allow-Headers "Authorization, Content-Type, Accept";
    add_header Access-Control-Allow-Credentials true;
    add_header Content-Length 0;
    add_header Content-Type text/plain;
    add_header Access-Control-Max-Age 1728000;
    add_header Access-Control-Allow-Methods "$ALLOWED_METHODS";
    add_header Access-Control-Allow-Origin "$ALLOW_ORIGIN";
    return 200;
}

...

envsubst "$defined_envs" < /etc/nginx/nginx.conf.envsubst > /etc/nginx/conf.d/default.conf

@ridoo
Copy link

ridoo commented May 15, 2024

Well, in my opinion CORS should be configured at the application (geoserver is doing it this way too). However, there may have been reasons (did not investigate) to configure CORS for GeoNode at nginx.

What I can see: django-cors-headers is already available, so we could just move the CORS config from nginx to a proper application config. Also, in a development setup you usually do not have nginx upfront.

@t-book
Copy link
Contributor Author

t-book commented May 15, 2024

Good points @ridoo !

@giohappy
Copy link
Contributor

At the application level, we already have a configuration for it: CORS_ALLOW_ALL_ORIGINS.

Of course, it has effects only if the HTTP server in front of the application relays the configuration of the headers to the application.
Also, Geoserver's CORS configurations have effect only if Tomcat is configured accordingly.

We must also keep into account that certain requests are managed directly by Nginx (for example access to filesystem resources). If a deployment wants to block CORS for these requests it must be done at the Nginx level.

I don't think there's a one fits all solution, and as @ridoo says GeoNode could also be employed without Nginx as its frontend server. For the typical deployment with Docker and Nginx, I'm in favor of adding a new variable to the Nginx conf as @t-book suggests.

t-book added a commit to csgis/geonode-docker that referenced this issue May 20, 2024
@ridoo
Copy link

ridoo commented May 22, 2024

Also, Geoserver's CORS configurations have effect only if Tomcat is configured accordingly.

@giohappy AFAIK GeoServer CORS config is "Servlet Container only", i.e. there is no application check on GeoServer, right? Setting up CORS for GeoNode and GeoServer, I find it natural to configure it at a similar layer (behind nginx in this case) -- but this might be a personal preference.

We must also keep into account that certain requests are managed directly by Nginx (for example access to filesystem resources). If a deployment wants to block CORS for these requests it must be done at the Nginx level.

Accessing those static resources can be considered to be safe as you'd do not state change on the server, right? If you really want to block access on those, CORS would not be enough as you always could do the request through a proxy.

@giohappy
Copy link
Contributor

I'm fine with enabling CORS in Nginx, without introducing additional configurations.
As you say @ridoo they can still be blocked by Django and Tomcat (for Geoserver).

I cannot test the proposed configuration by @t-book but I would rather use a static one, without envsubst.

@ridoo
Copy link

ridoo commented May 23, 2024

@giohappy well, actually I would say configuration at nginx is not necessary :-).

CORS config at Django site is already possible and is more flexible even. You'd have to sync CORS config manually or by introducing more and more variables. However, I think that blocking static resources via CORS is not necessary at all (as they still can be accessed) and blowing up config for that reason makes no sense IMO.

My current take is to remove the nginx CORS config in favor of configure it via django-cors-header only.

@giohappy
Copy link
Contributor

My current take is to remove the nginx CORS config in favor of configure it via django-cors-header only.

@ridoo I think we're saying the same thing :)
Actually, I don't think we have CORS configs now in Nginx. My understanding is that @t-book is proposing to configure Nginx to support CORS, and the proposal is to enable them statically by simply adding add_header Access-Control-Allow-Origin "*". Am I wrong?

@ridoo
Copy link

ridoo commented May 23, 2024

Ah .. you're completely right. Sorry for confusing the current status with the proposed one.

@t-book
Copy link
Contributor Author

t-book commented May 23, 2024

@ridoo Thanks for your reply in the PR. I will continue the discussion here.
As @giohappy said. My intention just was to solve the cumbersome problem with A-C-A-O *. Which ended up having to manage a fork and not just using the upstream image.

I'm unsure if adding

CORS_ALLOW_METHODS
CORS_ALLOW_HEADERS
CORS_PREFLIGHT_MAX_AGE
CORS_ALLOW_CREDENTIALS

to nginx might in the end in conflict with settings from django … If things get confusing as the same settings can be set in different places. But sure I'm open to change the PR to add those as well.

@ridoo
Copy link

ridoo commented May 23, 2024

@t-book As far I can see, you could just set these in the settings.py without touching nginx config. No need to maintain an own fork for this. The mentioned settings are all covered by the django-cors-header app which should be available by default.

@t-book
Copy link
Contributor Author

t-book commented May 23, 2024

thanks @ridoo in past my tests did not succeed without setting them explicitly in options config of ngix. But I will give it a try again!

@ridoo
Copy link

ridoo commented May 23, 2024

@t-book if it does not work as expected please let me know. Curious about the settings then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants