Browse Source

fix(auth): render Remember Me on SSO sign in page when local login is disabled (#2784) (#3117)

Vũ Thành Trung 3 days ago
parent
commit
93a88b21e0
2 changed files with 245 additions and 11 deletions
  1. 211 0
      frontend/src/__tests__/pages/LoginPage.test.tsx
  2. 34 11
      frontend/src/pages/LoginPage.tsx

+ 211 - 0
frontend/src/__tests__/pages/LoginPage.test.tsx

@@ -19,6 +19,13 @@ vi.mock('react-router-dom', async (importActual) => {
   return { ...actual, useNavigate: () => mockNavigate };
 });
 
+// The setup file's Location proxy. Several tests below swap window.location
+// for a plain object so the OIDC redirect's href write can be read back.
+// Everything restored in the afterEach below used to leak into whichever test
+// ran next: a stale location object hid the URL it set with pushState, and a
+// token left by a successful login signed in a visitor meant to be anonymous.
+const realLocation = window.location;
+
 describe('LoginPage', () => {
   beforeEach(() => {
     server.use(
@@ -28,6 +35,12 @@ describe('LoginPage', () => {
     );
   });
 
+  afterEach(() => {
+    Object.defineProperty(window, 'location', { configurable: true, writable: true, value: realLocation });
+    setAuthToken(null);
+    sessionStorage.clear();
+  });
+
   describe('rendering', () => {
     it('renders the login form', async () => {
       render(<LoginPage />);
@@ -638,6 +651,204 @@ describe('LoginPage', () => {
     });
   });
 
+  // #2784: Remember Me on SSO sign in page when local login is disabled
+  describe('SSO login with local login disabled (#2784)', () => {
+    const mockProviders = [
+      {
+        id: 101,
+        name: 'Authentik',
+        issuer_url: 'https://auth.test',
+        client_id: 'client-101',
+        is_enabled: true,
+        icon_url: null,
+        has_icon: false,
+        email_claim: 'email',
+        require_email_verified: true,
+        auto_create_users: false,
+        auto_link_existing_accounts: false,
+      },
+    ];
+
+    beforeEach(() => {
+      vi.mocked(localStorage.setItem).mockClear();
+      sessionStorage.clear();
+      server.use(
+        http.get('/api/v1/auth/advanced-auth/status', () =>
+          HttpResponse.json({
+            advanced_auth_enabled: true,
+            smtp_configured: false,
+            local_login_enabled: false,
+            autologin_provider_id: null,
+          })
+        ),
+        http.get('/api/v1/auth/oidc/providers', () =>
+          HttpResponse.json(mockProviders)
+        ),
+        http.get('/api/v1/auth/oidc/authorize/101', () =>
+          HttpResponse.json({ auth_url: 'https://auth.test/authorize?state=xyz' })
+        )
+      );
+    });
+
+    afterEach(() => {
+      sessionStorage.clear();
+    });
+
+    it('renders Remember Me checkbox on SSO login page when local login is disabled', async () => {
+      render(<LoginPage />);
+
+      // Notice for disabled local login should be shown
+      await waitFor(() => {
+        expect(screen.getByText(/Local sign-in is disabled/i)).toBeInTheDocument();
+      });
+
+      // Local username & password inputs should NOT be shown
+      expect(screen.queryByLabelText(/Username/i)).not.toBeInTheDocument();
+      expect(screen.queryByLabelText(/Password/i)).not.toBeInTheDocument();
+
+      // SSO provider button should be shown
+      expect(screen.getByRole('button', { name: /Authentik/i })).toBeInTheDocument();
+
+      // "or continue with" divider should NOT be shown when local login is disabled
+      expect(screen.queryByText(/or continue with/i)).not.toBeInTheDocument();
+
+      // Remember Me checkbox MUST be rendered and unchecked by default
+      const rememberCheckbox = screen.getByRole('checkbox', { name: /Remember Me/i });
+      expect(rememberCheckbox).toBeInTheDocument();
+      expect(rememberCheckbox).not.toBeChecked();
+    });
+
+    it('writes auth_remember_me flag to sessionStorage before SSO redirect when Remember Me is checked', async () => {
+      const user = userEvent.setup();
+      render(<LoginPage />);
+
+      await waitFor(() => {
+        expect(screen.getByRole('button', { name: /Authentik/i })).toBeInTheDocument();
+      });
+
+      const rememberCheckbox = screen.getByRole('checkbox', { name: /Remember Me/i });
+      await user.click(rememberCheckbox);
+      expect(rememberCheckbox).toBeChecked();
+
+      Object.defineProperty(window, 'location', {
+        writable: true,
+        value: { ...window.location, href: 'http://localhost:3000/' },
+      });
+
+      await user.click(screen.getByRole('button', { name: /Authentik/i }));
+
+      await waitFor(() => {
+        expect(sessionStorage.getItem('auth_remember_me')).toBe('1');
+      });
+    });
+
+    it('does not write auth_remember_me flag when Remember Me is not checked', async () => {
+      const user = userEvent.setup();
+      render(<LoginPage />);
+
+      await waitFor(() => {
+        expect(screen.getByRole('button', { name: /Authentik/i })).toBeInTheDocument();
+      });
+
+      Object.defineProperty(window, 'location', {
+        writable: true,
+        value: { ...window.location, href: 'http://localhost:3000/' },
+      });
+
+      await user.click(screen.getByRole('button', { name: /Authentik/i }));
+
+      // The flag starts out null, so wait for the redirect itself before
+      // asserting on it -- otherwise this passes before the click is handled.
+      await waitFor(() => {
+        expect(window.location.href).toBe('https://auth.test/authorize?state=xyz');
+      });
+      expect(sessionStorage.getItem('auth_remember_me')).toBeNull();
+    });
+
+    it('cleans up stale auth_remember_me flag if Remember Me is unchecked before SSO redirect', async () => {
+      sessionStorage.setItem('auth_remember_me', '1');
+      const user = userEvent.setup();
+      render(<LoginPage />);
+
+      await waitFor(() => {
+        expect(screen.getByRole('button', { name: /Authentik/i })).toBeInTheDocument();
+      });
+
+      Object.defineProperty(window, 'location', {
+        writable: true,
+        value: { ...window.location, href: 'http://localhost:3000/' },
+      });
+
+      // Clicking SSO without checking Remember Me should remove the stale flag
+      await user.click(screen.getByRole('button', { name: /Authentik/i }));
+
+      // The flag starts out null, so wait for the redirect itself before
+      // asserting on it -- otherwise this passes before the click is handled.
+      await waitFor(() => {
+        expect(window.location.href).toBe('https://auth.test/authorize?state=xyz');
+      });
+      expect(sessionStorage.getItem('auth_remember_me')).toBeNull();
+    });
+  });
+
+  // #1589 autologin: the page redirects to the IdP on mount, so the
+  // destination the user was sent to /login from has to be stashed first, or
+  // the round-trip lands them on "/" instead (#2784 review).
+  describe('OIDC autologin keeps the post-login destination', () => {
+    beforeEach(() => {
+      sessionStorage.clear();
+      server.use(
+        http.get('/api/v1/auth/advanced-auth/status', () =>
+          HttpResponse.json({
+            advanced_auth_enabled: true,
+            smtp_configured: false,
+            local_login_enabled: false,
+            autologin_provider_id: 101,
+          })
+        ),
+        http.get('/api/v1/auth/oidc/providers', () => HttpResponse.json([])),
+        http.get('/api/v1/auth/oidc/authorize/101', () =>
+          HttpResponse.json({ auth_url: 'https://auth.test/authorize?state=xyz' })
+        )
+      );
+    });
+
+    afterEach(() => {
+      window.history.replaceState(null, '', '/');
+    });
+
+    // BrowserRouter reads router state from history.state.usr, which is where
+    // ProtectedRoute's navigate('/login', { state: { from } }) puts it.
+    function arriveAtLogin(from?: { pathname: string; search?: string }) {
+      window.history.replaceState(from ? { usr: { from }, key: 'test', idx: 0 } : null, '', '/login');
+      Object.defineProperty(window, 'location', {
+        configurable: true,
+        writable: true,
+        value: { ...window.location, href: 'http://localhost:3000/login' },
+      });
+    }
+
+    it('stashes the page the user was sent from before redirecting to the IdP', async () => {
+      arriveAtLogin({ pathname: '/archives', search: '?id=5' });
+      render(<LoginPage />);
+
+      await waitFor(() => {
+        expect(window.location.href).toBe('https://auth.test/authorize?state=xyz');
+      });
+      expect(sessionStorage.getItem('auth_post_login_redirect')).toBe('/archives?id=5');
+    });
+
+    it('stashes nothing when the user opened /login directly', async () => {
+      arriveAtLogin();
+      render(<LoginPage />);
+
+      await waitFor(() => {
+        expect(window.location.href).toBe('https://auth.test/authorize?state=xyz');
+      });
+      expect(sessionStorage.getItem('auth_post_login_redirect')).toBeNull();
+    });
+  });
+
   // #1333: icon proxy — login page renders <img src> from /icon endpoint
   // rather than the upstream icon_url, so the strict img-src CSP holds.
   describe('OIDC icon proxy (#1333)', () => {

+ 34 - 11
frontend/src/pages/LoginPage.tsx

@@ -207,12 +207,16 @@ export function LoginPage() {
     );
     Promise.race([api.getOIDCAuthorizeUrl(providerId), timeoutPromise])
       .then((result) => {
+        const fromState = (location.state as { from?: { pathname?: string; search?: string } } | null)?.from;
+        if (fromState?.pathname) {
+          stashPostLoginRedirect(`${fromState.pathname}${fromState.search ?? ''}`);
+        }
         window.location.href = (result as { auth_url: string }).auth_url;
       })
       .catch(() => {
         setAutologinFailed(true);
       });
-  }, [advancedAuthStatus, searchParams]);
+  }, [advancedAuthStatus, searchParams, location.state]);
 
   const localLoginEnabled = advancedAuthStatus?.local_login_enabled !== false;
   const showAutologinBanner = autologinFailed && advancedAuthStatus?.autologin_provider_id != null;
@@ -391,12 +395,14 @@ export function LoginPage() {
   const oidcLoginMutation = useMutation({
     mutationFn: (providerId: number) => api.getOIDCAuthorizeUrl(providerId),
     onSuccess: (data) => {
-      if (rememberMe) {
-        try {
+      try {
+        if (rememberMe) {
           sessionStorage.setItem(REMEMBER_ME_KEY, '1');
-        } catch (err) {
-          console.warn('setItem auth_remember_me failed, Remember Me will not carry through OIDC redirect', err);
+        } else {
+          sessionStorage.removeItem(REMEMBER_ME_KEY);
         }
+      } catch (err) {
+        console.warn('setItem/removeItem auth_remember_me failed, Remember Me will not carry through OIDC redirect', err);
       }
       // Stash the post-login destination from router state so it survives the
       // provider round-trip (window.location.href kills React state). If the
@@ -809,14 +815,31 @@ export function LoginPage() {
         {/* OIDC provider buttons */}
         {oidcProviders && oidcProviders.length > 0 && (
           <div className="space-y-3">
-            <div className="relative">
-              <div className="absolute inset-0 flex items-center">
-                <div className="w-full border-t border-bambu-dark-tertiary" />
+            {localLoginEnabled && (
+              <div className="relative">
+                <div className="absolute inset-0 flex items-center">
+                  <div className="w-full border-t border-bambu-dark-tertiary" />
+                </div>
+                <div className="relative flex justify-center text-sm">
+                  <span className="px-2 bg-bambu-dark-secondary text-bambu-gray">{t('login.twoFA.orContinueWith')}</span>
+                </div>
               </div>
-              <div className="relative flex justify-center text-sm">
-                <span className="px-2 bg-bambu-dark-secondary text-bambu-gray">{t('login.twoFA.orContinueWith')}</span>
+            )}
+
+            {!localLoginEnabled && (
+              <div className="flex items-center gap-2">
+                <input
+                  id="remember-me"
+                  type="checkbox"
+                  checked={rememberMe}
+                  onChange={(e) => setRememberMe(e.target.checked)}
+                  className="h-4 w-4 rounded border-bambu-dark-tertiary bg-bambu-dark-secondary text-bambu-green focus:ring-bambu-green/50 cursor-pointer"
+                />
+                <label htmlFor="remember-me" className="text-sm text-bambu-gray cursor-pointer">
+                  {t('login.rememberMe')}
+                </label>
               </div>
-            </div>
+            )}
 
             <div className="space-y-2">
               {oidcProviders.map((provider) => (