Skip to content

Commit 4d3381b

Browse files
committed
fix(login): Add error messages for failed login attempts
After removing Flask's flash message system, login errors weren't being displayed. This adds a temporary solution using sessionStorage and toast notifications to show 'Invalid username or password' when login fails. Implementation: - Set a flag in sessionStorage before form submission - On page reload, check if flag exists (indicates failed login) - Show error toast and clear password field for security - Added TODO comment noting this should be replaced with proper SPA auth This addresses reviewer concerns about missing login error messages while keeping the solution simple since it will eventually be replaced with a JSON API approach.
1 parent 3499c7f commit 4d3381b

File tree

3 files changed

+86
-7
lines changed

3 files changed

+86
-7
lines changed

PR_BODY.md

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
## Summary
2+
3+
The Flask flash message system is a legacy feature from when Superset was a server-rendered application. In the current SPA architecture, these messages are never displayed to users because:
4+
- Flash messages only render on page load via FlashProvider's componentDidMount
5+
- Modern UI interactions use async API calls that don't trigger page reloads
6+
- The main consumer (/explore/ POST endpoint) is already marked @deprecated
7+
8+
All user-facing notifications are already handled by the frontend toast system, including chart save operations (see saveModalActions.ts) which dispatches success toasts for:
9+
- Chart saved/overwritten
10+
- Chart added to dashboard
11+
- New dashboard created with chart
12+
13+
## Changes
14+
15+
### Backend
16+
- Removed all flash() calls from views (14 occurrences in 4 files)
17+
- Converted error flashes to JSON responses or logging
18+
- Removed redirect_with_flash utility function
19+
- Fixed open redirect vulnerability in dashboard access denial
20+
21+
### Frontend
22+
- Deleted FlashProvider component and tests
23+
- Removed flash_messages from CommonBootstrapData type
24+
- Cleaned up context providers and test fixtures
25+
- Removed unused getBootstrapData imports
26+
27+
### Security Fixes
28+
- Fixed open redirect vulnerability by using url_for() instead of request.url
29+
- Dashboard access denial now uses safe URL construction
30+
31+
### Code Cleanup
32+
- Removed unnecessary pass statements and comments
33+
- Converted permalink errors to JSON responses for consistency
34+
- Verified no tests depend on flash functionality
35+
36+
## BREAKING CHANGE
37+
Removes flask.flash() messaging infrastructure. However, no actual functionality is lost as the frontend already handles all notifications through its own toast system.
38+
39+
## TESTING INSTRUCTIONS
40+
1. Save a chart from Explore - verify toast notifications appear
41+
2. Add chart to dashboard - verify success message
42+
3. Try accessing a dashboard without permissions - verify proper redirect
43+
4. Run frontend tests: `npm test`
44+
5. Run backend tests: `pytest`
45+
46+
## ADDITIONAL INFORMATION
47+
The application now relies entirely on client-side toast notifications for user feedback, which properly support async operations and provide a consistent UX.
48+
49+
<!--- Required --->
50+
- [x] Has associated issue: Fixes #35236
51+
- [x] Required feature flags: None
52+
- [x] Changes UI: No (removes unused UI component)
53+
- [x] Includes DB Migration: No
54+
55+
<!--- Check any relevant boxes with "x" --->
56+
- [x] Bugfix (non-breaking change which fixes an issue)
57+
- [ ] New feature (non-breaking change which adds functionality)
58+
- [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
59+
- [ ] Documentation (typos, code examples, or any documentation update)

superset-frontend/src/pages/Login/Login.test.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ jest.mock('src/utils/getBootstrapData', () => ({
3333
}));
3434

3535
test('should render login form elements', () => {
36-
render(<Login />);
36+
render(<Login />, { useRedux: true });
3737
expect(screen.getByTestId('login-form')).toBeInTheDocument();
3838
expect(screen.getByTestId('username-input')).toBeInTheDocument();
3939
expect(screen.getByTestId('password-input')).toBeInTheDocument();
@@ -42,13 +42,13 @@ test('should render login form elements', () => {
4242
});
4343

4444
test('should render username and password labels', () => {
45-
render(<Login />);
45+
render(<Login />, { useRedux: true });
4646
expect(screen.getByText('Username:')).toBeInTheDocument();
4747
expect(screen.getByText('Password:')).toBeInTheDocument();
4848
});
4949

5050
test('should render form instruction text', () => {
51-
render(<Login />);
51+
render(<Login />, { useRedux: true });
5252
expect(
5353
screen.getByText('Enter your login and password below:'),
5454
).toBeInTheDocument();

superset-frontend/src/pages/Login/index.tsx

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,10 @@ import {
2727
Typography,
2828
Icons,
2929
} from '@superset-ui/core/components';
30-
import { useState } from 'react';
30+
import { useState, useEffect } from 'react';
3131
import { capitalize } from 'lodash/fp';
32+
import { addDangerToast } from 'src/components/MessageToasts/actions';
33+
import { useDispatch } from 'react-redux';
3234
import getBootstrapData from 'src/utils/getBootstrapData';
3335

3436
type OAuthProvider = {
@@ -77,6 +79,7 @@ const StyledLabel = styled(Typography.Text)`
7779
export default function Login() {
7880
const [form] = Form.useForm<LoginForm>();
7981
const [loading, setLoading] = useState(false);
82+
const dispatch = useDispatch();
8083

8184
const bootstrapData = getBootstrapData();
8285

@@ -85,11 +88,28 @@ export default function Login() {
8588
const authRegistration: boolean =
8689
bootstrapData.common.conf.AUTH_USER_REGISTRATION;
8790

91+
// TODO: This is a temporary solution for showing login errors after form submission.
92+
// Should be replaced with proper SPA-style authentication (JSON API with error responses)
93+
// when Flask-AppBuilder is updated or we implement a custom login endpoint.
94+
useEffect(() => {
95+
const loginAttempted = sessionStorage.getItem('login_attempted');
96+
97+
if (loginAttempted === 'true') {
98+
sessionStorage.removeItem('login_attempted');
99+
dispatch(addDangerToast(t('Invalid username or password')));
100+
// Clear password field for security
101+
form.setFieldsValue({ password: '' });
102+
}
103+
}, [dispatch, form]);
104+
88105
const onFinish = (values: LoginForm) => {
89106
setLoading(true);
90-
SupersetClient.postForm('/login/', values, '').finally(() => {
91-
setLoading(false);
92-
});
107+
108+
// Mark that we're attempting login (for error detection after redirect)
109+
sessionStorage.setItem('login_attempted', 'true');
110+
111+
// Use standard form submission for Flask-AppBuilder compatibility
112+
SupersetClient.postForm('/login/', values, '');
93113
};
94114

95115
const getAuthIconElement = (

0 commit comments

Comments
 (0)