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(embed): fix server error due to breaking change on flask-login #22462

Merged
merged 4 commits into from
Jan 12, 2023

Conversation

Usiel
Copy link
Contributor

@Usiel Usiel commented Dec 20, 2022

SUMMARY

Due to a breaking change in Flask-Login (maxcountryman/flask-login#378) the code for logging in an AnonymousUser breaks for dashboard embedding.

Unfortunately, Flask-Login not only renames the method we need, but also makes it quasi-private. We can switch to a different public util function Flask-Login offers since at least version 0.3.0. In all versions I checked it essentially executes the same steps as reload_user(...) did (it additionally signals the login event internally, which shouldn't cause issues).

Fixes #21987, #21146

TESTING INSTRUCTIONS

For any Flask-Login>=0.3.0,<0.7.0:

  1. Enable EMBEDDED_SUPERSET in superset/config.py
  2. Run Superset locally
  3. Go to any dashboard, get an embed UUID (see feat: Embedded dashboard configuration #19364 for detailed instructions)
  4. Execute the following
curl "http://localhost:8088/embedded/<embed_uuid>"

Expected: 200 OK with some valid HTML
(Before we would fail with a 500 for any Flask-Login>=0.5.0)

ADDITIONAL INFORMATION

cc @suddjian: Thanks for building this! Tagging you for better visibility and in case I missed anything. Btw, I noticed that g.user and the user on the ctx is actually set to an AnonymousUser instance before doing anything in the endpoint; I believe that's some Flask-Login fallback logic. I'm assuming there is some circumstance, some case, where this might not happen and that is why we added the bit of code previously? (That's why I propose to replace it instead of removing it in this PR.)

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #22462 (c1fe46e) into master (73e53fa) will decrease coverage by 1.50%.
The diff coverage is 70.75%.

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

@@            Coverage Diff             @@
##           master   #22462      +/-   ##
==========================================
- Coverage   67.10%   65.59%   -1.51%     
==========================================
  Files        1869     1869              
  Lines       71580    71589       +9     
  Branches     7806     7822      +16     
==========================================
- Hits        48031    46959    -1072     
- Misses      21521    22601    +1080     
- Partials     2028     2029       +1     
Flag Coverage Δ
hive ?
mysql 78.04% <68.08%> (+0.02%) ⬆️
postgres 78.11% <68.08%> (+0.02%) ⬆️
presto ?
python 78.22% <68.08%> (-3.10%) ⬇️
sqlite 76.48% <48.93%> (+<0.01%) ⬆️
unit ?

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

Impacted Files Coverage Δ
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <ø> (ø)
...rt-controls/src/shared-controls/sharedControls.tsx 55.17% <ø> (ø)
...ui-core/src/chart/components/FallbackComponent.tsx 100.00% <ø> (ø)
...et-ui-core/src/chart/components/SuperChartCore.tsx 100.00% <ø> (ø)
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
...s/legacy-plugin-chart-calendar/src/controlPanel.ts 50.00% <ø> (ø)
...s/legacy-plugin-chart-heatmap/src/controlPanel.tsx 57.14% <ø> (ø)
...ns/legacy-plugin-chart-horizon/src/controlPanel.ts 100.00% <ø> (ø)
...ns/legacy-plugin-chart-map-box/src/controlPanel.ts 30.00% <0.00%> (ø)
...legacy-plugin-chart-partition/src/controlPanel.tsx 25.00% <ø> (ø)
... and 187 more

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

@adimyth
Copy link
Contributor

adimyth commented Jan 10, 2023

Has this been merged & released?

@pythys
Copy link

pythys commented Jan 10, 2023

I applied the patch to my system and built the docker images, tried to access the dashboards and got instead a new error as attached. I'm not sure if I made a mistake though.
2023-01-10_14-52

@pythys
Copy link

pythys commented Jan 10, 2023

ok taking back my comment above, this is not related to this PR, but rather to other security settings. I confirm that the patch is working and fixing my issue on latest code base

@Usiel
Copy link
Contributor Author

Usiel commented Jan 11, 2023

ok taking back my comment above, this is not related to this PR, but rather to other security settings. I confirm that the patch is working and fixing my issue on latest code base

Thanks for verifying the fix @pythys!

Has this been merged & released?

Not yet @adimyth. Let me bump this PR on the community Slack.

Due to a breaking change in Flask-Login (maxcountryman/flask-login#378) the code for logging in our the AnonymousUser breaks. Unfortunately, Flask-Login not only renames the method we need, but also makes it quasi-private. We can switch to a different public util function Flask-Login offers since at least version 0.3.0. In all versions I checked it essentially executes the same steps as `reload_user(...)` did (it additionally signals the login event internally, which shouldn't cause issues).

Fixes apache#21987
@adimyth
Copy link
Contributor

adimyth commented Jan 11, 2023

Thanks very much, @Usiel Let me know when you bump this PR in community slack, I'll follow the thread & provide support if needed

@villebro
Copy link
Member

villebro commented Jan 11, 2023

This appears to be a regression from #22355 . Ping @EugeneTorap , can you check this?

Edit: this seems to predate the bump PR, but it would be good to ensure it's valid for the new version

@EugeneTorap
Copy link
Contributor

EugeneTorap commented Jan 11, 2023

Yes, it's regression bug from my PR because we don't have unit tests for the logic.
In order to prevent the regression bug in the future we need to cover everything with unit tests.

@EugeneTorap
Copy link
Contributor

@Usiel Can you add the unit tests for it?

@villebro
Copy link
Member

Yes, it's regression bug from my PR because we don't have unit tests for the logic.
In order to prevent the regression bug in the future we need to cover everything with unit tests.

@EugeneTorap looking more closely it seems it's the missing pinning that was the problem. So we should have restricted to <0.5 before, and now we should probably have <0.7 to avoid similar breakage going forward.

@EugeneTorap
Copy link
Contributor

@villebro We use only ´"flask-login==0.6.0"´ in setup.py because 0.6.2 has breaking changes for superset.

@Usiel Usiel force-pushed the usiel/fixes-embed-server-error branch from f44a840 to be46675 Compare January 11, 2023 11:57
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 11, 2023
@Usiel
Copy link
Contributor Author

Usiel commented Jan 11, 2023

@Usiel Can you add the unit tests for it?

Sure, I added an integration test which fails with the previous implementation and Flask-Login>0.4.

@EugeneTorap
Copy link
Contributor

@Usiel Thank you!
@dpgaspar Can you review the PR?

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.

Thanks so much for this fix + the added tests, a much needed improvement to ensure embedded is properly maintained going forward 👍 One minor nit, but I'm happy merging as-is.

tests/integration_tests/embedded/view_tests.py Outdated Show resolved Hide resolved
@adimyth
Copy link
Contributor

adimyth commented Jan 11, 2023

Looking forward to this PR being merged & being shipped in the next release!

@cwegener
Copy link
Contributor

we should have restricted to <0.5 before, and now we should probably have <0.7 to avoid similar breakage going forward.

If assuming SemVer 2.0, this is not true, as releases in Major version 0.y.z SHOULD NOT have their Public API considered stable.

@Usiel Usiel force-pushed the usiel/fixes-embed-server-error branch from 0717fb7 to c0b3679 Compare January 12, 2023 08:19
@villebro
Copy link
Member

we should have restricted to <0.5 before, and now we should probably have <0.7 to avoid similar breakage going forward.

If assuming SemVer 2.0, this is not true, as releases in Major version 0.y.z SHOULD NOT have their Public API considered stable.

Strictly speaking this is true, but in my experience most patch releases in the 0 major version tend to be API stable.

@villebro villebro merged commit d5ecfbb into apache:master Jan 12, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 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 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed dashboard on v2.0 generates the following error and throws a 500 internal server error
7 participants