Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: change 401 response to a 403 for Security Exceptions #17768

Merged
merged 3 commits into from
Dec 16, 2021

Conversation

rusackas
Copy link
Member

SUMMARY

This changes the response for security exceptions from 401 (Unauthorized) to 403 (Forbidden). This is a fix because it means when these exceptions are encountered, it will no longer redirect users to the login page, which is intended to only happen for proper 401 errors.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks! i wonder if you got the chance to check the other 401 instances in superset? this is probably the big one, but might be worth doing a check of the others

@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #17768 (525ebf4) into master (6d97e89) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17768      +/-   ##
==========================================
+ Coverage   67.71%   67.78%   +0.06%     
==========================================
  Files        1604     1604              
  Lines       64186    64186              
  Branches     6786     6786              
==========================================
+ Hits        43464    43508      +44     
+ Misses      18867    18823      -44     
  Partials     1855     1855              
Flag Coverage Δ
hive 81.79% <100.00%> (ø)
javascript 54.81% <ø> (ø)
mysql 82.17% <100.00%> (ø)
postgres 82.22% <100.00%> (ø)
presto 82.09% <100.00%> (?)
python 82.71% <100.00%> (+0.14%) ⬆️
sqlite 81.90% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/exceptions.py 91.42% <100.00%> (ø)
superset/models/core.py 90.00% <0.00%> (+0.73%) ⬆️
superset/connectors/sqla/models.py 88.42% <0.00%> (+1.34%) ⬆️
superset/db_engine_specs/presto.py 90.39% <0.00%> (+6.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d97e89...525ebf4. Read the comment docs.

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for this 🙏

@rusackas rusackas merged commit 3aa1161 into master Dec 16, 2021
@rusackas rusackas deleted the fix-change-401-to-403-for-Security-Exceptions branch December 16, 2021 00:24
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* fix: change 401 to 403 for Security Exceptions

* updating tests to reflect new (proper) status code

* another test update
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* fix: change 401 to 403 for Security Exceptions

* updating tests to reflect new (proper) status code

* another test update
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/XS 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants