Skip to content
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

feat(oidc): add generic oidc capabilities #550

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DrummyFloyd
Copy link

@DrummyFloyd DrummyFloyd commented Jan 15, 2025

What kind of change does this PR introduce?

Features

Why was this change needed?

add new oidc features for self hosted stuff

Please link to related issues when possible, and explain WHY you changed things, not WHAT you changed.

Other information:

Related to #344

Checklist:

Put a "X" in the boxes below to indicate you have followed the checklist;

  • I have read the CONTRIBUTING guide.
  • I checked that there were not similar issues or PRs already open for this.
  • This PR fixes just ONE issue (do not include multiple issues or types of change in the same PR) For example, don't try and fix a UI issue and include new dependencies in the same PR.

TODO:

  • test on my infra can be tested with following image drummyfloyd/postiz:gh-550 for authentik provider only
  • make use of a genric OIDC instead => drummyfloyd/postiz:gh-550-generic
  • add documenation

Summary by CodeRabbit

  • New Features

    • Added support for a generic OAuth authentication provider.
    • Introduced new properties in the app layout for OAuth configuration.
    • Enhanced login flow to support an additional OAuth authentication method.
    • Added a new component for initiating OAuth login.
  • Configuration

    • Updated environment configuration to support the generic OAuth provider.
    • Added new OAuth-related environment variables for configuration.
  • Authentication

    • Expanded authentication providers to include a new generic OAuth option.
    • Implemented a flexible OAuth authentication mechanism.

Copy link

vercel bot commented Jan 15, 2025

@DrummyFloyd is attempting to deploy a commit to the Listinai Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

coderabbitai bot commented Jan 15, 2025

Walkthrough

This pull request introduces support for a generic OAuth authentication provider across the application. The changes span multiple files, adding a new GENERIC provider type to the Prisma schema, creating an OauthProvider class for backend and frontend authentication, and updating environment configurations. The implementation allows for dynamic OAuth authentication by introducing new context variables and middleware logic to handle the generic OAuth flow.

Changes

File Change Summary
libraries/nestjs-libraries/src/database/prisma/schema.prisma Added GENERIC to Provider enum
libraries/react-shared-libraries/src/helpers/variable.context.tsx Added genericOauth, oauthLogoUrl, and oauthDisplayName to context interface
apps/backend/src/services/auth/providers/providers.factory.ts Added support for instantiating OauthProvider for Provider.GENERIC
apps/frontend/src/app/layout.tsx Added new OAuth-related properties to VariableContextComponent
apps/frontend/src/components/auth/login.tsx Updated to include OauthProvider in authentication options
.env.example Added multiple OAuth-related environment variables for configuration
apps/backend/src/services/auth/providers/oauth.provider.ts Created new OauthProvider class with OAuth authentication methods
apps/frontend/src/components/auth/providers/oauth.provider.tsx Added new OauthProvider React component for OAuth login
apps/frontend/src/middleware.ts Modified provider selection logic to support generic OAuth

Sequence Diagram

sequenceDiagram
    participant User
    participant Frontend
    participant Backend
    participant OAuthProvider

    User->>Frontend: Initiate Login
    Frontend->>Backend: Request OAuth Login Link
    Backend->>OAuthProvider: Generate Authorization URL
    OAuthProvider-->>Backend: Return Authorization URL
    Backend-->>Frontend: Provide Login Redirect Link
    Frontend->>OAuthProvider: Redirect to Authorization
    OAuthProvider->>User: Request Authorization
    User->>OAuthProvider: Approve Access
    OAuthProvider-->>Backend: Return Authorization Code
    Backend->>OAuthProvider: Exchange Code for Token
    OAuthProvider-->>Backend: Provide Access Token
    Backend->>OAuthProvider: Fetch User Info
    OAuthProvider-->>Backend: Return User Details
    Backend-->>Frontend: Authenticate User
Loading

Possibly related PRs

  • Not secured #574: The changes in the main PR, which introduce the OauthProvider class and modify the loadProvider method, are related as both involve the implementation and handling of OAuth providers, specifically the new OauthProvider functionality.
  • Add instagram as a standalone without a business #522: The changes in the main PR, which involve the addition of the OauthProvider class and its integration into the provider loading mechanism, are related to the retrieved PR that introduces the InstagramStandaloneProvider, as both involve the implementation of new provider classes for handling different authentication mechanisms.
  • Farcaster login #563: The changes in the main PR are related to those in the retrieved PR as both involve the addition of new provider classes (OauthProvider and FarcasterProvider) that implement the ProvidersInterface and are integrated into the provider loading mechanism.

Poem

🐰 OAuth's new dance, a generic delight,
Providers spinning with technological might!
From Authentik's realm, a login so bright,
Connecting users with magical flight.
A rabbit's code hops, with security's grace! 🔐

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@DrummyFloyd DrummyFloyd marked this pull request as draft January 15, 2025 14:43
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (5)
apps/frontend/src/components/auth/providers/authentik.provider.tsx (2)

8-11: Include dependencies in the useCallback dependency array

The fetch function is used within gotoLogin but is not included in the dependency array. Even if fetch is stable, it's good practice to include it to ensure consistency.

-  }, []);
+  }, [fetch]);

14-27: Improve accessibility by using a button element

Currently, a div is used as a clickable element, which may not be accessible to all users (e.g., those using screen readers). Using a button element enhances accessibility and semantics.

Replace the div with a button:

- <div
+ <button
     onClick={gotoLogin}
     className={`cursor-pointer bg-white h-[44px] rounded-[4px] flex justify-center items-center text-customColor16 ${interClass} gap-[4px]`}
- >
+ >
     <div>
       <Image
         src="/icons/authentik.svg"
         alt="Authentik"
         width={40}
         height={40}
       />
     </div>
     <div>Sign in with Authentik</div>
- </div>
+ </button>
apps/frontend/src/components/auth/login.tsx (3)

66-66: Remove debugging console.log statement.

Production code should not contain console.log statements.

-        {console.log('authentikSso', authentikSso)}

67-74: Simplify provider rendering logic.

The current nested ternary structure could be simplified for better readability.

-        {isGeneral && authentikSso ? (
-          <AuthentikProvider />
-        ) : !isGeneral ? (
-          <GithubProvider />
-        ) : (
-          <GoogleProvider />
-        )}
-        {/* {!isGeneral ? <GithubProvider /> : <GoogleProvider />} */}
+        {(() => {
+          if (isGeneral && authentikSso) return <AuthentikProvider />;
+          if (!isGeneral) return <GithubProvider />;
+          return <GoogleProvider />;
+        })()}

113-113: Fix typo in sign-up text.

The sign-up link text contains an unusual double colon.

-              Sign Up::
+              Sign Up
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 230e8ff and 855253b.

⛔ Files ignored due to path filters (1)
  • apps/frontend/public/icons/authentik.svg is excluded by !**/*.svg
📒 Files selected for processing (7)
  • apps/backend/src/services/auth/providers/authentik.provider.ts (1 hunks)
  • apps/backend/src/services/auth/providers/providers.factory.ts (2 hunks)
  • apps/frontend/src/app/layout.tsx (1 hunks)
  • apps/frontend/src/components/auth/login.tsx (4 hunks)
  • apps/frontend/src/components/auth/providers/authentik.provider.tsx (1 hunks)
  • libraries/nestjs-libraries/src/database/prisma/schema.prisma (2 hunks)
  • libraries/react-shared-libraries/src/helpers/variable.context.tsx (3 hunks)
🔇 Additional comments (5)
apps/backend/src/services/auth/providers/providers.factory.ts (1)

14-15: Verify that Provider.AUTHENTIK is defined in the Provider enum

In the loadProvider method, the case for Provider.AUTHENTIK assumes that AUTHENTIK is a valid member of the Provider enum from @prisma/client. Ensure that AUTHENTIK has been added to the enum to prevent runtime errors.

Run the following script to confirm that AUTHENTIK exists in the Provider enum:

✅ Verification successful

AUTHENTIK is properly defined in the Provider enum

The AUTHENTIK value is correctly defined in the Provider enum within the Prisma schema at libraries/nestjs-libraries/src/database/prisma/schema.prisma. The code is safe to use and won't cause runtime errors.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `AUTHENTIK` is defined in the `Provider` enum.

# Test: Search for the enum definition. Expect: The enum includes `AUTHENTIK`.
rg 'enum Provider \{[^\}]*AUTHENTIK' ./node_modules/@prisma/client/index.d.ts

Length of output: 160


Script:

#!/bin/bash
# Search for Provider enum definition in schema.prisma and source files
echo "=== Searching in schema.prisma ==="
fd "schema.prisma" --exec grep -A 10 "enum Provider" {}

echo -e "\n=== Searching for Provider enum definitions across the codebase ==="
rg "enum\s+Provider\s*\{" -A 10

echo -e "\n=== Looking for Provider imports ==="
rg "import.*Provider.*from"

Length of output: 24049

libraries/react-shared-libraries/src/helpers/variable.context.tsx (2)

8-8: LGTM! Interface updates look good.

The addition of authentikOIDC and the syntax fix for storageProvider are well-structured.

Also applies to: 11-11


21-21: LGTM! Default context value is properly initialized.

The authentikOIDC is correctly defaulted to false.

apps/frontend/src/app/layout.tsx (1)

42-42: LGTM! Environment variable integration is correct.

The authentikOIDC prop is properly configured using the environment variable AUTHENTIK_OIDC.

libraries/nestjs-libraries/src/database/prisma/schema.prisma (1)

539-539: LGTM! Provider enum update is correct.

The addition of AUTHENTIK to the Provider enum is consistent with the new authentication feature.

@nevo-david
Copy link
Contributor

Looks pretty good!
Should I check it or is it not ready yet?

@DrummyFloyd
Copy link
Author

Looks pretty good!
Should I check it or is it not ready yet?

On my end I have some internet issues , won't be able to test on my infra, but you can still have a look and check if it for your vision

@cchance27
Copy link

Silly question maybe, but why a specific Authentik integration instead of just standard OIDC, so that it would work with authentik or any generic oidc backend?

@DrummyFloyd
Copy link
Author

Silly question maybe, but why a specific Authentik integration instead of just standard OIDC, so that it would work with authentik or any generic oidc backend?

tbh, dunno if @nevo-david want somthing more generic, if it's ok with i may cahnge to something more generic at this point, (i've finally internet again)

and it started with authentik , because it was one of the proposed IDP in the related discussion

@cchance27
Copy link

I just sorta feel like generalizing would work better long term as doing one-offs just leads to more work down the line and more requests meanwhile general oidc and a readme for say authentik solves both issues I’d imagine.

Would head off the requests for authelia, pocketid and others that are getting popular lately, at least that’s how I’d imagine it going longterm

@DrummyFloyd
Copy link
Author

I just sorta feel like generalizing would work better long term as doing one-offs just leads to more work down the line and more requests meanwhile general oidc and a readme for say authentik solves both issues I’d imagine.

Would head off the requests for authelia, pocketid and others that are getting popular lately, at least that’s how I’d imagine it going longterm

i'm fully agree, let's wait and see the awser from the maintener

@egelhaus
Copy link
Collaborator

Nice work!

@DrummyFloyd
Copy link
Author

FYI it's under testing with Authentik only, will provide a commit for a generic OIDC after ( like this is the maintainer do not want this i can rever easily)

@DrummyFloyd
Copy link
Author

seems to work as expected
image

will clean a bit , then make another commit for the generic OIDC

@DrummyFloyd DrummyFloyd changed the title feat(oidc): add authentik as oidc feat(oidc): add generic oidc capabilities Jan 31, 2025
@DrummyFloyd DrummyFloyd marked this pull request as ready for review January 31, 2025 16:29
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (6)
apps/backend/src/services/auth/providers/oauth.provider.ts (3)

3-3: Consider renaming class to OAuthProvider for consistency.

Renaming the class to OAuthProvider (capital "A") can make it more recognizable and align with general naming conventions, improving readability.


23-40: Centralize environment variable validation to reduce repetition.

While each missing environment variable triggers a descriptive error message, you could centralize validation to reduce repetitive checks. For example, create a helper function that accepts an array of environment variable keys and processes them in a single pass, throwing errors if any are missing. This can streamline code and improve maintainability.


59-60: Avoid potential double slashes in API endpoints.

Appending /${params.toString()} or using ${this.tokenUrl}/ may introduce unintentional double slashes if the environment variables already contain a trailing slash. Consider trimming any trailing slash from the environment variable or removing the hardcoded slash.

Apply this diff in the constructor to strip trailing slashes:

-    this.authUrl = POSTIZ_OAUTH_AUTH_URL;
-    this.baseUrl = POSTIZ_OAUTH_URL;
-    this.tokenUrl = POSTIZ_OAUTH_TOKEN_URL;
-    this.userInfoUrl = POSTIZ_OAUTH_USERINFO_URL;
+    const stripTrailing = (url: string) => url.replace(/\/+$/, '');
+    this.authUrl = stripTrailing(POSTIZ_OAUTH_AUTH_URL);
+    this.baseUrl = stripTrailing(POSTIZ_OAUTH_URL);
+    this.tokenUrl = stripTrailing(POSTIZ_OAUTH_TOKEN_URL);
+    this.userInfoUrl = stripTrailing(POSTIZ_OAUTH_USERINFO_URL);

Also applies to: 63-64, 88-89

apps/frontend/src/components/auth/providers/oauth.provider.tsx (1)

7-24: Synchronize your callback dependencies.

gotoLogin is declared with useCallback but doesn’t list fetch in its dependency array. Though it may be intentional, you risk stale references if fetch logic changes. If you want to ensure the memoized function is updated when fetch changes, include fetch in the dependency array.

apps/frontend/src/middleware.ts (1)

47-49: Validate branching logic for provider detection.

This conditional assigns 'generic' only when POSTIZ_GENERIC_OAUTH is truthy and 'settings' is in the URL. Otherwise, 'github' is set. If you anticipate multiple OAuth providers, you may want a more flexible approach that doesn’t rely solely on 'settings'.

.env.example (1)

92-101: Add documentation for OAuth configuration.

The OAuth-related environment variables lack documentation explaining their purpose and requirements. Consider adding comments to guide users.

+# === OAuth/OIDC Configuration
+# Display name and logo shown on the login page
 NEXT_PUBLIC_POSTIZ_OAUTH_DISPLAY_NAME="OAuth Provider"
 NEXT_PUBLIC_POSTIZ_OAUTH_LOGO_URL=""
+
+# Enable/disable generic OAuth support
 POSTIZ_GENERIC_OAUTH="false"
+
+# Base URL of your OAuth provider
 POSTIZ_OAUTH_URL="https://auth.example.com"
+
+# Standard OAuth 2.0/OIDC endpoints
 POSTIZ_OAUTH_AUTH_URL="${POSTIZ_OAUTH_URL}/oauth2/authorize"
 POSTIZ_OAUTH_TOKEN_URL="${POSTIZ_OAUTH_URL}/oauth2/token"
 POSTIZ_OAUTH_USERINFO_URL="${POSTIZ_OAUTH_URL}/oauth2/userinfo"
+
+# OAuth client credentials (obtain these from your OAuth provider)
 POSTIZ_OAUTH_CLIENT_ID=""
 POSTIZ_OAUTH_CLIENT_SECRET=""
+
+# Optional: Override default OAuth scopes
 # POSTIZ_OAUTH_SCOPE="openid profile email" # default values
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 855253b and e73c232.

⛔ Files ignored due to path filters (1)
  • apps/frontend/public/icons/generic-oauth.svg is excluded by !**/*.svg
📒 Files selected for processing (9)
  • .env.example (3 hunks)
  • apps/backend/src/services/auth/providers/oauth.provider.ts (1 hunks)
  • apps/backend/src/services/auth/providers/providers.factory.ts (2 hunks)
  • apps/frontend/src/app/layout.tsx (1 hunks)
  • apps/frontend/src/components/auth/login.tsx (3 hunks)
  • apps/frontend/src/components/auth/providers/oauth.provider.tsx (1 hunks)
  • apps/frontend/src/middleware.ts (1 hunks)
  • libraries/nestjs-libraries/src/database/prisma/schema.prisma (2 hunks)
  • libraries/react-shared-libraries/src/helpers/variable.context.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • libraries/nestjs-libraries/src/database/prisma/schema.prisma
  • apps/backend/src/services/auth/providers/providers.factory.ts
  • apps/frontend/src/app/layout.tsx
  • apps/frontend/src/components/auth/login.tsx
  • libraries/react-shared-libraries/src/helpers/variable.context.tsx
🔇 Additional comments (3)
apps/backend/src/services/auth/providers/oauth.provider.ts (1)

87-102: Ensure the userInfo endpoint consistently returns an email field.

Some OIDC or generic OAuth providers may return different user field names. Verify that the chosen provider returns a user object with email and sub, or otherwise handle variation.

Use this script to confirm if any references to user fields beyond email and sub exist in the codebase:

apps/frontend/src/components/auth/providers/oauth.provider.tsx (1)

33-33: Check existence of fallback logo.

Verify that '/icons/generic-oauth.svg' actually exists to avoid runtime 404 errors in production.

✅ Verification successful

Fallback OAuth logo exists and is properly referenced

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for any generic-oauth.svg file in the project.
fd generic-oauth.svg

Length of output: 67

.env.example (1)

92-101: Consider security implications of public variables.

The NEXT_PUBLIC_ prefix exposes these variables to the client-side. While the display name and logo URL are safe to expose, ensure no sensitive information is added to these public variables in future updates.

✅ Verification successful

Current NEXT_PUBLIC_ variables are appropriately public, maintain this security practice

All existing client-side environment variables contain only public information like URLs, display text, and public analytics IDs. Continue following this pattern by ensuring future NEXT_PUBLIC_ variables never contain sensitive data like API secrets, tokens, or credentials.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potentially sensitive information in public environment variables
# Test: Search for sensitive patterns in NEXT_PUBLIC_ variables

echo "Checking for potentially sensitive patterns in NEXT_PUBLIC_ variables..."
rg -i '(secret|key|token|password|auth|oauth|credentials?).*NEXT_PUBLIC' --type env

Length of output: 268


Script:

#!/bin/bash
# Search for all NEXT_PUBLIC environment variables
echo "=== Searching for NEXT_PUBLIC environment variables ==="
rg "NEXT_PUBLIC_" -A 1

# Search specifically for sensitive patterns in NEXT_PUBLIC variables
echo -e "\n=== Checking for sensitive patterns in NEXT_PUBLIC variables ==="
rg "NEXT_PUBLIC_.*(SECRET|KEY|TOKEN|PASSWORD|CREDENTIAL)"

# Search for environment variable definitions in code
echo -e "\n=== Checking for NEXT_PUBLIC environment usage in code ==="
rg "process\.env\.NEXT_PUBLIC_" -A 1

Length of output: 7297

@DrummyFloyd
Copy link
Author

@nevo-david you can

Looks pretty good! Should I check it or is it not ready yet?

You can have a look now =)

@cchance27
Copy link

Those coderabbit recommendations actually look pretty good i've gotta play with that some on my own projects.

@DrummyFloyd
Copy link
Author

Those coderabbit recommendations actually look pretty good i've gotta play with that some on my own projects.

will just keep the nitpick about trailling slash , but i think i'll will keep all the example related to Auhtentik, because everything started we a discussion for Authentik seems legit to leave Authetnik example in .env.example will maybe just comment them

But yeah, will definitely add to one of my oSS project , once is migrated from Gitlab =)

@eaglesemanation
Copy link

This might need to be a separate PR, but could this resolve #392 as well?

@DrummyFloyd
Copy link
Author

@nevo-david can you please have a look ?

@egelhaus egelhaus requested a review from nevo-david February 4, 2025 09:01
chore(env): missing var in .env.example

chore: typo

chore: add reviewed stuff

fix: rebased stuff

fix(authentik-oidc): redirect corect oidc provider
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (1)
apps/frontend/src/components/auth/login.tsx (1)

66-75: Add loading and error states for OAuth provider.

The OAuth provider should handle loading and error states consistently with other providers.

Update the conditional rendering to include error handling:

 {isGeneral && genericOauth ? (
-  <OauthProvider />
+  <OauthProvider
+    onError={(error) => {
+      form.setError('email', {
+        message: error.message,
+      });
+      setLoading(false);
+    }}
+    onLoading={setLoading}
+  />
 ) : !isGeneral ? (
   <GithubProvider />
 ) : (
   <div className="gap-[5px] flex flex-col">
     <GoogleProvider />
     {!!neynarClientId && <FarcasterProvider />}
   </div>
 )}
🧹 Nitpick comments (2)
apps/backend/src/services/auth/providers/oauth.provider.ts (2)

62-85: Add retry logic and type safety to token exchange.

Consider these improvements:

  1. Add retry logic for transient network failures
  2. Define token response type for better type safety

Here's how to implement these improvements:

+interface TokenResponse {
+  access_token: string;
+  token_type: string;
+  expires_in?: number;
+  refresh_token?: string;
+  scope?: string;
+}
+
+private async fetchWithRetry(
+  url: string,
+  options: RequestInit,
+  retries = 3
+): Promise<Response> {
+  for (let i = 0; i < retries; i++) {
+    try {
+      const response = await fetch(url, options);
+      if (response.ok || response.status >= 400) return response;
+    } catch (error) {
+      if (i === retries - 1) throw error;
+      await new Promise(resolve => setTimeout(resolve, 1000 * Math.pow(2, i)));
+    }
+  }
+  throw new Error('Max retries reached');
+}
+
-async getToken(code: string): Promise<string> {
+async getToken(code: string): Promise<TokenResponse> {
-  const response = await fetch(`${this.tokenUrl}/`, {
+  const response = await this.fetchWithRetry(`${this.tokenUrl}/`, {
     method: 'POST',
     headers: {
       'Content-Type': 'application/x-www-form-urlencoded',
       Accept: 'application/json',
     },
     body: new URLSearchParams({
       grant_type: 'authorization_code',
       client_id: this.clientId,
       client_secret: this.clientSecret,
       code,
       redirect_uri: `${this.frontendUrl}/settings`,
     }),
   });

   if (!response.ok) {
     const error = await response.text();
     throw new Error(`Token request failed: ${error}`);
   }

-  const { access_token } = await response.json();
-  return access_token;
+  const tokenResponse: TokenResponse = await response.json();
+  return tokenResponse;
 }

87-102: Add type safety and field validation to user info retrieval.

Consider these improvements:

  1. Define user info response type
  2. Validate required fields

Here's how to implement these improvements:

+interface UserInfoResponse {
+  sub: string;
+  email: string;
+  email_verified?: boolean;
+  name?: string;
+  preferred_username?: string;
+  [key: string]: unknown;
+}
+
 async getUser(access_token: string): Promise<{ email: string; id: string }> {
   const response = await fetch(`${this.userInfoUrl}/`, {
     headers: {
       Authorization: `Bearer ${access_token}`,
       Accept: 'application/json',
     },
   });

   if (!response.ok) {
     const error = await response.text();
     throw new Error(`User info request failed: ${error}`);
   }

-  const { email, sub: id } = await response.json();
+  const userInfo: UserInfoResponse = await response.json();
+  
+  if (!userInfo.sub || !userInfo.email) {
+    throw new Error('Required fields missing in user info response');
+  }
+  
+  if (userInfo.email_verified === false) {
+    throw new Error('Email not verified');
+  }
+  
+  const { email, sub: id } = userInfo;
   return { email, id };
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66b9502 and 1a13d46.

⛔ Files ignored due to path filters (1)
  • apps/frontend/public/icons/generic-oauth.svg is excluded by !**/*.svg
📒 Files selected for processing (9)
  • .env.example (3 hunks)
  • apps/backend/src/services/auth/providers/oauth.provider.ts (1 hunks)
  • apps/backend/src/services/auth/providers/providers.factory.ts (2 hunks)
  • apps/frontend/src/app/layout.tsx (1 hunks)
  • apps/frontend/src/components/auth/login.tsx (3 hunks)
  • apps/frontend/src/components/auth/providers/oauth.provider.tsx (1 hunks)
  • apps/frontend/src/middleware.ts (1 hunks)
  • libraries/nestjs-libraries/src/database/prisma/schema.prisma (2 hunks)
  • libraries/react-shared-libraries/src/helpers/variable.context.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • apps/backend/src/services/auth/providers/providers.factory.ts
  • libraries/nestjs-libraries/src/database/prisma/schema.prisma
  • apps/frontend/src/components/auth/providers/oauth.provider.tsx
  • apps/frontend/src/middleware.ts
  • apps/frontend/src/app/layout.tsx
  • libraries/react-shared-libraries/src/helpers/variable.context.tsx
  • .env.example

Comment on lines +51 to +60
generateLink(): string {
const params = new URLSearchParams({
client_id: this.clientId,
scope: 'openid profile email',
response_type: 'code',
redirect_uri: `${this.frontendUrl}/settings`,
});

return `${this.authUrl}/?${params.toString()}`;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add CSRF protection and PKCE support to OAuth flow.

The current implementation is missing critical security features:

  1. State parameter for CSRF protection
  2. PKCE (Proof Key for Code Exchange) support for enhanced security

Here's how to implement these security features:

+private generateCodeVerifier(): string {
+  const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~';
+  const length = 128;
+  return Array.from(crypto.getRandomValues(new Uint8Array(length)))
+    .map(byte => chars[byte % chars.length])
+    .join('');
+}
+
+private async generateCodeChallenge(verifier: string): Promise<string> {
+  const encoder = new TextEncoder();
+  const data = encoder.encode(verifier);
+  const hash = await crypto.subtle.digest('SHA-256', data);
+  return btoa(String.fromCharCode(...new Uint8Array(hash)))
+    .replace(/\+/g, '-')
+    .replace(/\//g, '_')
+    .replace(/=+$/, '');
+}
+
 generateLink(): string {
+  const state = crypto.randomUUID();
+  const codeVerifier = this.generateCodeVerifier();
+  // Store state and codeVerifier in session or database
+
   const params = new URLSearchParams({
     client_id: this.clientId,
     scope: 'openid profile email',
     response_type: 'code',
     redirect_uri: `${this.frontendUrl}/settings`,
+    state,
+    code_challenge: await this.generateCodeChallenge(codeVerifier),
+    code_challenge_method: 'S256',
   });

   return `${this.authUrl}/?${params.toString()}`;
 }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +23 to +40
if (!POSTIZ_OAUTH_USERINFO_URL)
throw new Error(
'POSTIZ_OAUTH_USERINFO_URL environment variable is not set'
);
if (!POSTIZ_OAUTH_URL)
throw new Error('POSTIZ_OAUTH_URL environment variable is not set');
if (!POSTIZ_OAUTH_TOKEN_URL)
throw new Error('POSTIZ_OAUTH_TOKEN_URL environment variable is not set');
if (!POSTIZ_OAUTH_CLIENT_ID)
throw new Error('POSTIZ_OAUTH_CLIENT_ID environment variable is not set');
if (!POSTIZ_OAUTH_CLIENT_SECRET)
throw new Error(
'POSTIZ_OAUTH_CLIENT_SECRET environment variable is not set'
);
if (!POSTIZ_OAUTH_AUTH_URL)
throw new Error('POSTIZ_OAUTH_AUTH_URL environment variable is not set');
if (!FRONTEND_URL)
throw new Error('FRONTEND_URL environment variable is not set');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add URL format validation for URL-type environment variables.

While the presence of environment variables is checked, their format should also be validated to ensure they are valid URLs.

Add URL validation like this:

 if (!POSTIZ_OAUTH_USERINFO_URL)
   throw new Error(
     'POSTIZ_OAUTH_USERINFO_URL environment variable is not set'
   );
+try {
+  new URL(POSTIZ_OAUTH_USERINFO_URL);
+} catch (e) {
+  throw new Error('POSTIZ_OAUTH_USERINFO_URL must be a valid URL');
+}

@DrummyFloyd
Copy link
Author

@nevo-david any chance to review this soon ? =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants