codeant-ai-for-open-source[bot] commented on code in PR #37773: URL: https://github.com/apache/superset/pull/37773#discussion_r3580165671
########## superset/config.py: ########## @@ -1809,6 +1809,11 @@ def allowed_schemas_for_csv_upload( # pylint: disable=unused-argument FAB_API_KEY_ENABLED = False FAB_API_KEY_PREFIXES = ["sst_"] +# When False (default), legacy FAB SSR password reset routes +# (/superset/resetpassword, /superset/resetmypassword) are not registered. +# Set to True to re-enable direct URL access to those views (e.g. during migration). +ENABLE_LEGACY_FAB_PASSWORD_VIEWS = False Review Comment: **Suggestion:** Add an explicit boolean type annotation to this new config variable assignment to satisfy the type-hint requirement for annotatable variables. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This is a newly added Python config variable that could be annotated, but it is assigned without a type hint. That matches the type-hint requirement rule for annotatable variables. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=413edf3fef2a4b6695d3f6f9c1189c41&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=413edf3fef2a4b6695d3f6f9c1189c41&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/config.py **Line:** 1815:1815 **Comment:** *Custom Rule: Add an explicit boolean type annotation to this new config variable assignment to satisfy the type-hint requirement for annotatable variables. 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%2F37773&comment_hash=6ef12265675b4734e26ddcc8a433a6cabe1a94679a066567cb8727017c07aabc&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=6ef12265675b4734e26ddcc8a433a6cabe1a94679a066567cb8727017c07aabc&reaction=dislike'>π</a> ########## superset/security/manager.py: ########## @@ -4358,16 +4386,28 @@ def register_views(self) -> None: original_auth_rate_limited = current_app.config["AUTH_RATE_LIMITED"] current_app.config["AUTH_RATE_LIMITED"] = False + original_add_view_no_menu = self._skip_legacy_fab_password_view_registration() + try: super().register_views() finally: - # Restore original value even if an exception occurs + # Restore original values even if an exception occurs current_app.config["AUTH_RATE_LIMITED"] = original_auth_rate_limited + self.appbuilder.add_view_no_menu = ( # type: ignore[method-assign] + original_add_view_no_menu + ) + # temporal change to remove the roles view from the security menu, after + # migrating all views to frontend, we will set FAB_ADD_SECURITY_VIEWS = False for view in list(self.appbuilder.baseviews): - if isinstance(view, self.rolemodelview.__class__) and getattr( - view, "route_base", None - ) in ["/roles", "/users", "/groups", "registrations"]: + route_base = getattr(view, "route_base", None) Review Comment: **Suggestion:** Add a concrete type annotation to this newly added variable (for example an optional string) since its type is known and annotatable. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> The variable is newly introduced and its type is inferable from the code path, so it can be annotated. Since it is left unannotated, this is a real instance of the type-hint rule violation. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b7fbfeec50449758c2eb84c8ace7424&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=4b7fbfeec50449758c2eb84c8ace7424&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/security/manager.py **Line:** 4403:4403 **Comment:** *Custom Rule: Add a concrete type annotation to this newly added variable (for example an optional string) since its type is known and annotatable. 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%2F37773&comment_hash=946b1c877d9d79ca3e810f3a2def33a63039ef206ce121f4ae40a56fcf00cb22&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=946b1c877d9d79ca3e810f3a2def33a63039ef206ce121f4ae40a56fcf00cb22&reaction=dislike'>π</a> ########## tests/unit_tests/security/manager_test.py: ########## @@ -2095,6 +2095,68 @@ def test_user_view_menu_names_for_guest_user_no_roles( mock_get_user_id.assert_not_called() [email protected]( + "enable_legacy_password_views,enable_force_password_change,expected_registered", + [ + (False, False, {"NonPasswordView"}), + (False, True, {"NonPasswordView", "ResetMyPasswordView"}), + ( + True, + False, + {"NonPasswordView", "ResetMyPasswordView", "ResetPasswordView"}, + ), + ], +) +def test_skip_legacy_fab_password_view_registration_keeps_forced_change_target( + app_context: None, + enable_legacy_password_views: bool, + enable_force_password_change: bool, + expected_registered: set[str], +) -> None: + """Forced password changes require the self-service reset view.""" + from flask import current_app + from flask_appbuilder.security.views import ResetMyPasswordView, ResetPasswordView + + class NonPasswordView: + pass + + registered: list[str] = [] + + def add_view_no_menu(baseview: type[Any], *args: Any, **kwargs: Any) -> type[Any]: + registered.append(baseview.__name__) + return baseview + + fake_appbuilder = SimpleNamespace(add_view_no_menu=add_view_no_menu) + sm = SupersetSecurityManager.__new__(SupersetSecurityManager) + sm.appbuilder = fake_appbuilder + + previous_config = { + "ENABLE_LEGACY_FAB_PASSWORD_VIEWS": current_app.config[ + "ENABLE_LEGACY_FAB_PASSWORD_VIEWS" + ], + "ENABLE_FORCE_PASSWORD_CHANGE": current_app.config[ + "ENABLE_FORCE_PASSWORD_CHANGE" + ], + } + current_app.config["ENABLE_LEGACY_FAB_PASSWORD_VIEWS"] = ( + enable_legacy_password_views + ) + current_app.config["ENABLE_FORCE_PASSWORD_CHANGE"] = enable_force_password_change + + original_add_view_no_menu = fake_appbuilder.add_view_no_menu Review Comment: **Suggestion:** Add an explicit type annotation for this local callable reference so the new test code keeps full type-hint coverage for relevant variables. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> The new local variable `original_add_view_no_menu` is assigned without an explicit type annotation, and it is a relevant variable in new Python code that can be annotated. This matches the type-hint requirement violation. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b311bcf8d434adf89a929c259623ca1&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=0b311bcf8d434adf89a929c259623ca1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/security/manager_test.py **Line:** 2146:2146 **Comment:** *Custom Rule: Add an explicit type annotation for this local callable reference so the new test code keeps full type-hint coverage for relevant variables. 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%2F37773&comment_hash=01b58875a5bd957426ac95befa50efd7d68f07bfb176b4c2a081508ba1883e05&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=01b58875a5bd957426ac95befa50efd7d68f07bfb176b4c2a081508ba1883e05&reaction=dislike'>π</a> ########## superset/security/manager.py: ########## @@ -4358,16 +4386,28 @@ def register_views(self) -> None: original_auth_rate_limited = current_app.config["AUTH_RATE_LIMITED"] current_app.config["AUTH_RATE_LIMITED"] = False + original_add_view_no_menu = self._skip_legacy_fab_password_view_registration() Review Comment: **Suggestion:** Annotate this newly added callable restoration variable with an explicit callable type to comply with the type-hint requirement. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This assignment introduces another callable-typed local variable that is amenable to annotation, but none is present. That is consistent with the project rule requiring type hints for annotatable variables. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b0381eaf7484241a890542e0fc2bc49&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=0b0381eaf7484241a890542e0fc2bc49&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/security/manager.py **Line:** 4389:4389 **Comment:** *Custom Rule: Annotate this newly added callable restoration variable with an explicit callable type to comply with the type-hint requirement. 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%2F37773&comment_hash=85c89a3e45eea7190532f9d020b92f65bad72b2701117b24eef4bd717edb5e51&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=85c89a3e45eea7190532f9d020b92f65bad72b2701117b24eef4bd717edb5e51&reaction=dislike'>π</a> ########## superset/security/manager.py: ########## @@ -4327,8 +4327,36 @@ def is_admin(self) -> bool: role.name for role in self.get_user_roles() ] - # temporal change to remove the roles view from the security menu, - # after migrating all views to frontend, we will set FAB_ADD_SECURITY_VIEWS = False + def _skip_legacy_fab_password_view_registration(self) -> Callable[..., Any]: + original_add_view_no_menu = self.appbuilder.add_view_no_menu Review Comment: **Suggestion:** Add an explicit type annotation for this newly introduced callable variable to satisfy the projectβs type-hint requirement for annotatable variables. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This new local variable stores a callable and is fully annotatable, but no type hint is provided. That matches the repository rule requiring type hints on new or modified Python code where annotations are applicable. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=817f12c523eb47ab9c2beb166056d834&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=817f12c523eb47ab9c2beb166056d834&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/security/manager.py **Line:** 4331:4331 **Comment:** *Custom Rule: Add an explicit type annotation for this newly introduced callable variable to satisfy the projectβs type-hint requirement for annotatable variables. 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%2F37773&comment_hash=542040e82cbb8afcacc17556c2318a71abc7a1b66d8447854a9443a6350ed984&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=542040e82cbb8afcacc17556c2318a71abc7a1b66d8447854a9443a6350ed984&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]
