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

REDASH_MULTI_ORG broken in Angular 1.5 #1447

Closed
adamlwgriffiths opened this issue Dec 5, 2016 · 12 comments
Closed

REDASH_MULTI_ORG broken in Angular 1.5 #1447

adamlwgriffiths opened this issue Dec 5, 2016 · 12 comments

Comments

@adamlwgriffiths
Copy link
Contributor

The latest code in master no longer works with REDASH_MULTI_ORG=True.

What I can gleam is that the asset requests are being thought to be an organization (http://host/app.js -> http://host/org where org: app.js).
I think there may have been some logic for this in the template previously, but it's been lost.

The output is as follows (I've added a print for the SLUG = X in authentication/org_resolving.py):

[2016-12-05 11:51:45,164][PID:5358][CRITICAL][root] SLUG = styles.css
11:51:45 web.1    | [2016-12-05 11:51:45,167][PID:5358][ERROR][redash] Exception on /styles.css/ [GET]
11:51:45 web.1    | Traceback (most recent call last):
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask/app.py", line 1817, in wsgi_app
11:51:45 web.1    |     response = self.full_dispatch_request()
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask/app.py", line 1477, in full_dispatch_request
11:51:45 web.1    |     rv = self.handle_user_exception(e)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask_restful/__init__.py", line 271, in error_router
11:51:45 web.1    |     return original_handler(e)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask/app.py", line 1381, in handle_user_exception
11:51:45 web.1    |     reraise(exc_type, exc_value, tb)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask/app.py", line 1475, in full_dispatch_request
11:51:45 web.1    |     rv = self.dispatch_request()
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask/app.py", line 1461, in dispatch_request
11:51:45 web.1    |     return self.view_functions[rule.endpoint](**req.view_args)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask_login.py", line 790, in decorated_view
11:51:45 web.1    |     elif not current_user.is_authenticated:
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/werkzeug/local.py", line 343, in __getattr__
11:51:45 web.1    |     return getattr(self._get_current_object(), name)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/werkzeug/local.py", line 302, in _get_current_object
11:51:45 web.1    |     return self.__local()
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask_login.py", line 47, in <lambda>
11:51:45 web.1    |     current_user = LocalProxy(lambda: _get_user())
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask_login.py", line 858, in _get_user
11:51:45 web.1    |     current_app.login_manager._load_user()
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask_login.py", line 389, in _load_user
11:51:45 web.1    |     return self.reload_user()
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/flask_login.py", line 351, in reload_user
11:51:45 web.1    |     user = self.user_callback(user_id)
11:51:45 web.1    |   File "redash/redash/authentication/__init__.py", line 40, in load_user
11:51:45 web.1    |     return models.User.get_by_id_and_org(user_id, current_org.id)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/werkzeug/local.py", line 343, in __getattr__
11:51:45 web.1    |     return getattr(self._get_current_object(), name)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/werkzeug/local.py", line 302, in _get_current_object
11:51:45 web.1    |     return self.__local()
11:51:45 web.1    |   File "redash/redash/authentication/org_resolving.py", line 19, in _get_current_org
11:51:45 web.1    |     org = Organization.get_by_slug(slug)
11:51:45 web.1    |   File "redash/models.py", line 302, in get_by_slug
11:51:45 web.1    |     return cls.get(cls.slug == slug)
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/peewee.py", line 4012, in get
11:51:45 web.1    |     return sq.get()
11:51:45 web.1    |   File ".anaconda/envs/redash/lib/python2.7/site-packages/peewee.py", line 2645, in get
11:51:45 web.1    |     % self.sql())
11:51:45 web.1    | OrganizationDoesNotExist: Instance matching query does not exist:
11:51:45 web.1    | SQL: SELECT "t1"."id", "t1"."updated_at", "t1"."created_at", "t1"."name", "t1"."slug", "t1"."settings" FROM "organizations" AS t1 WHERE ("t1"."slug" = %s)
11:51:45 web.1    | PARAMS: [u'styles.css']
@adamlwgriffiths
Copy link
Contributor Author

Any file under client/dist will not be resolved.
Putting a prefix before the client resource will change the error to a 404.
Interestingly the results are 301'ed to a path with / post-fixed, as seen in this command:

$ $ wget http://127.0.0.1:5000/app.js
--2016-12-05 15:31:00--  http://127.0.0.1:5000/app.js
Connecting to 127.0.0.1:5000... connected.
HTTP request sent, awaiting response... 301 MOVED PERMANENTLY
Location: http://127.0.0.1:5000/app.js/ [following]
--2016-12-05 15:31:02--  http://127.0.0.1:5000/app.js/
Connecting to 127.0.0.1:5000... connected.
HTTP request sent, awaiting response... 500 INTERNAL SERVER ERROR
2016-12-05 15:31:02 ERROR 500: INTERNAL SERVER ERROR.

The send_static handler is also not called despite it having the route '/<path:filename>'.

Files under redash/static will resolve properly.
This includes /robots.txt which rules out the issue being the assets in the root path.

@adamlwgriffiths
Copy link
Contributor Author

Adding specific routes for the files in /dist allows the assets to load:

from flask import ..., request

@routes.route('/app.js')
@routes.route('/vendor.js')
@routes.route('/manifest.js')
@routes.route('/styles.css')
@routes.route('/<path:filename>')
def send_static(filename=None):
    filename = filename or request.path[1:]
    ....

Redash however, does not load properly.
I am left with a blank screen (with the styled background grey colour), but no menu bar, etc.
There are no network calls to attempt to load the dashboard list or any resources, just the asset files are requested and loaded. There are no errors in the Chrome JS console.

@arikfr
Copy link
Member

arikfr commented Dec 5, 2016

The multi tenancy support is not officially supported or documented as it is something I introduced as provisioning for the hosted version. This is why I felt comfortable breaking it in the webpack branch until I will have the time to fix it.

There are two issues here:

  1. The index path (/<org_slug>) takes precedence over the static route. The workaround for this was the SlugConverted. This is why it worked for you with robots.txt. We need to find a better/more robust solution here -- maybe serve all assets from /assets/?

  2. We were setting the base's href value to the organization prefix when rendering the template in Flask. Now that we no longer render this as a template, but rather send as is we need to find an alternative solution.

Some options:

  1. Send index.html as a template and update the base href when rendering if needed. I prefer to avoid this, as I want to make the client app completely independent from the backend.
  2. Update the href in the Angular code before starting the app. This might just work, but requires us to make this update right before doing any other call. Might prove to be hard.
  3. Manually update API calls and links to use the org slug prefix. Feels like a lot of work and has marginal benefits.
  4. Use subdomains to decide which organization account I'm using now. This was my original plan, but then I had issues with Google OAuth, so decided to use a single domain with each org as a "folder"

I like option #2 or #4. The subdomain option is probably the best, but might take time to migrate so prefer "to_parse".

@arikfr
Copy link
Member

arikfr commented Dec 5, 2016

(opened #1449, #1450, #1451 to track additional open issues with this version)

@arikfr
Copy link
Member

arikfr commented Dec 23, 2016

@adamlwgriffiths did you look into it more?

@adamlwgriffiths
Copy link
Contributor Author

adamlwgriffiths commented Dec 24, 2016 via email

@adamlwgriffiths
Copy link
Contributor Author

As per your comment above, there may also be a way to set the base url during the asset build stage?

Are there any plans to take a look at this?
I don't have the ability to make modifications to the NodeJS app / build process. We're stuck on the Angular 1.2 codebase until we can get multi-org support.

@arikfr
Copy link
Member

arikfr commented Feb 7, 2017

As per your comment above, there may also be a way to set the base url during the asset build stage?

No, because the base URL depends on the current org_slug, so it's something that needs to be done in runtime.

Are there any plans to take a look at this?

Yes.

@arikfr
Copy link
Member

arikfr commented May 2, 2017

Finally I got around to looking at this again and I had this thought: what if we drop org_slug from the URL?

I feel like the org_slug in the url adds some extra layer of protection against confusion of which account you're on and leaking data between tenants... but do we really need it?

@adamlwgriffiths
Copy link
Contributor Author

I'm not really fussed.
From a resource / URL point of view it makes sense to use an org in there as part of the URL hierarchy. But not at the cost of delaying, perhaps indefinitely, this feature.

@arikfr
Copy link
Member

arikfr commented May 3, 2017

For sure it won't be indefinitely -- I need this too :) I just didn't want to block the 1.0 release with it.

arikfr added a commit that referenced this issue May 3, 2017
@arikfr
Copy link
Member

arikfr commented May 15, 2017

I merged the branch that supposed to fix this. It seems to be working properly. It has a different workflow for development than the single tenant one (you can't use webpack-dev-server with it).

I still want to review all APIs to make sure that in the migration to SQLAlchemy we didn't break multi-tenancy protection anywhere...

@arikfr arikfr closed this as completed May 15, 2017
alison985 pushed a commit to alison985/mozilla-redash that referenced this issue Jun 14, 2017
dairyo pushed a commit to KiiCorp/redash that referenced this issue Mar 1, 2019
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

No branches or pull requests

2 participants