Ujjwaljain16 commented on PR #44142: URL: https://github.com/apache/superset/pull/44142#issuecomment-5679974441
@rebenitez1802 really appreciate the deep dive here both of your Medium findings checked out, and pulling that thread turned out to matter a lot more than i expected `can_sqllab_table_viz`/`can_get_or_create_table` was exactly as you described `can_get_or_create_table` was never a real permission turns out `UPDATING.md` itself has the wrong name, which is probably where it got picked up from. Moved it into the rename migration under the right key `can_test_conn` needed more than the spelling fix, though once i went and looked at the actual PR that removed `testconn()` (#24354), it had a real successor all along `Database.can_write` so instead of just renaming the string i moved it out of the delete list entirely the bigger surprise was yr "incomplete cleanup" note i went and actually checked PR #24400's diff for the six permissions you called out as a minor doc nit, and every single one of them has a real `@deprecated(new_target=...)` pointing at a live endpoint that's not a doc problem, that's the same silent-drop bug you found just hiding in six more places so i stopped trusting the original "no successor" labels entirely and re-checked all 25 the same way actual PR diffs, actual deprecation decorators, tracing each one to a real live endpoint 19 of the 25 had a real successor the delete list is down to 6 now the rename list is up to 34 also added `can_annotation_json` and `can_fave_dashboards_by_username`, which were genuinely missing from both lists didn't add `can_theme` though checked its whole history and it never had any access decorator on it, so it was never a real permission to begin with, would've just been a third phantom entry. fixed the `SECURITY.md` line, and added tests for the disjointness check, for roles keeping their other permissions, and for running both migrations back to back one more thing came up on a final pass i did over the shakier mappings before merging: `can_my_queries` needed to go to *both* `SavedQuery.can_list` and `SavedQuery.can_read`, not just `can_list` that permission alone only unlocks the page itself, the actual query list comes from a separately-gated API call would've left people staring at a blank page otherwise two mappings (`can_my_queries` and `can_import_dashboards`) don't have a formal deprecation decorator backing them like the rest do have called that out directly in the migration's docstring rather than papering over it tests went from 154 to 157 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
