codeant-ai-for-open-source[bot] commented on code in PR #37537:
URL: https://github.com/apache/superset/pull/37537#discussion_r2739234287
##########
superset/config.py:
##########
@@ -741,6 +741,11 @@ class D3TimeFormat(TypedDict, total=False):
# @lifecycle: stable
# @category: runtime_config
"MENU_HIDE_USER_INFO": False,
+ # Hide the logout button in embedded contexts (e.g., when using SSO in
iframes)
+ # @lifecycle: stable
+ # @category: runtime_config
+ # @docs:
https://superset.apache.org/docs/configuration/networking-settings#hiding-the-logout-button-in-embedded-contexts
+ "DISABLE_EMBEDDED_SUPERSET_LOGOUT": False,
Review Comment:
**Suggestion:** Security documentation/warning missing: disabling the logout
button may leave active sessions that cannot be terminated from Superset,
potentially creating orphaned sessions or stale credentials; add an inline
security warning so operators are aware of the risk and consult documentation
before enabling this flag. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Users cannot terminate Superset sessions via UI.
- ⚠️ Potential orphaned Superset sessions if parent app mismanages logout.
- ⚠️ Operators need extra validation before enabling flag.
```
</details>
```suggestion
# SECURITY: Disabling logout prevents users from terminating Superset
sessions via the UI and may produce orphaned sessions if the parent app does
not fully manage session lifecycle. Enable only when the embedding application
completely controls authentication/termination flows.
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect superset/config.py declaration at superset/config.py:744-748 and
enable the
flag by overriding it to True via superset_config.py or env var.
2. Restart Superset and confirm RightMenu
(superset-frontend/src/components/RightMenu.tsx)
hides the logout UI element because the feature flag is active.
3. Note that the logout API/server-side session invalidation endpoint
remains available
(logout endpoints live in the backend auth/security stack), but users cannot
trigger it
through the UI; if the embedding parent application does not actively
terminate Superset
sessions, Superset sessions stay active (orphaned).
4. Observe potential risk: privileged sessions may remain active longer than
intended;
adding an explicit SECURITY comment at superset/config.py:744-748 warns
operators to
validate parent-app-managed session lifecycle before enabling the flag.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 748:748
**Comment:**
*Security: Security documentation/warning missing: disabling the logout
button may leave active sessions that cannot be terminated from Superset,
potentially creating orphaned sessions or stale credentials; add an inline
security warning so operators are aware of the risk and consult documentation
before enabling this flag.
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.
```
</details>
##########
superset-frontend/src/features/home/RightMenu.test.tsx:
##########
@@ -155,6 +165,7 @@ const mockNonExamplesDB = Array.from({ length: 2 })
const useSelectorMock = jest.spyOn(reactRedux, 'useSelector');
beforeEach(async () => {
+ mockIsFeatureEnabled.mockReturnValue(false);
Review Comment:
**Suggestion:** The mocked `isFeatureEnabled` may retain previous
implementations or call history set by other tests; calling `mockReset()` at
the start of each `beforeEach` ensures a clean mock state before setting the
default return value, preventing cross-test interference. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Test behavior may be influenced by prior test mocks.
- ⚠️ RightMenu feature-flag related tests may be brittle.
```
</details>
```suggestion
mockIsFeatureEnabled.mockReset();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `superset-frontend/src/features/home/RightMenu.test.tsx` and find
the
`beforeEach` at lines 167–169 where
`mockIsFeatureEnabled.mockReturnValue(false)` is set
but the mock is not reset.
2. Run the whole file's tests (`yarn test RightMenu.test.tsx`) — some tests
in this file
call `mockIsFeatureEnabled.mockImplementation(...)` (see the 'hides logout
button...'
test). If a previous test set a custom implementation or left call history,
it can
persist.
3. Because the mock instance is reused, residual mockImplementation or call
history from
prior tests can leak into subsequent tests and affect assertions or behavior
that inspects
the mock's calls or relies on a clean mock state.
4. Calling `mockIsFeatureEnabled.mockReset()` at the start of `beforeEach`
guarantees a
clean mock (no implementation, no call history) before setting the default
return value,
removing cross-test interference.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/RightMenu.test.tsx
**Line:** 168:168
**Comment:**
*Possible Bug: The mocked `isFeatureEnabled` may retain previous
implementations or call history set by other tests; calling `mockReset()` at
the start of each `beforeEach` ensures a clean mock state before setting the
default return value, preventing cross-test interference.
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.
```
</details>
##########
superset-frontend/src/features/home/RightMenu.test.tsx:
##########
@@ -398,3 +409,31 @@ test('Logs out and clears local storage item redux', async
() => {
expect(sessionStorage.getItem('login_attempted')).toBeNull();
});
});
+
+test('shows logout button when DISABLE_EMBEDDED_SUPERSET_LOGOUT is false',
async () => {
+ mockIsFeatureEnabled.mockReturnValue(false);
+ resetUseSelectorMock();
+ render(<RightMenu {...createProps()} />, {
+ useRedux: true,
+ useQueryParams: true,
+ useTheme: true,
+ });
+
+ userEvent.hover(await screen.findByText(/Settings/i));
+ expect(await screen.findByText('Logout')).toBeInTheDocument();
+});
+
+test('hides logout button when DISABLE_EMBEDDED_SUPERSET_LOGOUT is true',
async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.DisableEmbeddedSupersetLogout,
+ );
+ resetUseSelectorMock();
+ render(<RightMenu {...createProps()} />, {
+ useRedux: true,
+ useQueryParams: true,
+ useTheme: true,
+ });
+
+ userEvent.hover(await screen.findByText(/Settings/i));
+ expect(screen.queryByText('Logout')).not.toBeInTheDocument();
Review Comment:
**Suggestion:** The "hides logout button" test asserts absence immediately
after hovering, which can race with menu rendering and produce flaky/pass-false
positives; wait for the menu state to settle (use waitFor) before asserting
that "Logout" is not in the DOM. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ RightMenu test intermittently flakes.
- ⚠️ CI runs may show intermittent failures.
```
</details>
```suggestion
await waitFor(() => {
expect(screen.queryByText('Logout')).not.toBeInTheDocument();
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/features/home/RightMenu.test.tsx` and locate
the test
`hides logout button when DISABLE_EMBEDDED_SUPERSET_LOGOUT is true` (the
test block
contains the lines at 426–439, including the hover and assertion at lines
437–438).
2. Run the single test or the whole test file using Jest (e.g., `yarn test
RightMenu.test.tsx`).
3. When the Settings menu renders asynchronously, the immediate
`expect(screen.queryByText('Logout')).not.toBeInTheDocument()` at line 438
can execute
before the menu stabilizes, producing intermittent failures or false passes
depending on
timing.
4. Replace the immediate assertion with a `waitFor` around the expectation
(as suggested)
so the test waits for the menu to settle before asserting absence; this
prevents the
timing race documented in this file.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/RightMenu.test.tsx
**Line:** 438:438
**Comment:**
*Possible Bug: The "hides logout button" test asserts absence
immediately after hovering, which can race with menu rendering and produce
flaky/pass-false positives; wait for the menu state to settle (use waitFor)
before asserting that "Logout" is not in the DOM.
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.
```
</details>
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -489,15 +494,17 @@ const RightMenu = ({
),
});
}
- userItems.push({
- key: 'logout',
- label: (
- <Typography.Link href={navbarRight.user_logout_url}>
- {t('Logout')}
- </Typography.Link>
- ),
- onClick: handleLogout,
- });
+ if (!isFeatureEnabled(FeatureFlag.DisableEmbeddedSupersetLogout)) {
+ userItems.push({
+ key: 'logout',
+ label: (
+ <Typography.Link href={navbarRight.user_logout_url}>
Review Comment:
**Suggestion:** The logout link uses `navbarRight.user_logout_url` directly
without calling `ensureAppRoot`, while other user URLs (like `user_info_url`)
are wrapped with `ensureAppRoot`. If `user_logout_url` is a relative path or
undefined this can produce an incorrect href or broken navigation; wrap the URL
with `ensureAppRoot` (and default to an empty string) to match existing
behavior. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Settings menu logout anchor may point to relative path.
- ⚠️ Invalid logout href when logout URL is unset.
```
</details>
```suggestion
<Typography.Link
href={ensureAppRoot(navbarRight.user_logout_url || '')}>
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the file superset-frontend/src/features/home/RightMenu.tsx and
locate the logout
rendering block in the RightMenu component (lines 497-507 in the PR hunk).
Observe the
anchor is rendered as: <Typography.Link href={navbarRight.user_logout_url}>
(lines
500-502).
2. Render the RightMenu component in isolation (e.g., a unit test or
Storybook) passing a
navbarRight prop with a relative logout path: { user_is_anonymous: false,
user_logout_url:
'/logout' } and ensure the feature flag DisableEmbeddedSupersetLogout is
false so the
block executes.
3. Inspect the rendered DOM for the Logout anchor: verify the href attribute
is exactly
the raw navbarRight.user_logout_url value ("/logout") and not resolved to
the application
root. This demonstrates relative path handling difference from other links
that use
ensureAppRoot (see user_info_url handling at lines ~488-494).
4. Repeat rendering with navbarRight.user_logout_url undefined (or empty
string) and
observe the anchor's href becomes missing/empty or potentially invalid. This
reproduces
the broken/incorrect href behavior caused by not normalizing the logout URL.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/RightMenu.tsx
**Line:** 501:501
**Comment:**
*Possible Bug: The logout link uses `navbarRight.user_logout_url`
directly without calling `ensureAppRoot`, while other user URLs (like
`user_info_url`) are wrapped with `ensureAppRoot`. If `user_logout_url` is a
relative path or undefined this can produce an incorrect href or broken
navigation; wrap the URL with `ensureAppRoot` (and default to an empty string)
to match existing behavior.
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.
```
</details>
##########
docs/docs/configuration/networking-settings.mdx:
##########
@@ -96,6 +96,22 @@ To enable this entry, add the following line to the `.env`
file:
SUPERSET_FEATURE_EMBEDDED_SUPERSET=true
```
+### Hiding the Logout Button in Embedded Contexts
+
+When Superset is embedded in an application that manages authentication via
SSO (OAuth2, SAML, or JWT), the logout button should be hidden since session
management is handled by the parent application.
+
+To hide the logout button, add to `superset_config.py`:
+
+```python
+FEATURE_FLAGS = {
+ "DISABLE_EMBEDDED_SUPERSET_LOGOUT": True,
+}
Review Comment:
**Suggestion:** Overwriting `FEATURE_FLAGS` with a new dict will clobber any
existing feature flags in a user's `superset_config.py`; instead merge or
update the existing `FEATURE_FLAGS` so other flags are preserved. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Overwrites user-defined feature flags silently.
- ❌ Unexpected UI/feature disappearance after startup.
- ⚠️ Admins misconfigure embedded deployments.
```
</details>
```suggestion
try:
FEATURE_FLAGS.update({"DISABLE_EMBEDDED_SUPERSET_LOGOUT": True})
except NameError:
FEATURE_FLAGS = {"DISABLE_EMBEDDED_SUPERSET_LOGOUT": True}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the documentation snippet added at
`docs/docs/configuration/networking-settings.mdx:103-109` and copy the code
block shown at
`docs/docs/configuration/networking-settings.mdx:105-109` into your
`superset_config.py`
file as-is.
2. In `superset_config.py` (user-managed config), place the assignment
`FEATURE_FLAGS = {
"DISABLE_EMBEDDED_SUPERSET_LOGOUT": True }` (from docs line 105-109). This
is a direct
Python assignment that replaces the module-level name `FEATURE_FLAGS`.
3. Start Superset (the Flask app reads `superset_config.py` at startup). The
Python name
`FEATURE_FLAGS` in the module is the final value; any previously defined
flags the user
had set earlier in their file (or merged in via other helpers) are
overwritten by this
assignment.
4. Verify by inspecting effective feature flags (e.g., by printing/logging
`app.config.get("FEATURE_FLAGS")` during startup or observing UI/features
previously
enabled by other flags are now missing). This demonstrates the concrete
overwrite behavior
caused by a direct assignment.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/configuration/networking-settings.mdx
**Line:** 106:108
**Comment:**
*Logic Error: Overwriting `FEATURE_FLAGS` with a new dict will clobber
any existing feature flags in a user's `superset_config.py`; instead merge or
update the existing `FEATURE_FLAGS` so other flags are preserved.
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.
```
</details>
##########
docs/docs/configuration/networking-settings.mdx:
##########
@@ -96,6 +96,22 @@ To enable this entry, add the following line to the `.env`
file:
SUPERSET_FEATURE_EMBEDDED_SUPERSET=true
```
+### Hiding the Logout Button in Embedded Contexts
+
+When Superset is embedded in an application that manages authentication via
SSO (OAuth2, SAML, or JWT), the logout button should be hidden since session
management is handled by the parent application.
Review Comment:
**Suggestion:** The text implies setting the feature flag fully disables
logout, but the implemented feature flag only hides the logout UI; it does not
prevent requests to the server-side logout endpoint — clarify this in the docs
so operators are not left with a false sense of protection and can take
additional backend measures if needed. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Operators may assume logout is disabled.
- ❌ Logout endpoint remains callable at /logout.
- ⚠️ Confusing SSO session management expectations.
```
</details>
```suggestion
When Superset is embedded in an application that manages authentication via
SSO (OAuth2, SAML, or JWT), the logout button should be hidden in the UI since
session management is handled by the parent application. Note that this feature
flag only hides the logout menu item in the frontend UI — it does not disable
or protect the server-side logout endpoint; if you need to prevent logout
requests or enforce logout behavior centrally, also secure or disable the
backend logout route accordingly.
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Follow the docs and add the feature flag exactly as shown at
`docs/docs/configuration/networking-settings.mdx:105-109` (copy
`FEATURE_FLAGS = {
"DISABLE_EMBEDDED_SUPERSET_LOGOUT": True }` into `superset_config.py`).
2. Start Superset with that configuration. The frontend change added in the
PR wraps the
logout menu item in `RightMenu` with `isFeatureEnabled()` (PR description
notes change to
`RightMenu.tsx`), so the Settings dropdown will not render the Logout item
in the client
UI.
3. From a browser, confirm the Settings dropdown no longer shows the Logout
entry
(observed UI effect coming from `RightMenu.tsx` as described in the PR).
4. To verify server-side behavior still exists, issue a direct HTTP request
to the logout
endpoint (e.g., GET/POST to `/logout`). The server-side logout route remains
active unless
additional backend changes were made; the feature flag only suppressed the
UI element.
This reproduces the gap between hiding the UI and disabling the endpoint.
Note: the PR description explicitly states the change is a UI conditional in
`RightMenu.tsx` (frontend). The documentation should therefore clarify the
scope (UI-only)
so operators do not assume the backend route is protected.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/configuration/networking-settings.mdx
**Line:** 101:101
**Comment:**
*Security: The text implies setting the feature flag fully disables
logout, but the implemented feature flag only hides the logout UI; it does not
prevent requests to the server-side logout endpoint — clarify this in the docs
so operators are not left with a false sense of protection and can take
additional backend measures if needed.
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.
```
</details>
--
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]
