rusackas commented on code in PR #44605:
URL: https://github.com/apache/superset/pull/44605#discussion_r4131187286
##########
superset/views/auth.py:
##########
@@ -39,7 +39,14 @@ def login(self, provider: Optional[str] = None) ->
WerkzeugResponse:
if g.user is not None and g.user.is_authenticated:
return redirect(self.appbuilder.get_url_for_index)
- return super().render_app_template()
+ # Drain pending flash messages (failed logins, session
+ # invalidation, forced password change) so they render here
+ # instead of accumulating in the session. This must go through
+ # per-request extra_bootstrap_data, never the cached common
+ # bootstrap payload, so messages can't bleed between users.
+ return super().render_app_template(
+ {"auth_messages": get_flashed_messages(with_categories=True)}
+ )
Review Comment:
Codeant's off on this one. This view only renders the SPA shell on GET, the
actual login POST and provider routes go through Flask-AppBuilder's own auth
blueprint, untouched by this diff.
##########
superset-frontend/src/pages/Login/Login.test.tsx:
##########
@@ -319,3 +331,79 @@ test('should use ensureAppRoot for all generated URLs with
deep application root
screen.getByRole('link', { name: /Sign in with Google/ }),
).toHaveAttribute('href', '/my-org/superset/login/google');
});
+
+// --- Auth flash messages ---
+
+test('should show flashed auth failures as danger toasts', () => {
Review Comment:
Already renamed a few minutes after this landed (e39309a1), matches the
assertions now.
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -115,26 +120,32 @@ export default function Login() {
const authRegistration: boolean =
bootstrapData.common.conf.AUTH_USER_REGISTRATION;
- // TODO: This is a temporary solution for showing login errors after form
submission.
- // Should be replaced with proper SPA-style authentication (JSON API with
error responses)
- // when Flask-AppBuilder is updated or we implement a custom login endpoint.
+ // Flashed auth messages (failed logins, session invalidation, forced
+ // password change) are drained by the login view into the bootstrap
+ // payload - surface them here, once, on mount.
+ const authMessages = bootstrapData.auth_messages ?? [];
useEffect(() => {
- const loginAttempted = sessionStorage.getItem('login_attempted');
-
- if (loginAttempted === 'true') {
- sessionStorage.removeItem('login_attempted');
- dispatch(addDangerToast(t('Invalid username or password')));
- // Clear password field for security
- form.setFieldsValue({ password: '' });
- }
+ authMessages.forEach(([category, message]) => {
+ if (category === 'success') {
+ dispatch(addSuccessToast(message));
+ } else if (category === 'info' || category === 'message') {
+ dispatch(addInfoToast(message));
+ } else if (category === 'warning') {
+ dispatch(addWarningToast(message));
+ } else {
+ dispatch(addDangerToast(message));
+ }
+ // Clear the password field on auth failures, independent of toast color
+ if (['warning', 'danger', 'error'].includes(category)) {
+ form.setFieldsValue({ password: '' });
Review Comment:
@flcrom fair point, could you add a case that seeds the password field then
asserts it clears on an error-category message?
--
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]
