Conversation
- Introduced LOCAL auth mode
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change introduces support for both Google OAuth and local username/email and password authentication in SurfSense. It conditionally configures backend authentication logic, updates environment variables, modifies database models, and adds frontend login and registration forms. Documentation and UI are updated to reflect the dual authentication options and clarify setup for each method. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
User->>Frontend: Open Login Page
Frontend->>Frontend: Reads AUTH_TYPE from env
alt AUTH_TYPE == "GOOGLE"
Frontend->>User: Render GoogleLoginButton
User->>Frontend: Clicks Google Login
Frontend->>Backend: Start OAuth flow
Backend->>Google: OAuth handshake
Google->>Backend: Callback with token
Backend->>Frontend: Redirect with access token
Frontend->>User: User logged in
else AUTH_TYPE == "LOCAL"
Frontend->>User: Render LocalLoginForm
User->>Frontend: Submit email/password
Frontend->>Backend: POST /auth/jwt/login
Backend->>Frontend: Return access token (JSON)
Frontend->>User: User logged in
end
sequenceDiagram
participant User
participant Frontend
participant Backend
User->>Frontend: Open Registration Page
Frontend->>Frontend: Reads AUTH_TYPE from env
alt AUTH_TYPE == "LOCAL"
Frontend->>User: Render registration form
User->>Frontend: Submit registration data
Frontend->>Backend: POST /auth/register
Backend->>Frontend: Return success or error
Frontend->>User: Redirect to login with success message
else AUTH_TYPE == "GOOGLE"
Frontend->>User: Redirect to login page
end
Assessment against linked issues
Possibly related PRs
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
1 similar comment
|
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| from httpx_oauth.clients.google import GoogleOAuth2 | ||
|
|
||
| from fastapi.responses import JSONResponse | ||
| from fastapi_users.schemas import model_dump |
There was a problem hiding this comment.
Incorrect import and usage of model_dump. The code imports model_dump as a function from fastapi_users.schemas, but model_dump is actually an instance method of Pydantic models in recent versions. The correct way would be to call model_dump() directly on the bearer_response instance (bearer_response.model_dump()) rather than importing it as a function. This will cause a runtime error when trying to serialize the response.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| if config.AUTH_TYPE == "GOOGLE": | ||
| return RedirectResponse(redirect_url, status_code=302) | ||
| else: | ||
| return JSONResponse(model_dump(bearer_response)) |
There was a problem hiding this comment.
Incorrect usage of model_dump as a function. The bearer_response is a Pydantic model (BearerResponse), and model_dump should be called as an instance method. The correct code should be: return JSONResponse(bearer_response.model_dump()). The current code will raise an AttributeError at runtime.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 2 issues. Time to roll up your sleeves! 😱 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
surfsense_web/content/docs/docker-installation.mdx (1)
240-242: 🛠️ Refactor suggestionUpdate next steps section to reflect both auth types
The "Next Steps" section still assumes Google authentication only, mentioning "log in using your Google account". This should be updated to reflect both authentication options.
- Once your installation is complete, you can start using SurfSense! Navigate to the frontend URL and log in using your Google account. + Once your installation is complete, you can start using SurfSense! Navigate to the frontend URL and log in using either your Google account or local credentials, depending on your configured AUTH_TYPE.surfsense_backend/app/users.py (1)
81-89: 💡 Verification agent🧩 Analysis chain
Security enhancement for token response.
The current implementation returns the access token directly in the response body for local authentication. This could potentially expose the token to interception if not over HTTPS.
Verify that:
- All communications are over HTTPS in production
- Consider adding token expiration information in the response
- Implement additional security headers
🏁 Script executed:
#!/bin/bash # Check for HTTPS configuration and security headers in the codebase # Look for HTTPS/TLS configuration rg -A 5 "https|ssl|tls|certificate" --glob="*.{py,md,mdx}" # Check for security headers implementation rg -A 3 "security|headers|HSTS|X-Content|X-Frame|X-XSS" --glob="*.py"Length of output: 35665
Security Improvements for Token Response
Please address the following actionable items:
Enforce HTTPS in Production
Ensure your deployment (e.g., ingress, load-balancer or API gateway) terminates TLS on all endpoints so that thetokenquery parameter and JSON payload are never sent over plaintext HTTP.Include Token Expiration Data
Extend the response model to include anexpires_in(or expiration timestamp) field so clients know when the token will lapse. For example:class BearerResponse(BaseModel): access_token: str token_type: str expires_in: int # seconds until expiryAdd Standard Security Headers
Introduce middleware to inject headers such as:
- Strict-Transport-Security
- X-Content-Type-Options: nosniff
- X-Frame-Options: DENY
- X-XSS-Protection: 1; mode=block
You can use Starlette’s
Middlewareor a package likefastapi-secure-headersto automate this.Fix these in:
surfsense_backend/app/users.py→ updateget_login_responsesignature and payloadsurfsense_backend/app/app.py→ register security-headers middlewaresurfsense_web/content/docs/manual-installation.mdx (2)
15-17: 🛠️ Refactor suggestionUpdate prerequisites to reflect optional Google OAuth.
The prerequisites still list "Google OAuth setup" as a requirement, but it's now optional since local authentication is supported. Update this to clarify that Google OAuth is only needed for that authentication method.
- Google OAuth setup + Authentication setup (Google OAuth or local authentication)
278-284: 🛠️ Refactor suggestionUpdate verification section to include local authentication.
The verification section only mentions signing in with a Google account, but should be updated to include both authentication methods.
1. Open your browser and navigate to `http://localhost:3000` -2. Sign in with your Google account +2. Sign in with your Google account (if using Google authentication) or with your email and password (if using local authentication) 3. Create a search space and try uploading a document 4. Test the chat functionality with your uploaded content
🧹 Nitpick comments (16)
surfsense_web/components/Navbar.tsx (5)
3-3: Unused import detected.The
IconBrandGoogleFilledis imported but no longer used after replacing it with the genericIconUsericon.-import { IconMenu2, IconX, IconBrandGoogleFilled, IconUser } from "@tabler/icons-react"; +import { IconMenu2, IconX, IconUser } from "@tabler/icons-react";
66-69: Rename function for clarity.The function name
handleGoogleLoginis no longer accurate since it now redirects to a generic login page that supports multiple authentication methods.-const handleGoogleLogin = () => { +const handleLogin = () => { // Redirect to the login page window.location.href = '/login'; };
174-180: Update function reference.The button correctly uses the generic
IconUsericon but still calls the outdated function name.<Button - onClick={handleGoogleLogin} + onClick={handleLogin} variant="outline" className="hidden cursor-pointer md:flex items-center gap-2 rounded-full dark:bg-white/20 dark:hover:bg-white/30 dark:text-white bg-gray-100 hover:bg-gray-200 text-gray-800 border-0" > <IconUser className="h-4 w-4" /> <span>Sign in</span> </Button>
192-195: Rename function for clarity.Similar to the desktop navigation, this function name should be updated to reflect its generic login purpose.
-const handleGoogleLogin = () => { +const handleLogin = () => { // Redirect to the login page window.location.href = "./login"; };
277-283: Update function reference.The button correctly uses the generic
IconUsericon but still calls the outdated function name.<Button - onClick={handleGoogleLogin} + onClick={handleLogin} variant="outline" className="flex cursor-pointer items-center gap-2 mt-4 w-full justify-center rounded-full dark:bg-white/20 dark:hover:bg-white/30 dark:text-white bg-gray-100 hover:bg-gray-200 text-gray-800 border-0" > <IconUser className="h-4 w-4" /> <span>Sign in</span> </Button>surfsense_web/app/dashboard/page.tsx (1)
204-216: Good UI integration for logout.The logout button is well-integrated into the existing UI, maintaining design consistency with the theme toggler. Consider adding a tooltip for better user experience.
<Button variant="ghost" size="icon" onClick={handleLogout} className="h-9 w-9 rounded-full" aria-label="Logout" + title="Logout" > <LogOut className="h-5 w-5" /> </Button>surfsense_web/app/login/GoogleLoginButton.tsx (1)
8-92: Consider renaming the component.Since this PR introduces support for multiple authentication methods, the
GoogleLoginButtonname is now specific to just one authentication type. Consider renaming this component to better reflect its role in the flexible authentication system.The component still specifically handles Google authentication, but its name could be clarified to better fit within the new authentication framework. Consider renaming it to something like:
-export function GoogleLoginButton() { +export function GoogleOAuthButton() { // Existing implementation }Additionally, if this component is conditionally rendered based on the auth type, consider adding a comment indicating this relationship:
// This component is only used when AUTH_TYPE is set to "GOOGLE" export function GoogleOAuthButton() { // ... }surfsense_backend/.env.example (1)
6-10: Consider adding password policy configurationWhile implementing local authentication, it would be helpful to include environment variables for password policy configuration (min length, complexity requirements, etc.) to allow administrators to customize security requirements.
surfsense_web/app/login/page.tsx (1)
17-26: Consider adding error handling for environment variable accessThe code reads the environment variable without any fallback logic if it's undefined or has an invalid value other than "GOOGLE" or "LOCAL".
- setAuthType(process.env.NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPE || "GOOGLE"); + const configuredAuthType = process.env.NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPE; + if (configuredAuthType && ["GOOGLE", "LOCAL"].includes(configuredAuthType)) { + setAuthType(configuredAuthType); + } else { + console.warn(`Invalid auth type: ${configuredAuthType}, defaulting to "GOOGLE"`); + setAuthType("GOOGLE"); + }surfsense_web/app/login/LocalLoginForm.tsx (3)
14-17: Default authentication type should match component purpose.Setting the default authentication type to "GOOGLE" in a local login form is counterintuitive. Since this component specifically handles local authentication, consider defaulting to "LOCAL" or handling the case where authentication type doesn't match the component's purpose.
- setAuthType(process.env.NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPE || "GOOGLE"); + setAuthType(process.env.NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPE || "LOCAL");
44-46: Enhance error message extraction.The current error extraction only handles errors with a "detail" field. Consider more robust error handling to account for different error response formats.
- throw new Error(data.detail || "Failed to login"); + throw new Error( + data.detail || + (typeof data.message === 'string' ? data.message : null) || + (Array.isArray(data.errors) ? data.errors[0]?.message : null) || + "Failed to login" + );
65-77: Input field id doesn't match label.The email input's id "email" doesn't match with the username state variable it's updating. This could lead to confusion in maintenance and accessibility issues.
- <label htmlFor="email" className="block text-sm font-medium text-gray-700 dark:text-gray-300"> + <label htmlFor="username" className="block text-sm font-medium text-gray-700 dark:text-gray-300"> Email </label> <input - id="email" + id="username" type="email" required value={username} onChange={(e) => setUsername(e.target.value)} className="mt-1 block w-full rounded-md border border-gray-300 bg-white px-3 py-2 shadow-sm focus:border-blue-500 focus:outline-none focus:ring-blue-500 dark:border-gray-700 dark:bg-gray-800 dark:text-white" />surfsense_backend/app/users.py (1)
25-31: Consider lazy loading for conditional imports.Conditional imports at module level can make testing difficult. Consider lazy loading through a factory pattern or moving the imports inside functions where they're used.
-if config.AUTH_TYPE == "GOOGLE": - from httpx_oauth.clients.google import GoogleOAuth2 - - google_oauth_client = GoogleOAuth2( - config.GOOGLE_OAUTH_CLIENT_ID, - config.GOOGLE_OAUTH_CLIENT_SECRET, - ) +# Define a function to get the OAuth client only when needed +def get_oauth_client(): + if config.AUTH_TYPE == "GOOGLE": + from httpx_oauth.clients.google import GoogleOAuth2 + return GoogleOAuth2( + config.GOOGLE_OAUTH_CLIENT_ID, + config.GOOGLE_OAUTH_CLIENT_SECRET, + ) + return None + +# The client can be accessed when needed: oauth_client = get_oauth_client()surfsense_backend/app/db.py (2)
157-167: Reduce code duplication in User model definitions.The two User model definitions share common relationship declarations, leading to code duplication. Consider refactoring to use a single User class with conditional relationship inclusion.
-if config.AUTH_TYPE == "GOOGLE": - class OAuthAccount(SQLAlchemyBaseOAuthAccountTableUUID, Base): - pass - - - class User(SQLAlchemyBaseUserTableUUID, Base): - oauth_accounts: Mapped[list[OAuthAccount]] = relationship( - "OAuthAccount", lazy="joined" - ) - search_spaces = relationship("SearchSpace", back_populates="user") - search_source_connectors = relationship("SearchSourceConnector", back_populates="user") -else: - class User(SQLAlchemyBaseUserTableUUID, Base): - - search_spaces = relationship("SearchSpace", back_populates="user") - search_source_connectors = relationship("SearchSourceConnector", back_populates="user") +# Always define OAuthAccount, but it might not be used +class OAuthAccount(SQLAlchemyBaseOAuthAccountTableUUID, Base): + pass + + +class User(SQLAlchemyBaseUserTableUUID, Base): + if config.AUTH_TYPE == "GOOGLE": + oauth_accounts: Mapped[list[OAuthAccount]] = relationship( + "OAuthAccount", lazy="joined" + ) + + search_spaces = relationship("SearchSpace", back_populates="user") + search_source_connectors = relationship("SearchSourceConnector", back_populates="user")
196-201: Address static analysis warning for function call in argument defaults.The
Dependscall in argument defaults violates best practices for Python function definitions. Move the function call inside the function body.-if config.AUTH_TYPE == "GOOGLE": - async def get_user_db(session: AsyncSession = Depends(get_async_session)): - yield SQLAlchemyUserDatabase(session, User, OAuthAccount) -else: - async def get_user_db(session: AsyncSession = Depends(get_async_session)): - yield SQLAlchemyUserDatabase(session, User) +async def get_user_db(): + session = await anext(get_async_session()) + try: + if config.AUTH_TYPE == "GOOGLE": + yield SQLAlchemyUserDatabase(session, User, OAuthAccount) + else: + yield SQLAlchemyUserDatabase(session, User) + finally: + await session.close()🧰 Tools
🪛 Ruff (0.11.9)
197-197: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
200-200: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
surfsense_web/content/docs/manual-installation.mdx (1)
184-184: Improve authentication variable documentation.The documentation for
NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPEcould be clearer about the consequences of each authentication choice for the user experience and setup requirements.-| NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPE | Same value as set in backend AUTH_TYPE i.e `GOOGLE` for OAuth with Google, `LOCAL` for email/password authentication | +| NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPE | Must match backend AUTH_TYPE: `GOOGLE` enables Google OAuth login (requires Google Cloud setup), `LOCAL` enables email/password login with registration functionality |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.md(0 hunks)surfsense_backend/.env.example(1 hunks)surfsense_backend/app/app.py(2 hunks)surfsense_backend/app/config/__init__.py(1 hunks)surfsense_backend/app/db.py(4 hunks)surfsense_backend/app/users.py(3 hunks)surfsense_web/.env.example(1 hunks)surfsense_web/app/dashboard/page.tsx(3 hunks)surfsense_web/app/login/AmbientBackground.tsx(1 hunks)surfsense_web/app/login/GoogleLoginButton.tsx(2 hunks)surfsense_web/app/login/LocalLoginForm.tsx(1 hunks)surfsense_web/app/login/page.tsx(1 hunks)surfsense_web/app/register/page.tsx(1 hunks)surfsense_web/components/Navbar.tsx(8 hunks)surfsense_web/content/docs/docker-installation.mdx(3 hunks)surfsense_web/content/docs/index.mdx(1 hunks)surfsense_web/content/docs/manual-installation.mdx(2 hunks)
💤 Files with no reviewable changes (1)
- README.md
🧰 Additional context used
🧬 Code Graph Analysis (2)
surfsense_web/components/Navbar.tsx (1)
surfsense_web/components/Logo.tsx (1)
Logo(7-21)
surfsense_web/app/dashboard/page.tsx (2)
surfsense_web/hooks/use-search-spaces.ts (1)
useSearchSpaces(13-75)surfsense_web/components/theme/theme-toggle.tsx (1)
ThemeTogglerComponent(8-69)
🪛 Ruff (0.11.9)
surfsense_backend/app/db.py
197-197: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
200-200: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (18)
surfsense_web/app/login/AmbientBackground.tsx (1)
1-43: Clean implementation of a reusable ambient background component!This is a well-structured React component that creates a visually appealing gradient background. Extracting this from the original
GoogleLoginButton.tsximproves code modularity and enables reuse across both Google OAuth and local authentication pages.The component correctly uses:
- Client-side rendering directive
- Proper positioning with absolute elements
- Non-interactive design with
pointer-events-none- CSS transformations for visual effects
surfsense_web/.env.example (1)
1-2: Environment variable correctly added for authentication typeThe addition of
NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPEwith clear options (LOCAL or GOOGLE) provides users with the flexibility to choose their authentication method.surfsense_web/content/docs/index.mdx (1)
50-54: Documentation clearly indicates Google OAuth is now optionalThe documentation has been appropriately updated to reflect that Google OAuth is optional. The added clarification about supporting both authentication methods and the introductory line for the Google OAuth setup instructions improve user understanding.
surfsense_backend/app/config/__init__.py (2)
45-50: Good implementation of conditional Google OAuth credential loadingThe code correctly implements conditional loading of Google OAuth credentials based on the
AUTH_TYPEenvironment variable. This approach effectively makes Google authentication optional while maintaining compatibility for users who prefer it.
46-46: Verify authentication type validationThe code retrieves the
AUTH_TYPEenvironment variable but doesn't validate that it contains one of the expected values ("GOOGLE"or"LOCAL").Consider adding validation to ensure
AUTH_TYPEis set to a valid value, or provide a default:- AUTH_TYPE = os.getenv("AUTH_TYPE") + AUTH_TYPE = os.getenv("AUTH_TYPE", "LOCAL") + if AUTH_TYPE not in ["GOOGLE", "LOCAL"]: + raise ValueError(f"Invalid AUTH_TYPE: {AUTH_TYPE}. Must be either 'GOOGLE' or 'LOCAL'")surfsense_backend/app/app.py (2)
65-76: Good implementation of conditional authentication.The conditional import and router setup correctly implements the flexible authentication system, only loading Google OAuth components when explicitly configured. This aligns well with the PR objective of removing the hard dependency on Google Auth.
10-14: Clean dependency management.Removing the direct import of
google_oauth_clientand moving it to the conditional block helps to reduce unnecessary dependencies when using LOCAL authentication mode.surfsense_web/app/dashboard/page.tsx (2)
154-159: Well-implemented logout functionality.The logout function correctly clears authentication state and handles client-side navigation. The check for
typeof window !== 'undefined'is a good practice to prevent issues during server-side rendering.
164-184:Details
❓ Verification inconclusive
Verify authorization token usage pattern.
The logout function removes the token but the same token pattern is used in API calls. Ensure this pattern is consistent across the application for both Google and local authentication.
🏁 Script executed:
#!/bin/bash # Check for consistent token usage pattern across the codebase echo "Checking token usage pattern in fetch requests..." rg -A 2 "localStorage.getItem\('surfsense_bearer_token'\)" --glob "*.tsx" --glob "*.ts"Length of output: 6265
🏁 Script executed:
#!/bin/bash # Check for token removal in logout function rg -A3 "localStorage.removeItem('surfsense_bearer_token')" --glob "*.ts" --glob "*.tsx"Length of output: 95
🏁 Script executed:
#!/bin/bash echo "Searching for where the bearer token is set..." rg -n "localStorage.setItem('surfsense_bearer_token'" --glob "*.ts*" --glob "*.tsx" echo -e "\nSearching for any localStorage.removeItem calls..." rg -n "localStorage.removeItem" --glob "*.ts*" --glob "*.tsx" echo -e "\nSearching for localStorage.clear() calls..." rg -n "localStorage.clear" --glob "*.ts*" --glob "*.tsx"Length of output: 937
#!/bin/bash # Find all uses of localStorage.setItem to verify token storage rg -n "localStorage.setItem" --glob "*.ts*" --glob "*.tsx"
🏁 Script executed:
#!/bin/bash # Find all instances of setting the bearer token rg -n "localStorage.setItem.*'surfsense_bearer_token'" --glob "*.ts*" --glob "*.tsx"Length of output: 92
Ensure consistent token storage and retrieval across all authentication flows
We’ve verified that the bearer token is removed in these locations:
- surfsense_web/lib/api.ts:40
- surfsense_web/app/dashboard/page.tsx:156
- surfsense_web/components/sidebar/nav-user.tsx:49
And that every API call and hook reads the token via
localStorage.getItem('surfsense_bearer_token')(20+ instances). However, we didn’t find anylocalStorage.setItem('surfsense_bearer_token')—please confirm where and how the token is initially stored (local login vs. Google OAuth). Ensure both auth flows use the same key and consider centralizing get/set/remove logic inlib/api.tsrather than duplicating it across components.surfsense_web/app/login/GoogleLoginButton.tsx (1)
6-6: Good component organization.Moving the
AmbientBackgroundto its own file improves code organization and component reusability. This is a good refactoring practice.surfsense_backend/.env.example (1)
4-8: Good organization of auth configuration!The restructuring of environment variables with the new
AUTH_TYPEoption clearly supports the PR's objective of removing hard dependency on Google Auth. The "GOOGLE" or "LOCAL" options provide good flexibility.surfsense_web/app/login/page.tsx (1)
11-67: Well-implemented conditional authentication flowThe login page now elegantly handles both authentication types with a clean conditional approach. The loading state, registration success feedback, and authentication type detection are all implemented correctly.
surfsense_web/content/docs/docker-installation.mdx (3)
85-86: Good documentation of AUTH_TYPE configurationThe description clearly explains the two authentication options available, which aligns well with the PR objective of providing alternatives to Google Auth.
107-112: Well-organized conditional configuration sectionGrouping the Google OAuth credentials in a separate section with a clear conditional header ("if AUTH_TYPE=GOOGLE") makes it obvious that these are optional, which supports the PR's goal.
138-138: Good synchronization between frontend and backend configurationAdding
NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPEto match the backend'sAUTH_TYPEensures consistency across the stack, which is important for the dual authentication system to work properly.surfsense_web/app/register/page.tsx (2)
8-67: Well-structured registration functionalityThe registration page includes all necessary elements: form validation, error handling, loading states, and proper API interaction. The redirect to the login page with the "registered=true" parameter creates a smooth user flow.
16-22: Auth type validation provides good user experienceThe redirection to login when auth type is not LOCAL prevents users from attempting registration when it's not supported, avoiding confusion.
surfsense_web/app/login/LocalLoginForm.tsx (1)
56-113:Details
❓ Verification inconclusive
Add CSRF protection to login form.
The current implementation lacks CSRF protection, which is recommended for authentication forms to prevent cross-site request forgery attacks.
Consider implementing CSRF protection by:
- Adding a CSRF token to the form
- Verifying the token on the server side
🏁 Script executed:
#!/bin/bash # Check if CSRF protection is implemented elsewhere in the codebase # Search for CSRF-related code in the frontend rg -A 2 -B 2 "csrf|CSRF|xsrf|XSRF" --glob="*.{ts,tsx,js,jsx}"Length of output: 63
Verify CSRF protection on the login flow
I didn’t find any CSRF token usage in the frontend (LocalLoginForm.tsx) or elsewhere in the repo. Please confirm whether your authentication API enforces CSRF protection on login requests. If it isn’t implemented yet, consider:
- Exposing a CSRF token endpoint (e.g. GET /csrf-token)
- Adding a hidden
csrfTokenfield to the login form- Submitting that token with the POST /login request
- Verifying the token on the server side
| try { | ||
| const response = await fetch( | ||
| `${process.env.NEXT_PUBLIC_FASTAPI_BACKEND_URL}/auth/register`, | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| email, | ||
| password, | ||
| is_active: true, | ||
| is_superuser: false, | ||
| is_verified: false, | ||
| }), | ||
| } | ||
| ); | ||
|
|
||
| const data = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(data.detail || "Registration failed"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add password strength validation
Currently, the code doesn't validate password strength. Consider adding client-side validation to ensure passwords meet minimum security requirements (length, complexity, etc.).
// Add this after the passwords match check
+ if (password.length < 8) {
+ setError("Password must be at least 8 characters long");
+ return;
+ }
+
+ // Optional: add more complex validation
+ const hasUpperCase = /[A-Z]/.test(password);
+ const hasLowerCase = /[a-z]/.test(password);
+ const hasNumbers = /\d/.test(password);
+ const hasSpecialChar = /[!@#$%^&*(),.?":{}|<>]/.test(password);
+
+ if (!(hasUpperCase && hasLowerCase && hasNumbers && hasSpecialChar)) {
+ setError("Password must include uppercase, lowercase, numbers, and special characters");
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In surfsense_web/app/register/page.tsx between lines 36 and 58, add client-side
password strength validation before making the fetch request. Implement checks
for minimum length and complexity (such as including uppercase, lowercase,
numbers, and special characters). If the password does not meet these criteria,
prevent the fetch call and show an appropriate error message to the user.
| body: JSON.stringify({ | ||
| email, | ||
| password, | ||
| is_active: true, | ||
| is_superuser: false, | ||
| is_verified: false, | ||
| }), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review user properties being set during registration
The registration request sets is_active: true and is_verified: false. Verify that the backend handles these properties correctly, especially ensuring email verification logic works as expected if implemented.
🏁 Script executed:
#!/bin/bash
# Check how user verification is handled in the backend
# Look for user model definition
rg -A 10 "class User" --glob "*.py"
# Check registration endpoint to see how these fields are used
rg -A 20 "def register" --glob "*.py"
rg -A 20 "/auth/register" --glob "*.py"Length of output: 2647
🏁 Script executed:
#!/bin/bash
# Search for FastAPI app instantiation and router inclusion
rg -n "FastAPI\(" --glob "*.py"
rg -n "include_router" --glob "*.py"
# Locate FastAPI Users setup and registration/verification routers
rg -n "FastAPIUsers" --glob "*.py"
rg -n "get_register_router" --glob "*.py"
rg -n "get_verify_router" --glob "*.py"Length of output: 1495
🏁 Script executed:
#!/bin/bash
# Inspect registration and verification router setup
rg -n -A5 "get_register_router" --glob "surfsense_backend/app/app.py"
rg -n -A5 "get_verify_router" --glob "surfsense_backend/app/app.py"Length of output: 852
Remove unnecessary user flag fields in registration request
FastAPI Users’ get_register_router only accepts email and password. Any extra flags (is_active, is_superuser, is_verified) are ignored by Pydantic and default to:
is_active = Trueis_superuser = Falseis_verified = False
Recommendations:
- In
surfsense_web/app/register/page.tsx(lines 44–50), removeis_active,is_superuser, andis_verifiedfrom the JSON payload. - Ensure your email verification flow is wired up:
- Implement the
on_after_registerhook insurfsense_backend/app/users.pyto send a verification email. - Use the
/auth/verifyrouter (already included inapp.py) in your frontend to confirm the user’s email.
- Implement the
🤖 Prompt for AI Agents
In surfsense_web/app/register/page.tsx around lines 44 to 50, remove the user
flag fields is_active, is_superuser, and is_verified from the registration JSON
payload since FastAPI Users' register router only accepts email and password and
ignores these extra fields. Ensure the backend handles default values for these
flags and implement the on_after_register hook in surfsense_backend/app/users.py
to send verification emails. Also, confirm the frontend uses the /auth/verify
router to complete email verification.
feat: Removed Hard Dependency on Google Auth
feat: Removed Hard Dependency on Google Auth
1 participant
Description
Motivation and Context
FIX #104
API Changes
Types of changes
Testing
Checklist:
Summary by CodeRabbit
New Features
Improvements
Documentation