-
Notifications
You must be signed in to change notification settings - Fork 76
[FIX] Redirect logged-in users from / to /home #470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "os" | ||
|
|
||
| "github.com/gorilla/sessions" | ||
| ) | ||
|
|
||
| func sessionCookieOptions() *sessions.Options { | ||
| return &sessions.Options{ | ||
| Path: "/", | ||
| MaxAge: 86400 * 7, // 7 days | ||
| HttpOnly: true, // Prevent JavaScript access | ||
| Secure: os.Getenv("ENV") == "production", // HTTPS only in production | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No behavior change vs what is already on main: 7-day Pulled into a helper only so we can unit-test those flags (review on #462 asked for session tests). I did not take #462's 30-day lifetime or the |
||
| SameSite: http.SameSiteLaxMode, // CSRF protection (Lax allows OAuth redirects) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestSessionCookieOptions_Production(t *testing.T) { | ||
| t.Setenv("ENV", "production") | ||
|
|
||
| opts := sessionCookieOptions() | ||
|
|
||
| assert.Equal(t, "/", opts.Path) | ||
| assert.Equal(t, 86400*7, opts.MaxAge) | ||
| assert.True(t, opts.HttpOnly) | ||
| assert.True(t, opts.Secure) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Covers the two cookie properties that matter for #417 on HTTPS: The other two tests lock |
||
| assert.Equal(t, http.SameSiteLaxMode, opts.SameSite) | ||
| } | ||
|
|
||
| func TestSessionCookieOptions_NonProduction(t *testing.T) { | ||
| t.Setenv("ENV", "development") | ||
|
|
||
| opts := sessionCookieOptions() | ||
|
|
||
| assert.Equal(t, 86400*7, opts.MaxAge) | ||
| assert.True(t, opts.HttpOnly) | ||
| assert.False(t, opts.Secure) | ||
| } | ||
|
|
||
| func TestSessionCookieOptions_UnsetENV(t *testing.T) { | ||
| t.Setenv("ENV", "") | ||
|
|
||
| opts := sessionCookieOptions() | ||
|
|
||
| assert.Equal(t, 86400*7, opts.MaxAge) | ||
| assert.False(t, opts.Secure) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| import { useEffect } from 'react'; | ||
| import { useNavigate } from 'react-router'; | ||
| import { url } from '@/components/utils/URLs'; | ||
| import { About } from './LandingComponents/About/About'; | ||
| import { FAQ } from './LandingComponents/FAQ/FAQ'; | ||
| import { Footer } from './LandingComponents/Footer/Footer'; | ||
|
|
@@ -9,6 +12,26 @@ import { Contact } from './LandingComponents/Contact/Contact'; | |
| import '../App.css'; | ||
|
|
||
| export const LandingPage = () => { | ||
| const navigate = useNavigate(); | ||
|
|
||
| useEffect(() => { | ||
| const redirectIfLoggedIn = async () => { | ||
| try { | ||
| const response = await fetch(url.backendURL + 'api/user', { | ||
| method: 'GET', | ||
| credentials: 'include', | ||
| }); | ||
| if (response.ok) { | ||
| navigate('/home'); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the user-visible fix for bookmarking Same check HomePage already uses ( I did not add a loading gate — a brief flash of the landing page is possible before redirect. Happy to add a spinner if maintainers want that. |
||
| } | ||
| } catch (error) { | ||
| console.error('Error checking login status:', error); | ||
| } | ||
| }; | ||
|
|
||
| redirectIfLoggedIn(); | ||
| }, [navigate]); | ||
|
|
||
| return ( | ||
| <div className="overflow-x-hidden"> | ||
| <Navbar /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,11 @@ | ||
| import { render, screen } from '@testing-library/react'; | ||
| import { render, screen, waitFor } from '@testing-library/react'; | ||
| import { LandingPage } from '../LandingPage'; | ||
|
|
||
| const mockedNavigate = jest.fn(); | ||
| const consoleErrorSpy = jest | ||
| .spyOn(console, 'error') | ||
| .mockImplementation(() => {}); | ||
|
|
||
| // Mock dependencies | ||
| jest.mock('../LandingComponents/Navbar/Navbar', () => ({ | ||
| Navbar: () => <div>Mocked Navbar</div>, | ||
|
|
@@ -27,7 +32,28 @@ jest.mock('../../components/utils/ScrollToTop', () => ({ | |
| ScrollToTop: () => <div>Mocked ScrollToTop</div>, | ||
| })); | ||
|
|
||
| jest.mock('react-router', () => ({ | ||
| useNavigate: () => mockedNavigate, | ||
| })); | ||
|
|
||
| jest.mock('@/components/utils/URLs', () => ({ | ||
| url: { | ||
| backendURL: 'http://mocked-backend-url/', | ||
| }, | ||
| })); | ||
|
|
||
| global.fetch = jest.fn(() => | ||
| Promise.resolve({ | ||
| ok: false, | ||
| }) | ||
| ) as jest.Mock; | ||
|
|
||
| describe('LandingPage', () => { | ||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| consoleErrorSpy.mockClear(); | ||
| }); | ||
|
|
||
| it('renders all components correctly', () => { | ||
| render(<LandingPage />); | ||
|
|
||
|
|
@@ -40,6 +66,45 @@ describe('LandingPage', () => { | |
| expect(screen.getByText('Mocked Footer')).toBeInTheDocument(); | ||
| expect(screen.getByText('Mocked ScrollToTop')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('redirects to /home when the user is already logged in', async () => { | ||
| (fetch as jest.Mock).mockResolvedValueOnce({ | ||
| ok: true, | ||
| }); | ||
|
|
||
| render(<LandingPage />); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockedNavigate).toHaveBeenCalledWith('/home'); | ||
| }); | ||
| expect(fetch).toHaveBeenCalledWith('http://mocked-backend-url/api/user', { | ||
| method: 'GET', | ||
| credentials: 'include', | ||
| }); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default This case asserts both the redirect and that the request includes credentials (the cookie is what makes production |
||
| }); | ||
|
|
||
| it('stays on the landing page when the user is not logged in', async () => { | ||
| render(<LandingPage />); | ||
|
|
||
| await waitFor(() => { | ||
| expect(fetch).toHaveBeenCalledWith('http://mocked-backend-url/api/user', { | ||
| method: 'GET', | ||
| credentials: 'include', | ||
| }); | ||
| }); | ||
| expect(mockedNavigate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('stays on the landing page when the session check fails', async () => { | ||
| (fetch as jest.Mock).mockRejectedValueOnce(new Error('network error')); | ||
|
|
||
| render(<LandingPage />); | ||
|
|
||
| await waitFor(() => { | ||
| expect(consoleErrorSpy).toHaveBeenCalled(); | ||
| }); | ||
| expect(mockedNavigate).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('LandingPage Component using Snapshot', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,11 @@ spec: | |
| configMapKeyRef: | ||
| key: CONTAINER_ORIGIN | ||
| name: backend-env | ||
| - name: ENV | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| key: ENV | ||
| name: backend-env | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wires the configmap |
||
| - name: FRONTEND_ORIGIN_DEV | ||
| valueFrom: | ||
| configMapKeyRef: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ data: | |
| CLIENT_ID: "YOUR_GOOGLE_CLOUD_AUTH_CLIENT_ID" # Replace this in order to access the frontend | ||
| CLIENT_SEC: "YOUR_GOOGLE_CLOUD_AUTH_CLIENT_SECRET" # Replace this in order to access the frontend | ||
| CONTAINER_ORIGIN: http://syncserver:8080/ | ||
| ENV: "production" # Required for secure HTTPS-only cookies | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same leftover as #420. Docker already had Must stay paired with the |
||
| FRONTEND_ORIGIN_DEV: http://localhost | ||
| PORT: "8000" | ||
| REDIRECT_URL_DEV: http://localhost:8000/auth/callback | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call-site only. Options values are unchanged. Auth middleware / session save path left as-is on purpose — no third copy of the #462 rewrite.