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

fix: enable strong session protection by default #24256

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented May 31, 2023

SUMMARY

Enable strong session protection by default. This is a more secure sane default, more details on: https://flask-login.readthedocs.io/en/latest/#session-protection

When session protection is active, each request, it generates an identifier for the user’s computer (basically, a secure hash of the IP address and user agent). If the session does not have an associated identifier, the one generated will be stored. If it has an identifier, and it matches the one generated, then the request is OK.
If the identifiers do not match in basic mode, or when the session is permanent, then the session will simply be marked as non-fresh, and anything requiring a fresh login will force the user to re-authenticate. (Of course, you must be already using fresh logins where appropriate for this to have an effect.)
If the identifiers do not match in strong mode for a non-permanent session, then the entire session (as well as the remember token if it exists) is deleted.

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

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #24256 (bf82cbc) into master (d1c57e0) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head bf82cbc differs from pull request most recent head 93367ef. Consider uploading reports for the commit 93367ef to get more accurate results

@@            Coverage Diff             @@
##           master   #24256      +/-   ##
==========================================
- Coverage   68.31%   68.22%   -0.09%     
==========================================
  Files        1957     1957              
  Lines       75596    75597       +1     
  Branches     8222     8222              
==========================================
- Hits        51640    51576      -64     
- Misses      21848    21913      +65     
  Partials     2108     2108              
Flag Coverage Δ
hive ?
mysql 78.92% <100.00%> (+<0.01%) ⬆️
postgres 78.99% <100.00%> (+<0.01%) ⬆️
presto 53.29% <100.00%> (+<0.01%) ⬆️
python 82.64% <100.00%> (-0.18%) ⬇️
sqlite 77.53% <100.00%> (+<0.01%) ⬆️
unit 53.42% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/components/Table/index.tsx 75.29% <ø> (ø)
superset-frontend/src/dashboard/types.ts 100.00% <ø> (ø)
...set-frontend/src/dashboard/util/permissionUtils.ts 86.66% <ø> (ø)
...perset-frontend/src/hooks/apiResources/queryApi.ts 100.00% <ø> (ø)
superset/config.py 92.02% <100.00%> (+0.02%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -1387,6 +1387,8 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
SESSION_COOKIE_HTTPONLY = True # Prevent cookie from being read by frontend JS?
SESSION_COOKIE_SECURE = False # Prevent cookie from being transmitted over non-tls?
SESSION_COOKIE_SAMESITE: Optional[Literal["None", "Lax", "Strict"]] = "Lax"
# Accepts None, "basic" and "strong", more details on: https://flask-login.readthedocs.io/en/latest/#session-protection
SESSION_PROTECTION = "strong"
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth adding something to UPDATING.md?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, more is better then less

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@dpgaspar dpgaspar merged commit f898c97 into apache:master Jun 1, 2023
31 checks passed
@dpgaspar dpgaspar deleted the fix/session-protection branch June 1, 2023 13:01
@virgofx
Copy link

virgofx commented Jun 11, 2023

Just an FYI - With session protection strong, this breaks selenium drivers connecting to app (which means things like cache warmup and thumbnails don't work). I had to revert to none which worked but didn't test basic. There may be some additional work here to update the machine auth function to properly work when this is enabled

@mapledan
Copy link
Contributor

mapledan commented Jun 19, 2023

I am facing the same issue where the Alert&Report are not working properly.
To resolve this issue, I need to configure the SESSION_PROTECTION to None.

Just an FYI - With session protection strong, this breaks selenium drivers connecting to app (which means things like cache warmup and thumbnails don't work). I had to revert to none which worked but didn't test basic. There may be some additional work here to update the machine auth function to properly work when this is enabled

@sfirke
Copy link
Member

sfirke commented Jun 26, 2023

Alerts & Reports works for me with SESSION_PROTECTION = "basic".

"strong" prevented me from logging in entirely, so I couldn't test Alerts & Reports with that value.

@dpgaspar
Copy link
Member Author

ok, thank you for the reports, I'll work on a fix for this

michael-s-molina added a commit to michael-s-molina/incubator-superset that referenced this pull request Jun 28, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants