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

Remove workaround from check_csrf() #6919

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Conversation

eradman
Copy link
Collaborator

@eradman eradman commented Apr 22, 2024

What type of PR is this?

  • Bug Fix

Description

This code was supposed to be temporary, and raises an exception if REDASH_MULTI_ORG=true is set.

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2213, in __call__
    return self.wsgi_app(environ, start_response)
  File "/usr/local/lib/python3.8/site-packages/werkzeug/middleware/proxy_fix.py", line 182, in __call__
    return self.app(environ, start_response)
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2193, in wsgi_app
    response = self.handle_exception(e)
  File "/usr/local/lib/python3.8/site-packages/flask_restful/__init__.py", line 298, in error_router
    return original_handler(e)
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2190, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1486, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.8/site-packages/flask_restful/__init__.py", line 298, in error_router
    return original_handler(e)
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1482, in full_dispatch_request
    rv = self.preprocess_request()
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1974, in preprocess_request
    rv = self.ensure_sync(before_func)()
  File "/app/redash/security.py", line 43, in check_csrf
    dest = f"{view.__module__}.{view.__name__}"
AttributeError: 'NoneType' object has no attribute '__module__'

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A
  1. Add REDASH_MULTI_ORG=true to .env
  2. Start the server
  3. Create an organization and a user
docker compose run server manage org create redash
docker compose run server manage users create_root [email protected] Redash --password ******
  1. Log into http://localhost:5001/redash/

This code was supposed to be temporary, and raises an exception if
REDASH_MULTI_ORG=true is set.

---

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2213, in __call__
    return self.wsgi_app(environ, start_response)
  File "/usr/local/lib/python3.8/site-packages/werkzeug/middleware/proxy_fix.py", line 182, in __call__
    return self.app(environ, start_response)
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2193, in wsgi_app
    response = self.handle_exception(e)
  File "/usr/local/lib/python3.8/site-packages/flask_restful/__init__.py", line 298, in error_router
    return original_handler(e)
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2190, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1486, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.8/site-packages/flask_restful/__init__.py", line 298, in error_router
    return original_handler(e)
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1482, in full_dispatch_request
    rv = self.preprocess_request()
  File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1974, in preprocess_request
    rv = self.ensure_sync(before_func)()
  File "/app/redash/security.py", line 43, in check_csrf
    dest = f"{view.__module__}.{view.__name__}"
AttributeError: 'NoneType' object has no attribute '__module__'
@justinclift
Copy link
Member

From the comment in that code, it looks like this was waiting upon PR 419 in the external wtforms/flask-wtf repo to be merged, but it never was, and no-one followed up on it.

k, lets merge this as there's no obvious better way for now. 😄

@justinclift justinclift merged commit e2a39de into getredash:master Apr 23, 2024
11 checks passed
@eradman eradman deleted the check-csrf branch April 23, 2024 12:51
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Jan 8, 2025
This code was supposed to be temporary, and raises an exception if REDASH_MULTI_ORG=true is set.
@mjgp2
Copy link

mjgp2 commented Feb 6, 2025

@justinclift this seems to have broken SAML login for our organization when trying to upgrade to the latest version though unfortunately

@eradman
Copy link
Collaborator Author

eradman commented Feb 6, 2025

@mjgp2 were you able to confirm that SAML works if you revert this change?

We should find an easy method of configuring a SAML provider so that we can test for this kind of regression.

@mjgp2
Copy link

mjgp2 commented Feb 10, 2025

@mjgp2 were you able to confirm that SAML works if you revert this change?

Yes, confirmed. We're having to run a manually patched version - 25.1.0 tag + revert this

eradman added a commit to eradman/redash that referenced this pull request Feb 14, 2025
This workaround was missing 'if view is not None ' as found in
https://github.com/pallets-eco/flask-wtf/pull/419/files

Tested with MULTI_ORG enabled.
@eradman
Copy link
Collaborator Author

eradman commented Feb 14, 2025

I proposed a change that reverts this change and fixes the exception that motivated this to begin with in
#7327

eradman added a commit to eradman/redash that referenced this pull request Feb 18, 2025
This workaround was missing 'if view is not None ' as found in
https://github.com/pallets-eco/flask-wtf/pull/419/files

Tested with MULTI_ORG enabled.
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.

3 participants