GeoNode / geonode-docker

Django base images for GeoNode
Other
9 stars 33 forks source link

External SPA applications allow origin #42

Open t-book opened 4 months ago

t-book commented 4 months ago

Hi,

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

https://github.com/GeoNode/geonode-docker/blob/62a0d29f6cba48b2dca193fc0d66307a0387c79d/docker/nginx/geonode.conf.envsubst#L93-L101

@@ -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

ridoo commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Good points @ridoo !

giohappy commented 4 months ago

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.

ridoo commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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

t-book commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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