codeant-ai-for-open-source[bot] commented on code in PR #37773:
URL: https://github.com/apache/superset/pull/37773#discussion_r3580571556
##########
superset/config.py:
##########
@@ -1928,6 +1928,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.
Review Comment:
**Suggestion:** The new comment states that both legacy password-reset
routes are not registered when this flag is false, but `ResetMyPasswordView` is
still intentionally registered when forced password change is enabled. This
documentation is incorrect and can mislead operators into thinking
forced-change users will not hit legacy reset pages. Update the comment to
reflect the conditional exception for forced-password-change mode. [comment
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Operators enabling forced-change misled about ResetMyPasswordView
registration.
- ⚠️ Legacy SSR reset page exposed contrary to configuration comment.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/config.py` around lines 1931-1935 and observe the comment
stating that
when `ENABLE_LEGACY_FAB_PASSWORD_VIEWS` is False, both
`/superset/resetpassword` and
`/superset/resetmypassword` routes "are not registered", followed by
`ENABLE_LEGACY_FAB_PASSWORD_VIEWS: bool = False` at line 1935 (per hunk:
1931-1935).
2. Inspect the registration shim in `superset/security/manager.py` at
`_skip_legacy_fab_password_view_registration` (around lines 4802-4826 in the
file, shown
in the snippet starting at 4790): it returns early if
`current_app.config["ENABLE_LEGACY_FAB_PASSWORD_VIEWS"]` is True, otherwise
it builds
`legacy_password_views` as `(ResetPasswordView,)` and only appends
`ResetMyPasswordView`
when `ENABLE_FORCE_PASSWORD_CHANGE` is False (`if not
current_app.config.get("ENABLE_FORCE_PASSWORD_CHANGE", False):
legacy_password_views =
(*legacy_password_views, ResetMyPasswordView)`).
3. From this logic, derive that with `ENABLE_LEGACY_FAB_PASSWORD_VIEWS =
False` and
`ENABLE_FORCE_PASSWORD_CHANGE = True`, only `ResetPasswordView` is filtered
out;
`ResetMyPasswordView` is not in `legacy_password_views` and therefore is
still registered
by `self.appbuilder.add_view_no_menu`, meaning the
`/superset/resetmypassword` legacy SSR
route remains available.
4. Confirm this behavior via the unit test
`test_skip_legacy_fab_password_view_registration_keeps_forced_change_target`
in
`superset/tests/unit_tests/security/manager_test.py` lines 2625-2663, which
parametrizes
`enable_legacy_password_views=False, enable_force_password_change=True` and
expects
`{"NonPasswordView", "ResetMyPasswordView"}` in `expected_registered`,
proving that
`ResetMyPasswordView` is still registered under that configuration and
contradicting the
config comment that both routes "are not registered" when the flag is False.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0c8016e913554dec964fcf3bab0a659f&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=0c8016e913554dec964fcf3bab0a659f&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:** 1931:1932
**Comment:**
*Comment Mismatch: The new comment states that both legacy
password-reset routes are not registered when this flag is false, but
`ResetMyPasswordView` is still intentionally registered when forced password
change is enabled. This documentation is incorrect and can mislead operators
into thinking forced-change users will not hit legacy reset pages. Update the
comment to reflect the conditional exception for forced-password-change mode.
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=40a975b3e46451fa8c46d6e9d62fc23707b361359b93216d76ebceab0ae6e16f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=40a975b3e46451fa8c46d6e9d62fc23707b361359b93216d76ebceab0ae6e16f&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -4830,16 +4858,30 @@ 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: Callable[..., Any] = (
+ 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: Optional[str] = getattr(view, "route_base", None)
+ # Remove FAB security menu views (roles, users, groups,
registrations)
+ if isinstance(view, self.rolemodelview.__class__) and route_base
in [
+ "/roles",
+ "/users",
+ "/groups",
Review Comment:
**Suggestion:** The registrations route entry is missing a leading `/`, so
it will not match normal route base values (for example `/registrations`), and
that view will be skipped by this filter unexpectedly. Use the correct
route-base string format to ensure consistent matching. [typo]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Suggestion targets unused route_base; no current effect.
- ⚠️ Change would only align string formatting convention.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the legacy FAB security view pruning loop in
`superset/security/manager.py:4876-4885`, where `route_base` is compared
against
`["/roles", "/users", "/groups", "registrations"]`.
2. Use repo-wide search for route bases in Superset views
(`superset/views/*.py`), e.g.
`superset/views/user_registrations.py:27-30` and
`superset/views/roles.py:27-31`, and
confirm no Superset-defined view uses `route_base = "registrations"`; SPA
views use
`route_base = "/"` with `@expose("/registrations/")`.
3. Since the filter also requires `isinstance(view,
self.rolemodelview.__class__)`, only
FAB’s RoleModelView-type views are affected; user registration and other
security views
use different classes and thus are not matched, regardless of the
`"registrations"` entry.
4. In the current codebase, the `"registrations"` string does not correspond
to any
concrete `route_base` on views we define, so the missing leading `/` does
not cause an
observable mis-filtering today; it is at most a cosmetic inconsistency in
the string list,
making the suggested bug about unmatched registrations route
non-reproducible against
present code.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2d32940da6064e7a88ae21efab19ce97&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=2d32940da6064e7a88ae21efab19ce97&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:** 4882:4882
**Comment:**
*Typo: The registrations route entry is missing a leading `/`, so it
will not match normal route base values (for example `/registrations`), and
that view will be skipped by this filter unexpectedly. Use the correct
route-base string format to ensure consistent matching.
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=1503d4353eae703285e9bb48fdbab00fcc54e7f422d56f3f8b75934ab8d55121&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=1503d4353eae703285e9bb48fdbab00fcc54e7f422d56f3f8b75934ab8d55121&reaction=dislike'>👎</a>
##########
tests/integration_tests/core_tests.py:
##########
@@ -156,6 +160,14 @@ def assert_admin_view_menus_in(role_name, assert_func):
assert_admin_view_menus_in("Alpha", self.assertNotIn)
assert_admin_view_menus_in("Gamma", self.assertNotIn)
+ def test_legacy_fab_password_views_are_not_registered(self):
+ from flask import current_app
+
+ endpoints = {rule.endpoint for rule in
current_app.url_map.iter_rules()}
+
+ assert "ResetPasswordView.this_form_get" not in endpoints
+ assert "ResetMyPasswordView.this_form_get" not in endpoints
Review Comment:
**Suggestion:** This test hard-codes that both password-reset endpoints are
always absent, but `ResetMyPasswordView` is intentionally kept registered when
`ENABLE_FORCE_PASSWORD_CHANGE` is enabled (and both views are registered when
`ENABLE_LEGACY_FAB_PASSWORD_VIEWS` is enabled). As written, the test will fail
under valid config combinations and encodes behavior that contradicts the
security manager logic. Make the assertions conditional on those flags (or
explicitly set the config in-test before asserting). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Forced-password-change configuration fails integration test suite.
- ⚠️ Password-reset routing tests contradict manager flag logic.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the default configuration flags in `superset/config.py`:
`ENABLE_LEGACY_FAB_PASSWORD_VIEWS: bool = False` at line 1934 and
`ENABLE_FORCE_PASSWORD_CHANGE = False` at line 406.
2. Inspect the legacy FAB password view registration logic in
`superset/security/manager.py:_skip_legacy_fab_password_view_registration`
at lines
4802–4827: when `ENABLE_LEGACY_FAB_PASSWORD_VIEWS` is False and
`ENABLE_FORCE_PASSWORD_CHANGE` is True, `legacy_password_views` is set to
`(ResetPasswordView,)` only (lines 4813–4816), so `ResetMyPasswordView` is
still
registered while `ResetPasswordView` is filtered.
3. Confirm this behavior is explicitly tested and relied on in
`tests/unit_tests/security/manager_test.py:test_skip_legacy_fab_password_view_registration_keeps_forced_change_target`
at lines 2633–2641, where the `(False, True, expected_registered)` case
includes
`"ResetMyPasswordView"` and omits `"ResetPasswordView"`.
4. Inspect
`tests/integration_tests/core_tests.py:test_legacy_fab_password_views_are_not_registered`
at lines 163–170: it builds `endpoints = {rule.endpoint for rule in
current_app.url_map.iter_rules()}` and unconditionally asserts both
`"ResetPasswordView.this_form_get" not in endpoints` and
`"ResetMyPasswordView.this_form_get" not in endpoints` (lines 168–169).
Under a valid
configuration where `ENABLE_LEGACY_FAB_PASSWORD_VIEWS` is False and
`ENABLE_FORCE_PASSWORD_CHANGE` is True, `ResetMyPasswordView.this_form_get`
is expected to
be present by the security manager logic, so this test would fail despite
the application
behaving correctly. Making these assertions conditional on the flags (or
forcing the flags
to the default values in the test) is required to avoid this contradiction.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7b7df7472a7246688284bb68754bb331&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=7b7df7472a7246688284bb68754bb331&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/integration_tests/core_tests.py
**Line:** 168:169
**Comment:**
*Logic Error: This test hard-codes that both password-reset endpoints
are always absent, but `ResetMyPasswordView` is intentionally kept registered
when `ENABLE_FORCE_PASSWORD_CHANGE` is enabled (and both views are registered
when `ENABLE_LEGACY_FAB_PASSWORD_VIEWS` is enabled). As written, the test will
fail under valid config combinations and encodes behavior that contradicts the
security manager logic. Make the assertions conditional on those flags (or
explicitly set the config in-test before asserting).
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=cc6eaa38cdd40502e7fcacc51b340c12b6fbef63329c98f66ac02380e2405fb8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=cc6eaa38cdd40502e7fcacc51b340c12b6fbef63329c98f66ac02380e2405fb8&reaction=dislike'>👎</a>
##########
tests/integration_tests/core_tests.py:
##########
@@ -144,10 +144,14 @@ def test_viz_cache_key(self):
assert cache_key_with_groupby == viz.cache_key(qobj)
def test_admin_only_menu_views(self):
+ from flask import current_app
+
def assert_admin_view_menus_in(role_name, assert_func):
role = security_manager.find_role(role_name)
view_menus = [p.view_menu.name for p in role.permissions]
- assert_func("ResetPasswordView", view_menus)
+ # ResetPasswordView only present when legacy FAB password views
enabled
+ if current_app.config.get("ENABLE_LEGACY_FAB_PASSWORD_VIEWS",
False):
+ assert_func("ResetPasswordView", view_menus)
Review Comment:
**Suggestion:** This conditional only checks `ResetPasswordView` when the
legacy flag is enabled, but when the flag is disabled it performs no assertion
at all, so the test no longer verifies that the permission is actually removed.
Add the complementary negative assertion path so disabled-mode regressions are
detected instead of silently passing. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Legacy FAB reset permission removal untested when flag disabled.
- ⚠️ Future ResetPasswordView gating regressions may go unnoticed.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the admin-only view menu list in `superset/security/manager.py`
around line
1135: `"ResetPasswordView"` is included among admin-only security menu
items, alongside
`"RoleRestAPI"` and `"SQL Lab"`.
2. Examine the FAB view registration gating in
`superset/security/manager.py:_skip_legacy_fab_password_view_registration`
at lines
4802–4827 and its use in `register_views` at lines 4858–4865: when
`ENABLE_LEGACY_FAB_PASSWORD_VIEWS` is False, the wrapper actively filters out
`ResetPasswordView` (and, depending on `ENABLE_FORCE_PASSWORD_CHANGE`,
sometimes
`ResetMyPasswordView`) from registration.
3. Open `tests/integration_tests/core_tests.py:test_admin_only_menu_views`
at lines
146–161: the helper `assert_admin_view_menus_in` builds `view_menus =
[p.view_menu.name
for p in role.permissions]` and then, inside an `if
current_app.config.get("ENABLE_LEGACY_FAB_PASSWORD_VIEWS", False):` block
(lines 153–154),
asserts on `"ResetPasswordView"` using the injected `assert_func`, but
performs no
assertion on `"ResetPasswordView"` at all when the flag is False.
4. Because `assert_admin_view_menus_in` is called for `"Admin"` with
`self.assertIn` and
for `"Alpha"`/`"Gamma"` with `self.assertNotIn` (lines 159–161), the
disabled-flag path
never checks whether `"ResetPasswordView"` is absent from `view_menus` for
any role. As a
result, a regression that leaves `"ResetPasswordView"` permissions or menus
present
despite `ENABLE_LEGACY_FAB_PASSWORD_VIEWS` being False would not cause this
test to fail.
Adding the complementary negative assertion (e.g. asserting absence when the
flag is
False) is necessary to detect such regressions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=69912ac6ad814a46bbd94bd6237e2ec0&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=69912ac6ad814a46bbd94bd6237e2ec0&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/integration_tests/core_tests.py
**Line:** 153:154
**Comment:**
*Incomplete Implementation: This conditional only checks
`ResetPasswordView` when the legacy flag is enabled, but when the flag is
disabled it performs no assertion at all, so the test no longer verifies that
the permission is actually removed. Add the complementary negative assertion
path so disabled-mode regressions are detected instead of silently passing.
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=6c75e5dde1b7cbbf1cb779ca6038c8add7d288f612b1c6cf3ca2310bd632beba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37773&comment_hash=6c75e5dde1b7cbbf1cb779ca6038c8add7d288f612b1c6cf3ca2310bd632beba&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]
