fix(reports): skip inactive owners in get_executor (#33584) - #42611
Closed
Abdulrehman-PIAIC80387 wants to merge 1 commit into
Closed
Conversation
``get_executor`` in ``superset/tasks/utils.py`` walked the executor priority list (CREATOR → MODIFIER → EDITOR → …) and returned the first matching owner without checking ``user.active``. When the picked user was later deactivated, their username was handed to ``login_user()`` in ``MachineAuthProviderUser.get_auth_cookies``, which silently returns ``False`` for inactive users. No session cookie was set; the downstream screenshot / CSV / dashboard-render request failed without auth and surfaced as an opaque ``ReportScheduleCsvFailedError``. Fix adds a small ``_is_active(user)`` predicate and applies it at every direct-user return site in ``get_executor`` (CREATOR, MODIFIER, both ``_EDITOR`` variants, and the EDITOR modifier/creator/editor-users fallback). The priority chain now falls through to the next active candidate, or raises the existing ``ExecutorNotFoundError`` when none exist — which is far more actionable than the silent auth failure. Left intentionally unchanged: * The indirect (role/group) path in ``_get_indirect_editor_user`` already filters on ``User.active.is_(True)``. * ``FixedExecutor`` / ``CURRENT_USER`` paths are not ``ab_user``-backed at this layer. * ``login_user(force=True)`` was considered and rejected: it would let deactivated users execute schedules on their behalf, which is a regression, not a fix. Tests: extended the existing parametrized ``test_get_executor`` with four new cases (inactive-creator-falls-through, all-inactive-raises, EDITOR-skips-inactive-editor-user, CREATOR_EDITOR-inactive-editor- falls-through). The ``_get_users`` / ``_make_user_subject`` helpers now carry an explicit ``active`` flag defaulted to ``True`` so all existing cases keep their historical "user is usable" intent.
dosubot
Bot
added
the
alert-reports
Contributor
Code Review Agent Run #5ad4a1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Abdulrehman-PIAIC80387
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42611 +/- ##
==========================================
- Coverage 65.33% 65.33% -0.01%
==========================================
Files 2803 2803
Lines 158490 158493 +3
Branches 36178 36178
==========================================
Hits 103557 103557
- Misses 52922 52924 +2
- Partials 2011 2012 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Fixes #33584.
get_executorinsuperset/tasks/utils.pywalks the configured executor priority list (CREATOR → MODIFIER → EDITOR → …) and returns the first matching owner/creator/modifier without checking whether that user is active. When the picked user was later deactivated, their username lands inlogin_user()inMachineAuthProviderUser.get_auth_cookies, which silently returnsFalsefor inactive users. No session cookie is set; the downstream screenshot / CSV / dashboard-render request runs with no auth and surfaces as an opaqueReportScheduleCsvFailedError.Per @rusackas on the issue thread, the fix is to have
get_executorskip inactive owners so the priority chain falls through to an active one (or raises the existingExecutorNotFoundErrorif none exist).FIX
Adds a small
_is_active(user)predicate and applies it at every direct-user return site inget_executor:CREATOR,MODIFIER, both_EDITORvariantsEDITOR's modifier/creator/editor-users fallback chainIntentionally left unchanged:
_get_indirect_editor_user— already filters onUser.active.is_(True)FixedExecutor/CURRENT_USER— notab_user-backed at this layerlogin_user(force=True)— considered and rejected: bypassing the FAB active-user check would let deactivated users execute schedules on their behalf, which is a regression, not a fixBEHAVIOR MATRIX
ReportScheduleCsvFailedErrorReportScheduleCsvFailedErrorExecutorNotFoundError(actionable)is_active✓is_active✓ (unchanged)TESTING INSTRUCTIONS
Manual (requires a working Alerts & Reports setup):
UPDATE ab_user SET active=false WHERE username='A')ReportScheduleCsvFailedErrorin logs, no emailAutomated:
Four new parametrized cases: inactive-creator-falls-through, all-inactive-raises, EDITOR-skips-inactive-editor-user, CREATOR_EDITOR-inactive-editor-falls-through. Existing cases stay green because the
_get_users/_make_user_subjecthelpers now carry an explicitactiveflag defaulted toTrue.ADDITIONAL INFORMATION