Fix hardcoded urls in python and template files.

[joharohl]

This PR is in relation to issue #985.

The first commit basically converts all existing hard coded urls in superset to use the Flask url_for method to generate urls.

The second commit adds a middleware that runs all tests under the prefix /test to make sure that future contributions will not add in hard coded urls again. It also fixes current tests that wouldn't work with the new prefix.

I need to clean some of this up a bit, but wanted to create the PR now to get some feedback before I do much more with it.

If you want to go with running tests under a prefix, there are a some complications due to the fact that url_for needs the application context in order to resolve the urls. And it needs to be prepared with the right prefix to give the right urls. In addition, any test code that runs app code which generates urls need to wrap this code in an app context as well (exceptions being self.client.get/post). I have added a number of help methods to the SupersetTestCase class that should help people out (self.url_for, self.get_app_context. It still requires a bit more from the test code though since app context mishaps give very confusing errors.

Anything I missed, should change, etc., Please let me know

[xrmx]

Thanks for the PR, I would have left the tests untouched and added a single test to check that the middleware is working. Otherwise we are not testing that our urls are stable. Please wait for @mistercrunch and @bkyryliuk opinions before updating the PR.

[mistercrunch]

I'm open ot the idea of doing this.

Though what about the hard coded urls on the .js side?

We should also add tests around the FAB urls.

And to @xrmx's point, we need to make sure that all current URLs are unchanged as users have bookmarks.

[joharohl]

@xrmx thought we would do the python stuff first and then tackle the .js. I have something working for the .js as well, but need some feedback since it's not my strongest area.

I didn't think about bookmarks, good point. I still think it's necessary to run the tests under a prefix, otherwise hard coded urls will creep back in. How about manually adding the prefix to urls in the tests? That way it should also fail if someone changes an endpoint.

The FAB urls seems pretty good. The only one I have found so far is in AuthViewDB.login which uses a / redirect instead of appbuilder.get_url_for_index for when a user is already logged in. It doesn't really brake anything catastrophically though.

[mistercrunch]

Awesome. I have [recently obtained] merge priviledges on the FAB repo if we need to change anything on that side.

I'm assuming your end goal is to add a prefix (subfolder) in the URL for the whole site for reverse-proxying, so where in the configuration would you set this prefix? I was looking for a URL_PREFIX element in config.py as part of your PR but didn't see it.

[joharohl]

Cool with the FAB merge powers. I might have a more thorough look for hard coded urls there as well then.

I'd go for not adding a URL_PREFIX setting. Although a nice shortcut for people, I'd say it's good to get them to think about what they are doing. Especially since getting it working not only requires setting up Flask correctly, but also the proxy server.

It's fairly straight forward to setup the proxying, so I think it should be left as an exercise to the reader, with good hints in the documentation of course.

[rsxm]

@mistercrunch I ran into the same issue when trying to host Superset and our company's Django-based apps on the same domain under different paths (the primary purpose being not having to deal with CORS when embedding/interacting with graphs). The django apps required no changes or configuration, I just had to set the SCRIPT_NAME paramater in my nginx location config (see below) and the rest was handled by WSGI (PEP-0333) and Django.

I thus agree with @joharohl that all Superset needs to do is not have hard-coded URLs. Especially with #1832, the ReverseProxied middleware @joharohl referenced is easy to add to your local superset_config.py.

Example nginx location setting the SCRIPT_NAME environment variable:

location /d/ {
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Scheme $scheme;
    proxy_set_header Host $http_host;

    # Django running on host using Gunicorn
    proxy_set_header SCRIPT_NAME /d;
    proxy_pass http://django_server/d/;
}

[rsxm]

@joharohl I'm not sure what you have in mind for the javascript side of things, but what I'm currently doing for my projects is to make an AJAX request at the top of each of my javascript files and then get the SCRIPT_NAME value from the response header:

let request = new XMLHttpRequest();
request.open('GET', '/health');
request.send();
const SCRIPT_NAME = request.getResponseHeader('SCRIPT_NAME');

Then I construct all my javascript urls from the SCRIPT_NAME variable. This approach can be improved by storing the SCRIPT_NAME value in a cookie (or other form of local storage) and only making the ajax request if cookie value is not set.

I have no idea if this is a solution, a workaround or a dirty hack, but it allows the client-side code to be independent from the server-side and have both honor the "SCRIPT_NAME" header set by the proxy server.

*Got the idea from here

[joharohl]

@jr-minnaar I was lazy and just templated js constants in superset/templates/superset/basic.html like so:

const serverRoot = "{{ request.script_root }}";
const staticRoot = "{{ url_for('static', filename='') }}";

I then just used these for all the urls in .js code.

I'm all for either approach, but I have the nagging feeling that there is a better way to do it.

[xrmx]

Another option would be to add the needed prefix at build time through webpack.

[rsxm]

@joharohl Thats not lazy, that's efficient! I think when there is no need to separate client-side from server-side (meaning you can use Jinja templates to pass variables to javascript), then your solution is far more robust.

@xrmx, I'd prefer a solution like @joharohl described where the SCRIPT_NAME is entirely controlled by the proxy server. This allows for really cool setups where the SCRIPT_NAME doubles as the authentication mechanism on intranets. E.g.:
http://example.intranet/user1/superset/welcome logs in user1 and shows the dashboards they're authorised for through the REMOTE_USER authentication method. Then user2 can simply go to http://example.intranet/user2/superset/welcome for their dashboards.

[joharohl]

We also need a dynamic prefix/subdir for our usecase. We want to do multi-tenancy on the path so we need one instance of superset to be able to serve many different paths. It's still possible if the prefix is added on build time, but a lot more painful.

To not derail this much further, I think I should compile my JS changes and do another PR and we can discuss it there.

On topic: is it fine to change the tests to hardcode /test/... for the urls? If so, I can do that and update the PR.

[mistercrunch]

What is the best way to prove that URLs aren't changing (insuring that bookmarks point to the right place when running the default configuration)?

Maybe deriving the CoreTests into another class PrefixedUrlTests and make sure that both run? I'm not sure how it will impact the time it takes to run the tests, though in theory they could be run in parallel.

[joharohl]

It's probably fairly easy to run the tests twice, once with a prefix and once without. If you feel doubling the number of tests is fine, I can go ahead and make that change

[xrmx]

@joharohl it's not that it's difficult it's that it'll make tests longer to run :) unless made parallel too.

[joharohl]

Which is why I asked if you really want to run the tests twice ;).

I don't see any good way of making sure that the original urls are not changed. I personally think always running tests under a prefix is fine, but I got the impression that's not enough.

[joharohl]

@xrmx, @mistercrunch
How about being able to toggle the use of a prefix during tests with an environment variable? That way we only need to add one more test round in tox.ini. 1+7 is a lot less than 7+7.

It's less robust perhaps than running the tests with a prefix on all different variations, but it feels like it should be fine, url_for shouldn't really be very sensitive to variations in database or python version.

[xrmx]

@joharoh l was thinking on adding just a test case, we want to check that the this work in every platform and not just in the one with the flag set no?

Somethings like this in tests/url_tests.py:

"""Unit tests for Superset"""
from .base_tests import SupersetTestCase


class UrlTestCase(SupersetTestCase):
    requires_examples = True
    url_prefix = None

    def __init__(self, *args, **kwargs):
        super(UrlTestCase, self).__init__(*args, **kwargs)
        # this could be moved to SupersetTestCase of course
        if self.url_prefix:
            # FIXME: do whatever you need to wrap the app 

    def test_client(self, url, data, whatever)         
        # FIXME: extend self.client to inject automatically the prefix

    def test_welcome_url(self):
        self.login()
        resp = self.test_client.get('/superset/welcome')
        assert resp.status_code == 200

   def test_whatever_url(self):
       ....

class PrefixUrlTests(UrlTestCase)

    url_prefix = '/test/'

    def test_that_url_prefix_works(self):
        self.login()
        resp = self.client.get('/test/superset/welcome')
        assert resp.status_code == 200
        resp = self.client.get('/superset/welcome')
        assert resp.status_code == 404

    # the inherited tests should run fine 

[joharohl]

Ah, I see. I was obviously a bit confused what you were trying to get at earlier.

I think UrlTestCase is fine for making sure bookmarks survive.

I'm not convinced that the approach in PrefixUrlTests would actually stop anyone adding hard coded urls without a lot of work. All urls that are exposed in views.py with @expose will by default work under a prefix. The problem are links to static files and other resources in the rendered pages as well as redirects within the app itself.

To catch hard coded urls in those cases, there has to be tests in place that asserts that generated urls have the proper prefix. In addition, all redirects have to be tested.

By having the current tests run under a prefix it is possible to at least test a lot of the redirects, but I have no idea how the coverage looks like. I don't think there are many tests that look at hrefs in the templates.

So, I'm seeing two main routes here:

1. Just add the UrlTestCase with important urls to make sure I didn't screw something up. No changes to tests to make them work under a prefix. Add documentation to make everyone use url_for instead of hard coded urls.
2. Add the PrefixUrlTests that really probe for use of hardcoded urls. Document the practice of filling up the PrefixUrlTests with more tests as urls and/or behavior are changed/added.

I'm fine with 1. It will mean there will be the occasional hard coded url slipping in, but it should be easy to patch when it happens. The few of us that need to be able to run superset under a prefix just has to be a bit careful with the upgrades :)

[xrmx]

I'm fine with just 1 either too but please add a testcase checking that loading the flask app under a different path works.

[joharohl]

@xrmx @mistercrunch

I have updated the PR now to follow point 1 in my previous comment. I added a few urls that felt like it made sense, but you probably have more of an idea how superset is used and which urls are important to test. So please let me know which additional urls to add.

I feel we don't need to test the FAB urls because they work and should be the responsibility of FAB anyway. If you still want to include some of them (like the Dashboard model view e.g.) just let me know.

[HugoPu]

Can you solve these conflicts? or When will you solve these conflicts?

[joharohl]

There wasn't any conflicts when I updated this PR in January. I never got any feedback after that, so assumed it wasn't very highly prioritized. We have since started maintaining or own fork of superset so we are no longer in need for this to be merged.

Updating this PR right now will most likely involve going through the code again and look for more hardcoded urls. Since it's been a while (both time and commit wise) I'm expecting this to take some time. If you think this type of PR is useful I'd be happy to do it.

I'm wondering though if it's better to close this and #985 and accept that superset isn't supported running under a prefix? It's fairly complicated to maintain, and the number of users that need it is most likely small.

[rsxm]

I'm also maintaining a (fragile) fork based on this pull request. I still hope this will eventually get merged as that would save me a lot of rebasing and cherry picking effort. But I agree with @joharohl, resolving conflicts now without merging will be effort better spent elsewhere.

[mistercrunch]

Things aren't looking good as far as merge conflicts go...

[joharohl]

@xrmx Hardly surprising as it was almost 3 months and 277 commits to master since I last updated the PR.

I just need to know if this is a type of contribution that you are interested in. If yes, as stated above I'm happy to do the necessary adjustments to get things in line with master. If not, please close this PR and #985 as won't fix.

[djk2]

These changes would also be helpful for me.

[dgajewski1]

@joharohl I ran into the same issue with prefix. Is this a lot of work to get it merged and maybe resolve new hardcoded urls? It will be very useful.

[joharohl]

@gajes03

It's not that much work to fix all the urls. The problem is more on the testing side where it would be good to be able to still use hardcoded urls to make sure the relative ones don't break existing links. I have some ideas I wan't to try, but need some time to sit down and try them out.

I'm a bit pressed for time right now, but I will try and revisit this in a few weeks time

[mistercrunch]

Closing old PRs with broken build or merge conflict, feel free to re-open once merge conflicts are sorted out.

[vijaybandari]

Any alternative solutions until you guys fix it?

[mistercrunch]

If someone wants to start this over, I'd recommend writing a small PR with a proof of concep with a single URL on the client and server side so that it doesn't merge conflict with anything. Once this lands we can go section by section and get some cadence.

The alternative is some good old proxying.

[pawel-pxp]

Wanted to confirm - since in my org we wanted to deploy superset behind nginx reverse proxy under different prefix - and now that does only works on some links - so basically the effort is killed?

Any other solutions? examples, hints ? Some fancy url rewrite ?

[xrmx]

@mrpapini Does ENABLE_PROXY_FIX work for you?

[pawel-pxp]

Hi - thanks for response - no it does not - I actually tweaked the code from werkzeug.contrib.fixers.ProxyFix to parse HTTP_X_SCRIPT_NAME in addition what they have there - some links work - some still not - for instance sqllab links will not work - of course this is with conjuction with nginx reverse proxy config - of course if location is just "/" all works fine but if location is say "/apps/" it does not - just looking for proper combination of nginx parameters - but judging from this thread and others seems this is not possible unless some heavy patching of entire code base is done

[joharohl]

Hey @mrpapini

A solution that will avoid patching the code might be to do rewrites in the html in nginx. The sub_filter module should be able to do the trick. this stack overflow looks promising.

Might have a look myself soonish as this would probably be the easiest way of solving this issue.

[pawel-pxp]

Thanks @joharohl - seen this one - didn't try it yet - will do today - first need to compile that module in.

As a note, there is lot's of try this - try that but no definitive answer - I need to get to the bottom of this - like you said - even you didn't really try it so all these are guesses now

[pawel-pxp]

Hey @joharohl - so looks like sub_filter will work - I'm in process of figuring out exact pattern but so far I'm getting good results - once I have the nginx setting working I'll post it here

[Webgardener]

Hello @mrpapini .
I also needed an /apps location for Superset. The werkzeug.contrib.fixers.ProxyFix middlware helped a lot :) But still had issues with harcoded urls and redirection (superset/welcome ...).

Here my nginx configuration to make it work completely :

  server {
   listen       superset:80;
   server_name  superset;
   location /apps {
     proxy_pass http://0.0.0.0:8088;
     proxy_set_header Host $host;
     proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
     proxy_set_header X-Scheme $scheme;
     proxy_set_header X-Script-Name /apps;
   }
 
   location ~ ^/(static|superset|sqllab|savedqueryview) {
     try_files $uri /apps/$uri;
   }
 
 }

[pawel-pxp]

@Webgardener - thanks for the tip - but I have to say I just tried it and that didn't work
I'm using latest superset from GitHub and latest nginx 1.13.5 on mac
maybe there is something else you had configured?

basically i swtiched ENABLE_PROXY_FIX=True
and copy your nginx config

should your location be location /apps/ { ... instead of location /apps { ... (trailing slash) ?

[Webgardener]

@mrpapini

server {
    server_name YOUR_SERVER_NAME;
    location /YOUR_PREFIX/ {
        proxy_pass         http://YOUR_PROXIED_SERVER:PORT;
        proxy_set_header Host $host;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Scheme $scheme;
        proxy_set_header X-Script-Name /YOUR_PREFIX;
   }
   location  ~ ^/(static|superset|sqllab|savedqueryview|druid|druiddatasourcemodelview)/ {
       try_files $uri /YOUR_PREFIX/$uri;
   }
}

You have to replace the uppercase text with your values. Since you work on a mac, I assume Nginx and Superset run on the same "local" machine.

Here is the config on my local machine :

in /etc/nginx/sites-enabled/superset.conf:

server {
    server_name superset.local;
    location /analytics/ {
        proxy_pass         http://superset.local:8088;
        proxy_set_header Host $host;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Scheme $scheme;
        proxy_set_header X-Script-Name /analytics;

    }
    location  ~ ^/(static|superset|sqllab|savedqueryview|druid|druiddatasourcemodelview)/ {
	try_files $uri /analytics/$uri;
    }
}

In /etc/hosts:
127.0.0.1 superset.local

About the trailing slash : it depends on what you need. The behavior is different whether you add a trailing slash or not.

On the second location, without trailing slash:
location ~ ^/(static|superset|sqllab|savedqueryview|druid) {

• http://superset.local/druiddatasourcemodelview/list/ is OK

with trailing slash : location ~ ^/(static|superset|sqllab|savedqueryview|druid)/ {

• http://superset.local/druiddatasourcemodelview/list/ is KO

On the first location, since the requests are processed by proxy_pass, the behavior is special:

location /analytics/ {
        proxy_pass         http://superset.local:8088;
}

and go to http://superset.local/analytics, nginx will automatically redirect to http://superset.local/analytics/

[Txalamar]

Hi,

I'm triyng to get working superset as a subfolder of a virtual host:

http://my.server.com/superset/

Been unable so far. Is the previous config suitable to achieve this? If i replace YOUR_PREFIX for superset I end up in an endless loop:

"rewrite or internal redirection cycle while internally redirecting to /superset//superset//..."

Thank you.

[Webgardener]

Hi.

Here is the content of my nginx config file :

location /analytics {
            proxy_set_header Host $host;
            proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
            proxy_set_header X-Script-Name /analytics;
            proxy_pass http://YOUR_SERVER_NAME:8088;
            #YOUR_SERVER_NAME is localhost if both nginx and superset run on same server
}

location ~ ^/(static|superset|sqllab|savedqueryview|druid|tablemodelview|databaseasync|dashboardmodelview|slicemodelview) {
  try_files $uri /analytics/$uri /analytics/$uri?$query_string @rules;
}  

location @rules {
  rewrite ^(.*)$ /analytics$1 permanent;
} 

Also, don't forget to put the middleware in the superset_config.py file :

 class ReverseProxied(object):
 
   def __init__(self, app):
        self.app = app

    def __call__(self, environ, start_response):
        script_name = environ.get('HTTP_X_SCRIPT_NAME', '')
        if script_name:
            environ['SCRIPT_NAME'] = script_name
            path_info = environ['PATH_INFO']
            if path_info.startswith(script_name):
                environ['PATH_INFO'] = path_info[len(script_name):]

        scheme = environ.get('HTTP_X_SCHEME', '')
        if scheme:
            environ['wsgi.url_scheme'] = scheme
        return self.app(environ, start_response)

ADDITIONAL_MIDDLEWARE = [ReverseProxied, ]

[KalterTod]

lgtm

[VinodLouis]

@Webgardener Thanks that works for nginx Is this possible with a haproxy settings ?

[mohan-kumar]

@Webgardener
your config for nginx works for almost all links except filters in the dashboard.
Filters are not working. Is there any workaround?

[pandamien]

Hi all, i have been working on the same issue for a while and i have some suggestions,
the best way to workaround is to rely on reverse proxy configuration which is not very appropriate, you will end up with a configuration like this:

location ~ ^/(users|userstatschartview|roles|permissions|permissionviews|viewmenus|logout|login|logmodelview|dashboardasync|databaseasync|csstemplateasyncmodelview|druid|druidclustermodelview|druiddatasourcemodelview|csstemplatemodelview|csvtodatabaseview|sliceaddview|dashboard|chart|databaseview|tablemodelview|queryview|annotationlayermodelview|annotationmodelview|lang)/ {
    		auth_basic off;
		rewrite ^/(.*) /MY_PREFIX/$1 permanent;
    		proxy_pass http://MY_ADDRESS:MY_PORT;
    		proxy_set_header Host $host;
	}

as you can see you need to match all the hard codded path that are being redirected to the default root context ('/'). i will say this is ugly. There might be some further modifications in these paths which make the maintenance not very interesting.
1- The first solution i'm proposing is to make all the routes uniform by adding /superset, then you can rewrite the URL in nginx configuration like this:

location ^~ /MY_PREFIX/ {
    		auth_basic off;
    		proxy_pass http://MY_ADDRESS:MY_PORT;
    		proxy_set_header Host $host;
	}
		
	location ^~ /superset/ {
    		auth_basic off;
		rewrite ^/superset/(.*) /MY_PREFIX/superset/$1 permanent;
    		proxy_pass http://MY_ADDRESS:MY_PORT;
    		proxy_set_header Host $host;
	}

this is working well as long as all the paths start with /superset, but because there are many other paths that does you have to match all of them like mentioned above. Even if it will still be hard coded at least it will make the workaround much easy to maintain and reliable.

2- And the second which might be less easier but good for long term is to define a global parameter configuration in the config file like (URL_PREFIX), which should be added as base path to all the routes, then get rid of the hard coded URL.
But even if there is still hard codes like redirects make sure to add the base path to it.
This issue is been open for 2 years now and i really hope it can be fixed.
Also @joharohl has done a pretty job with this pull request https://github.com/apache/incubator-superset/pull/1866/files and i think it deserve to be reviewed, applied to all hardcoded, and merge to the master branch.

Thanks.

[mohan-kumar]

#1866 (comment)
This solution works fine for now. Additional paths might be needed to make it work for all links.
I had to change
location @rules {
rewrite ^(.*)$ /analytics$1 permanent;
}

to location @rules {
rewrite ^(.*)$ /analytics$1 last;
}
to make filters work.

[xentity]

#1866 (comment)
This does not work for me (nginx version: nginx/1.14.0 (Ubuntu) and superset 0.28.1 on different servers). We wanted to evaluate Superset, but I think about to give it up.

My trys ended up in cascades of https://host.xyz/superset/superset/superset/[...]/welcome, in lots of 404s, never-ending redirects or something similar.

I even configured nginx to listen to 8443 (since 443 is bounded to a bunch of other applications) to pass everything to the superset server. But even this ended up in an error: "The referrer does not match the host."

To be honest, I came in touch with several (>50) semi professional and professional web applications as an administrator of reverse proxies and web application firewalls. Even the worse ones were able to define a url prefix or were able to work behind a reverse proxy without breaking the fingers. This is a must have for web applications nowadays. (To be fair, it seems to be more a flask issue than a Superset issue, maybe flask is not the right choice.)

I like the idea of hiding applications behind url prefixes for public access-able services, because the most scanners try to tackle / and a bunch of well-know directories and nothing more. In addition to that it is much, much easier for http based routing, if you have one central entry point.

Any hint to solve it is appreciated.

[kirankumarpv]

Hi,

I wanted to run 'Superset' in complete serverless environment on AWS. Basically, want to run superset on AWS Lambda.

I have tried different methods recommended and all of them resulted in some or other place break of functionality. Eventually what i did was the below to make it work:

1. In config.py i have added another configuration variable and used this variable in locations where redirect or appbuilder.add_link has been used.
2. In templates folder there are places where directly '/superset/' has been used. So, even if i did first step right, the templates are not rendering in right way. So, i have to go and change the template as well (As of now I have hard-coded this. I need to make it configurable)
3. In front-end i have added a file called config.ts and I have used this config in locations wherever redirect was done in front-end. This has fixed up all my front-end links.
4. Only thing remaining for me was fixing "Upload CSV to Database" Link. When we click this link and enter the data, since Lambda doesn't allow any writes i tried to write to /tmp - but since we don't know whether the next request is going to be served by same lambda or not... so this is an issue as of now. The way I am planning to fix this is to write the files to s3 instead of local folder. I am still figuring out a way to do this.

After above all are done, I have used AWS Aurora Serverless as my metadata database. This way I don't even need to pay for the database since Aurora is MySQL version.

Hope this helps. Once i fix up the remaining this, I need to check how to make this available to all.

Thanks
Kiran

[christophlingg]

I found it very frustrating to setup a base url for superset. If you want to save some time, I condensed a couple of comments into a working example here: https://github.com/komoot/superset-reverse-nginx-example

[ebuildy]

@christophlingg this is just doing the job for the HTML page, all other stuff (static CSS, ...) is not using URL prefix and should never hit the right backend server (superset).

Most entreprise users are using software behind proxy, for infrastructure / security / auth reasons. At X.com, we are running all analytics services behind /analytics/{service} URL, most softwares provide settings for that (https://grafana.com/tutorials/run-grafana-behind-a-proxy/#1), this PR look awesome, any plan to finish it?

[christophlingg]

yes @ebuildy , I eventually gave up and went for a dedicated subdomain for superset

[ichux]

#1866 (comment) This solution works fine for now. Additional paths might be needed to make it work for all links. I had to change location @rules { rewrite ^(.*)$ /analytics$1 permanent; }

to location @rules { rewrite ^(.*)$ /analytics$1 last; } to make filters work.

Thanks for this. It helped solve some issues