orbisai0security commented on PR #40026: URL: https://github.com/apache/superset/pull/40026#issuecomment-4423026962
✅ **Changes Applied** I've updated the code based on your feedback: The fix addresses two code review concerns: 1. **Legacy dashboard ID bypass**: The original code only checked `can_access_dashboard` when `embedded` was non-None (i.e., the ID matched an EmbeddedDashboard UUID). For legacy integer dashboard IDs, `embedded` would be `None` and the access check was silently skipped, allowing users to mint guest tokens for dashboards they can't access. The fix now resolves both UUID-based and integer-based IDs, and always enforces the access check — raising `ForbiddenError` even when no matching dashboard is found. 2. **N+1 query**: The original loop issued one DB query per dashboard resource. The fix collects all dashboard IDs upfront, separates UUID-style IDs from legacy integer IDs, and resolves them in at most two batched queries (one for `EmbeddedDashboard` by UUID, one for `Dashboard` by integer ID), then iterates the resolved map to check access. **Files modified:** - `superset/security/api.py` The changes have been pushed to this PR branch. Please review! -- 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]
