chore: remove CssTemplate and Annotation access from gamma role

[lilykuang]

SUMMARY

• gamma user should not have access to csstemplatemodelview/list/ and annotationlayer/list/ .

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

[john-bodley]

@lilykuang what's the difference (from a Gamma role perspective) between providing access to these lists and say charts or dashboards.

[codecov]

Codecov Report

Merging #24826 (946d9b1) into master (85a7d5c) will decrease coverage by 0.01%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##           master   #24826      +/-   ##
==========================================
- Coverage   69.00%   68.99%   -0.01%     
==========================================
  Files        1906     1906              
  Lines       74134    74134              
  Branches     8208     8208              
==========================================
- Hits        51153    51152       -1     
- Misses      20858    20859       +1     
  Partials     2123     2123              

Flag	Coverage Δ
hive	54.17% <ø> (ø)
mysql	79.21% <ø> (ø)
postgres	79.30% <ø> (-0.02%)	⬇️
presto	54.07% <ø> (ø)
python	83.36% <ø> (-0.01%)	⬇️
sqlite	77.89% <ø> (ø)
unit	55.05% <ø> (ø)

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

Files Changed	Coverage Δ
superset/security/manager.py	94.43% <ø> (ø)

... and 1 file with indirect coverage changes

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

[eschutho]

@lilykuang what's the difference (from a Gamma role perspective) between providing access to these lists and say charts or dashboards.

@john-bodley this is one of those odd cases where the user doesn't have list/menu view access, but has access to the underlying data, which they shouldn't have. So it's just aligning the existing view perms with the underlying data.

[eschutho]

yey for alphabetization! 🙏

[eschutho]

LGTM!

[villebro]

Sorry for being late to the party @lilykuang @eschutho , but would it be a good idea to include this change in UPDATING.md, as it may break current workflows for some existing users/deployments?

[john-bodley]

@michael-s-molina if, per @villebro's comment, this is deemed breaking change, wouldn't this need to wait until Superset 4.0?

[villebro]

@lilykuang @eschutho @john-bodley @michael-s-molina this may well be murky, and I'm not personally super opinionated one way or another, as I don't think these are commonly used perms. but in the past I think we've erred on the side of caution to be as semver compliant as possible.

[eschutho]

I agree that if we think it's a breaking change, then we should wait until 4.0, but my understanding was that the user cannot access these menus from the client, so I don't believe that this change should impact any current workflows. Not sure @lilykuang if you have any other context. Where it does break functionality, is for api usage on these endpoints for /csstemplatemodelview/ and /annotationlayer/ which similar to what @villebro mentioned about the perms, these endpoints aren't widely used. We could leave it in master, but take it out of 2.1.2 or take it out of master and put it in 4.0, or just leave as is. @dpgaspar may have an opinion on this as well.

[dpgaspar]

This is actually a bug. A Gamma user should not have access to these views and users don't have access to it via the UI. Really don't view it as a breaking change, but a note on UPDATING would have been nice. This was included on 3.0

[michael-s-molina]

I agree with @dpgaspar here. I don't think this is a breaking change as we shouldn't have granted the access in the first place. It may impact current workflows as @villebro mentioned but that's exactly what I expect this change to do given that these users are accessing resources they shouldn't have access to.

[villebro]

Thanks for the comments @dpgaspar and @michael-s-molina - in other words it sounds like this can be thought of as a security fix of sorts. For future reference, let's try to add a note in UPDATING.md when doing these types of fixes that cause expected breaking changes.