codeant-ai-for-open-source[bot] commented on code in PR #43595: URL: https://github.com/apache/superset/pull/43595#discussion_r3873465596
########## docs/admin_docs/security/security.mdx: ########## @@ -243,6 +243,36 @@ Each subject in the response includes flat scalar ids (`user_id`, `role_id`, `gr than a nested object, so callers can match directly on whichever id they already have — only the id field matching the subject's `type` is populated; the others are `null`. +#### Extending Editorship with EXTRA_EDITORS_RESOLVER + +Deployments that grant edit access to a dashboard or chart through a mechanism outside +Superset's own Subject-based `editors` list — for example, a folder-permission system or an +internal directory service — can plug that logic in with `EXTRA_EDITORS_RESOLVER`: + +```python +def extra_editors_resolver(resource): + # `resource` is the Dashboard or Slice instance being checked. + # Return Subject instances, raw subject ids, or dicts with an `id` key. + return [...] + + +EXTRA_EDITORS_RESOLVER = extra_editors_resolver +``` + +The resolver's result is unioned with the resource's own `editors` for editorship checks: it +feeds `is_editor`, `raise_for_editorship`, save-as, and soft-delete restore. When +`EXTRA_EDITORS_RESOLVER` is configured, the usual lockout-prevention behavior — automatically +re-adding a non-admin who removes themselves from a resource's editors — is skipped, since the +deployment has its own way of keeping the resource editable. + +Resolved subject ids are also surfaced as `extra_editors` in the chart and dashboard `GET` +responses, so API clients can distinguish resolver-granted editorship from the resource's own +`editors` list. Review Comment: **Suggestion:** The `extra_editors` field is injected into responses after serialization but is not declared by `ChartGetResponseSchema` or `DashboardGetResponseSchema`, which are used to generate the OpenAPI response schemas. Consequently, generated API clients cannot rely on or expose this field as part of the documented typed response contract. Add the field to the corresponding schemas or qualify this documentation as an implementation detail. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Generated clients lack typed `extra_editors` response fields. - ⚠️ OpenAPI documentation disagrees with runtime chart responses. - ⚠️ OpenAPI documentation disagrees with runtime dashboard responses. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f53def8e864c4e2993893e8bde18ca09&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f53def8e864c4e2993893e8bde18ca09&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/admin_docs/security/security.mdx **Line:** 268:270 **Comment:** *Api Mismatch: The `extra_editors` field is injected into responses after serialization but is not declared by `ChartGetResponseSchema` or `DashboardGetResponseSchema`, which are used to generate the OpenAPI response schemas. Consequently, generated API clients cannot rely on or expose this field as part of the documented typed response contract. Add the field to the corresponding schemas or qualify this documentation as an implementation detail. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43595&comment_hash=14ef2b8232c413454ebc77c4c7e9a0a62641e1094ecfdf47d99669381eb037cc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43595&comment_hash=14ef2b8232c413454ebc77c4c7e9a0a62641e1094ecfdf47d99669381eb037cc&reaction=dislike'>👎</a> ########## docs/admin_docs/security/security.mdx: ########## @@ -243,6 +243,36 @@ Each subject in the response includes flat scalar ids (`user_id`, `role_id`, `gr than a nested object, so callers can match directly on whichever id they already have — only the id field matching the subject's `type` is populated; the others are `null`. +#### Extending Editorship with EXTRA_EDITORS_RESOLVER + +Deployments that grant edit access to a dashboard or chart through a mechanism outside +Superset's own Subject-based `editors` list — for example, a folder-permission system or an +internal directory service — can plug that logic in with `EXTRA_EDITORS_RESOLVER`: + +```python +def extra_editors_resolver(resource): + # `resource` is the Dashboard or Slice instance being checked. + # Return Subject instances, raw subject ids, or dicts with an `id` key. + return [...] + + +EXTRA_EDITORS_RESOLVER = extra_editors_resolver +``` + +The resolver's result is unioned with the resource's own `editors` for editorship checks: it +feeds `is_editor`, `raise_for_editorship`, save-as, and soft-delete restore. When +`EXTRA_EDITORS_RESOLVER` is configured, the usual lockout-prevention behavior — automatically +re-adding a non-admin who removes themselves from a resource's editors — is skipped, since the +deployment has its own way of keeping the resource editable. Review Comment: **Suggestion:** Configuring `EXTRA_EDITORS_RESOLVER` alone disables the self-lockout safeguard, regardless of whether the resolver returns any subjects for the resource. A resolver that returns an empty result can therefore let the last persisted editor remove themselves and leave the resource uneditable. Document this operational requirement explicitly, or state that the resolver must reliably grant an alternate editor before relying on the skipped safeguard. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Resources can become uneditable after self-removal. - ⚠️ Resolver configuration alone bypasses lockout protection. - ⚠️ Affected dashboards and charts require administrative recovery. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=189bed8793684065b86a56b01a2cf9a8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=189bed8793684065b86a56b01a2cf9a8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/admin_docs/security/security.mdx **Line:** 264:266 **Comment:** *Security: Configuring `EXTRA_EDITORS_RESOLVER` alone disables the self-lockout safeguard, regardless of whether the resolver returns any subjects for the resource. A resolver that returns an empty result can therefore let the last persisted editor remove themselves and leave the resource uneditable. Document this operational requirement explicitly, or state that the resolver must reliably grant an alternate editor before relying on the skipped safeguard. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43595&comment_hash=8394695fa58d3de7244119d005a31a06409376e6207d067d3e4cdec637cc9212&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43595&comment_hash=8394695fa58d3de7244119d005a31a06409376e6207d067d3e4cdec637cc9212&reaction=dislike'>👎</a> ########## docs/admin_docs/security/security.mdx: ########## @@ -243,6 +243,36 @@ Each subject in the response includes flat scalar ids (`user_id`, `role_id`, `gr than a nested object, so callers can match directly on whichever id they already have — only the id field matching the subject's `type` is populated; the others are `null`. +#### Extending Editorship with EXTRA_EDITORS_RESOLVER + +Deployments that grant edit access to a dashboard or chart through a mechanism outside +Superset's own Subject-based `editors` list — for example, a folder-permission system or an +internal directory service — can plug that logic in with `EXTRA_EDITORS_RESOLVER`: + +```python +def extra_editors_resolver(resource): + # `resource` is the Dashboard or Slice instance being checked. + # Return Subject instances, raw subject ids, or dicts with an `id` key. + return [...] + + +EXTRA_EDITORS_RESOLVER = extra_editors_resolver +``` + +The resolver's result is unioned with the resource's own `editors` for editorship checks: it +feeds `is_editor`, `raise_for_editorship`, save-as, and soft-delete restore. When +`EXTRA_EDITORS_RESOLVER` is configured, the usual lockout-prevention behavior — automatically +re-adding a non-admin who removes themselves from a resource's editors — is skipped, since the +deployment has its own way of keeping the resource editable. + +Resolved subject ids are also surfaced as `extra_editors` in the chart and dashboard `GET` +responses, so API clients can distinguish resolver-granted editorship from the resource's own +`editors` list. + +Because the resolver is arbitrary per-deployment Python rather than a SQL-expressible condition, +editorship it grants cannot be reflected in list-view filtering (for example, the soft-deleted +archive is scoped to editors via a SQL query) — it only takes effect on direct, per-object checks. Review Comment: **Suggestion:** The resolver is also invoked for every resource in chart and dashboard list responses to populate `extra_editors`, so it does not apply only to direct per-object checks. An external directory or folder-permission lookup implemented from this example can therefore run once per returned row and make ordinary list requests slow or dependent on external-service availability. Document the list-response invocation and its per-row cost. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Chart list requests perform resolver calls per row. - ⚠️ Dashboard list requests perform resolver calls per row. - ⚠️ External resolver latency can slow ordinary API listings. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4e2f3abdc9ed49d7ac65666ff1842214&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4e2f3abdc9ed49d7ac65666ff1842214&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/admin_docs/security/security.mdx **Line:** 272:274 **Comment:** *Performance: The resolver is also invoked for every resource in chart and dashboard list responses to populate `extra_editors`, so it does not apply only to direct per-object checks. An external directory or folder-permission lookup implemented from this example can therefore run once per returned row and make ordinary list requests slow or dependent on external-service availability. Document the list-response invocation and its per-row cost. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43595&comment_hash=c5352939a113d912ce9b6cb7f5b4dd599819d0cefa78276d6b0a246589a01e61&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43595&comment_hash=c5352939a113d912ce9b6cb7f5b4dd599819d0cefa78276d6b0a246589a01e61&reaction=dislike'>👎</a> -- 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]
