Rework OAuth2 providers #304
Conversation
- remove linkedin who even uses linkedin?? - add discord and github - consolidate complex account linking logic
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the authentication system by adding support for Discord as an OAuth provider. This includes the addition of a new service mode, Changes
Possibly related PRs
Warning Rate limit exceeded@Southclaws has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (24)
web/src/app/(bare)/error.tsx (1)
12-13: Consider utilizing the reset prop.The
resetprop is accepted but not utilized in the component. This prop is typically used in Next.js error boundaries to allow users to attempt recovery from errors.Consider adding a retry button:
- return <UnreadyBanner error={error} />; + return ( + <div> + <UnreadyBanner error={error} /> + <button onClick={reset}>Retry</button> + </div> + );web/src/lib/auth/oauth.ts (3)
1-3: Consider using native Array.filter instead of lodash/fpWhile lodash/fp's filter is good for functional programming, the native Array.filter would be simpler and reduce dependencies for this straightforward use case.
-import { filter } from "lodash/fp"; - import { AuthProvider } from "@/api/openapi-schema";
9-10: Simplify implementation using native Array.filterIf removing lodash/fp dependency, here's a cleaner implementation using native Array.filter:
-export const filterWithLink = (list: AuthProvider[]): OAuthProvider[] => - filter(hasLink)(list); +export const filterWithLink = (list: AuthProvider[]): OAuthProvider[] => + list.filter(hasLink);
12-12: Enhance type safety for OAuth providersConsider extending the type definition to include provider-specific information and ensure type safety across different OAuth providers.
-export type OAuthProvider = AuthProvider & { link: string }; +export type OAuthProviderName = 'discord' | 'github'; +export type OAuthProvider = AuthProvider & { + link: string; + name: OAuthProviderName; + scope?: string[]; +};web/src/app/(bare)/auth/[provider]/callback/layout.tsx (2)
6-6: Consider removing the async keywordThe function is marked as async but doesn't contain any await operations. Since this is a layout component that only renders UI, the async keyword appears unnecessary.
-export default async function Layout({ children }: PropsWithChildren) { +export default function Layout({ children }: PropsWithChildren) {
11-19: Consider enhancing navigation accessibilityThe navigation links implementation looks good, but consider wrapping them in a
navelement with an appropriate ARIA label for better accessibility.- <HStack> + <HStack as="nav" aria-label="Authentication options"> <LinkButton size="xs" variant="ghost" href="/password-reset"> Forgot password </LinkButton> <LinkButton size="xs" variant="subtle" href="/register"> Register </LinkButton> </HStack>app/resources/account/authentication/service.go (1)
14-16: Document migration path for existing LinkedIn usersWhile removing the LinkedIn provider aligns with the PR objectives due to low usage, we should ensure there's a clear migration path for any existing users who might have linked their accounts through LinkedIn.
Consider:
- Adding documentation about handling existing LinkedIn-linked accounts
- Implementing a notification system to inform affected users
- Providing steps for users to link their accounts using alternative providers
web/src/lib/auth/utils.ts (1)
Line range hint
14-24: Consider enhancing provider type safety with discriminated unionsGiven the PR's goal of reworking OAuth providers and consolidating account linking, consider enhancing type safety by:
- Using a discriminated union type for different provider types (password, phone, webauthn, oauth)
- Adding runtime type guards for provider validation
This would make future provider additions/removals more maintainable and type-safe.
Example type enhancement:
type BaseProvider = { provider: string; }; type OAuthProvider = BaseProvider & { kind: 'oauth'; link: string; }; type PasswordProvider = BaseProvider & { kind: 'password'; }; type AuthProvider = OAuthProvider | PasswordProvider | ...;web/src/components/settings/AuthMethodSettings/useAuthMethodSettings.ts (1)
1-1: Consider using native Array methods instead of lodashThe filtering logic could be simplified by using native Array methods instead of lodash's
find:-import { find } from "lodash"; // Remove any OAuth providers that are already active -const availableOAuth = oauth.filter((v) => { - return !find(methods, (m) => m.provider.provider === v.provider); -}); +const availableOAuth = oauth.filter((v) => + !methods.some((m) => m.provider.provider === v.provider) +);This change would:
- Remove an external dependency
- Make the code more concise
- Use more idiomatic JavaScript
Also applies to: 28-30
web/src/api/server.ts (1)
Line range hint
34-37: Consider improving type safety for Orval integration.The type casting workaround (
as T) for Orval's incorrect type generation could mask runtime errors, especially critical for OAuth2 flows.Consider:
- Creating a type guard to validate the response shape
- Adding runtime checks before the cast
- Opening an issue with Orval to fix the type generation
web/src/components/auth/OAuthProviderList.tsx (2)
16-18: Address TODO and improve code commentsThe comment style could be more professional, and the TODO indicates missing functionality.
Would you like me to help create a GitHub issue to track the admin configuration feature implementation?
45-51: Consider accessibility and type safety improvementsThe link button implementation could be enhanced with:
- ARIA labels for better screen reader support
- Visual indication of the OAuth provider (e.g., icons)
function OAuthProviderLink({ provider }: { provider: OAuthProvider }) { return ( <LinkButton size="sm" variant="outline" w="full" href={provider.link} + aria-label={`Sign in with ${provider.name}`} > + {provider.icon && <Icon name={provider.icon} aria-hidden="true" />} {provider.name} </LinkButton> ); }web/src/components/settings/AuthMethodSettings/OAuth/OAuth.tsx (1)
30-51: Consider improving identifier display formatThe active accounts section has been greatly improved with better visual hierarchy and additional context. However, the identifier display could be enhanced.
Consider this improvement for the identifier display:
- <styled.pre fontSize="sm">id:{v.identifier}</styled.pre> + <styled.code fontSize="sm" color="fg.subtle">ID: {v.identifier}</styled.code>app/services/authentication/service.go (2)
70-70: LGTM: Provider registration looks goodThe Discord provider is correctly added to the providers slice. The debug logging will automatically include it via the existing mapping functions.
Consider adding a test case to verify the Discord provider is correctly included in the provider list:
func TestDiscordProviderRegistration(t *testing.T) { m := New( zap.NewNop(), mockSettings, nil, nil, nil, nil, nil, &discord.Provider{}, nil, ) provider, err := m.Provider(authentication.ServiceOAuthDiscord) assert.NoError(t, err) assert.NotNil(t, provider) }
Line range hint
1-108: Architecture maintains clean separation of concernsThe changes preserve the existing architecture's strengths:
- Clean provider interface abstraction
- Consistent dependency injection pattern
- Extensible provider registration system
This makes future provider additions/removals straightforward and maintainable.
Consider documenting the provider addition/removal process in the README to help future contributors:
- Add/remove provider package
- Update imports
- Modify Build() registration
- Update New() parameters
- Add/remove from providers slice
web/src/api/common.ts (1)
80-82: LGTM! Consider adding documentation.The addition of string type handling is a good improvement, especially in the context of OAuth2 provider integration. This prevents double stringification of data that's already in string format.
Consider adding a comment to document this case:
if (data instanceof File || data instanceof Blob) { return data; } + // Handle pre-formatted string payloads (e.g., OAuth2 token requests) if (typeof data === "string") { return data; }app/services/authentication/provider/oauth/google/google.go (2)
40-42: Add field documentationConsider adding documentation comments for the struct fields to improve code maintainability.
type Provider struct { + // register handles account registration and lookup operations register *register.Registrar + // ed handles encryption/decryption of OAuth state ed endec.EncrypterDecrypter + // callback is the OAuth redirect URL callback string + // config contains OAuth provider configuration config *all.Configuration }
Line range hint
74-89: Consider explicit error handling for missing configurationThe nil check silently returns nil when config is missing. Consider making this more explicit to avoid potential runtime issues.
-func (p *Provider) oauthConfig() *oauth2.Config { +func (p *Provider) oauthConfig() (*oauth2.Config, error) { if p.config == nil { - return nil + return nil, fault.New("oauth configuration is not initialized") } return &oauth2.Config{ ClientID: p.config.ClientID, ClientSecret: p.config.ClientSecret, Endpoint: google.Endpoint, RedirectURL: p.callback, Scopes: []string{ "https://www.googleapis.com/auth/userinfo.email", "https://www.googleapis.com/auth/userinfo.profile", }, - } + }, nil }web/src/app/(bare)/auth/[provider]/callback/page.tsx (2)
21-21: Remove unusedpropsparameter and update component definitionAfter replacing
props.paramsandprops.searchParamswith the hooksuseParamsanduseSearchParams, thepropsparameter is no longer needed. You can remove it from the component definition.Apply this diff:
-export default function Page(props: Props) { +export default function Page() { const router = useRouter();
3-4: Remove unuseduseimport from ReactSince you're no longer using the
usefunction from React, you can remove it from the import statement to clean up the code.Apply this diff:
import { useRouter } from "next/navigation"; -import { use, useEffect, useState } from "react"; +import { useEffect, useState } from "react";app/services/authentication/provider/oauth/discord/discord.go (1)
129-130: Fallback toUsernameifGlobalNameis empty
u.GlobalNamemay be empty for some users. Consider falling back tou.Usernameto ensure thenamevariable is always populated.Suggested change:
handle := strings.ToLower(u.Username) - name := u.GlobalName + name := u.GlobalName + if name == "" { + name = u.Username + }app/services/account/register/register.go (3)
Line range hint
74-74: Fix comparison of value to pointer in onboarding status checkIn the
Createmethod, the comparisonif status == &onboarding.StatusRequiresFirstAccountis incorrect because it compares a value to the address of a value. You should compare the value directly without taking its address.Here's the diff to fix the comparison:
-if status == &onboarding.StatusRequiresFirstAccount { +if status == onboarding.StatusRequiresFirstAccount {
85-216: Consider refactoring to reduce code duplicationThe methods
GetOrCreateViaEmailandGetOrCreateViaHandlehave similar structures and logic, handling account creation and linking based on different identifiers. Consider refactoring common code into shared helper functions to improve maintainability and reduce duplication.
Line range hint
74-78: Ensure test coverage for first-time setup scenarioThe condition handling for first-time setup in the
Createmethod is crucial for setting the first account to admin. Ensure that there are unit tests covering this scenario to verify that the onboarding status is correctly interpreted and that the admin flag is properly set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (23)
app/resources/account/authentication/authentication_enum_gen.go(3 hunks)app/resources/account/authentication/service.go(1 hunks)app/services/account/register/register.go(2 hunks)app/services/authentication/provider/oauth/discord/discord.go(1 hunks)app/services/authentication/provider/oauth/github/github.go(3 hunks)app/services/authentication/provider/oauth/google/google.go(7 hunks)app/services/authentication/provider/oauth/linkedin/linkedin.go(0 hunks)app/services/authentication/service.go(4 hunks)web/src/api/common.ts(1 hunks)web/src/api/server.ts(1 hunks)web/src/app/(bare)/auth/[provider]/callback/layout.tsx(1 hunks)web/src/app/(bare)/auth/[provider]/callback/page.tsx(2 hunks)web/src/app/(bare)/error.tsx(1 hunks)web/src/app/(bare)/login/layout.tsx(2 hunks)web/src/app/(bare)/register/layout.tsx(1 hunks)web/src/components/auth/OAuthProviderList.tsx(1 hunks)web/src/components/settings/AuthMethodSettings/AuthMethodSettings.tsx(1 hunks)web/src/components/settings/AuthMethodSettings/OAuth/OAuth.tsx(1 hunks)web/src/components/settings/AuthMethodSettings/useAuthMethodSettings.ts(2 hunks)web/src/lib/auth/oauth.ts(1 hunks)web/src/lib/auth/utils.ts(1 hunks)web/src/screens/auth/components/AuthMethod/AuthMethod.tsx(0 hunks)web/src/screens/auth/components/AuthSelection/AuthSelection.tsx(0 hunks)
💤 Files with no reviewable changes (3)
- app/services/authentication/provider/oauth/linkedin/linkedin.go
- web/src/screens/auth/components/AuthMethod/AuthMethod.tsx
- web/src/screens/auth/components/AuthSelection/AuthSelection.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/app/(bare)/error.tsx
[error] 5-5: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (29)
web/src/app/(bare)/error.tsx (2)
1-3: LGTM: Proper Next.js client directive and import.
The "use client" directive is correctly placed at the top of the file, and the import statement is clean and focused.
1-13: Verify error handling in OAuth callback.
Since this error component is used in the OAuth callback flow, let's verify its integration.
✅ Verification successful
Error handling in OAuth callback is properly implemented
The verification shows that error handling is correctly implemented in the OAuth flow:
- The callback page (
auth/[provider]/callback/page.tsx) properly handles errors using theerrorstate and renders them using theUnreadycomponent - The email verification page (
auth/verify/email/page.tsx) catches errors and displays them usingUnreadyBanner - The
UnreadyBannercomponent is consistently used across the application for error display, showing it's a well-established pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the error component is integrated in the OAuth callback flow
# Find OAuth callback pages that might use this error component
rg -l "error.tsx|ErrorPage" "web/src/app/(bare)/auth/"
# Check for error handling patterns in OAuth callback
ast-grep --pattern 'catch (error) { $$$}' web/src/app/(bare)/auth/
Length of output: 306
Script:
#!/bin/bash
# Let's try a different approach to verify error handling in OAuth flow
# Find OAuth-related files in the auth directory
fd . web/src/app/\(bare\)/auth/ -t f
# Look for error handling patterns in these files
rg -n "catch|throw|error" web/src/app/\(bare\)/auth/
# Check if UnreadyBanner is used elsewhere
rg -n "UnreadyBanner" web/src/
Length of output: 8290
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
web/src/lib/auth/oauth.ts (1)
1-12: Verify provider configuration for Discord and GitHub
Since the PR objectives mention removing LinkedIn and adding Discord/GitHub providers, let's verify the provider configuration across the codebase.
✅ Verification successful
Let me gather more specific information about the OAuth provider configurations.
Let me check the actual provider implementations to verify the configuration.
Let me check the actual provider implementation in the server code to verify the OAuth configuration.
Based on the code analysis, I can now provide a final response:
OAuth provider implementation properly supports Discord and GitHub
The codebase shows proper implementation for both Discord and GitHub OAuth providers:
- The authentication system includes both providers in its initialization:
github.Newanddiscord.New - The OAuth provider interface is properly defined and implemented
- The generic OAuth handling code in
web/src/lib/auth/oauth.tscorrectly supports any provider with a link property - No references to LinkedIn were found, confirming its removal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for provider configurations and ensure LinkedIn is removed and Discord/GitHub are properly configured
echo "Checking for OAuth provider configurations..."
rg -l "linkedin|discord|github" --type ts
# Check for any remaining LinkedIn OAuth references
echo "Checking for remaining LinkedIn references..."
rg "linkedin" --type ts
# Verify Discord and GitHub provider implementations
echo "Verifying Discord and GitHub implementations..."
rg "discord|github" --type ts
Length of output: 1169
Script:
#!/bin/bash
# Check for OAuth provider configurations in API schema and related files
echo "Checking API schema for provider definitions..."
fd "openapi-schema" --exec rg -A 5 "AuthProvider|OAuthProvider" {}
# Look for provider-specific configuration or setup
echo "Checking for provider setup code..."
rg -A 5 "provider.*=|provider.*type" --type ts
# Search for any OAuth-related configuration files
echo "Looking for OAuth configuration files..."
fd -e ts -e json --exec rg -l "oauth|auth.*provider" {}
Length of output: 7129
Script:
#!/bin/bash
# Check the provider implementation files
echo "Checking provider implementations..."
cat web/src/lib/auth/providers.ts
# Check auth utility functions
echo "Checking auth utilities..."
cat web/src/lib/auth/utils.ts
# Check OpenAPI client implementation for auth
echo "Checking OpenAPI auth client..."
cat web/src/api/openapi-client/auth.ts
Length of output: 36594
Script:
#!/bin/bash
# Search for OAuth provider configuration in server code
echo "Checking server-side OAuth configuration..."
fd -e go --exec rg -A 5 "oauth.*provider|auth.*provider" {}
# Look for provider type definitions
echo "Checking provider type definitions..."
fd -e go --exec rg -A 3 "type.*Provider" {}
Length of output: 15999
web/src/app/(bare)/register/layout.tsx (2)
3-3: LGTM! Clean import organization.
The import is well-organized in a dedicated auth components directory, following good code organization practices.
12-12: Verify props requirements for OAuthProviderList.
Let's verify that OAuthProviderList doesn't require any mandatory props.
✅ Verification successful
OAuthProviderList component doesn't require any props - usage is correct
The component is defined as an async function with no parameters, and it's being correctly used without props in both login and register layouts. The implementation shows it fetches OAuth providers internally and handles the case when no providers are available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of OAuthProviderList to verify prop requirements
# Search for the OAuthProviderList component definition
ast-grep --pattern 'export function OAuthProviderList($_) {
$$$
}'
# Alternative: Look for type/interface definitions
ast-grep --pattern 'type OAuthProviderListProps = {
$$$
}'
# Alternative: Look for interface definitions
ast-grep --pattern 'interface OAuthProviderListProps {
$$$
}'
Length of output: 199
Script:
#!/bin/bash
# Let's try a broader search to locate the OAuthProviderList component
rg -l "OAuthProviderList" --type ts --type tsx
# Then let's look at the content of any files that contain this component
rg "OAuthProviderList" -A 10 --type ts --type tsx
Length of output: 152
Script:
#!/bin/bash
# Search for files containing OAuthProviderList without file type restrictions
fd . -e tsx -e ts | xargs rg "OAuthProviderList" -A 10
Length of output: 3267
web/src/app/(bare)/auth/[provider]/callback/layout.tsx (1)
1-22: LGTM! Clean and well-structured layout component
The implementation is clean, well-organized, and follows React best practices:
- Proper typing with PropsWithChildren
- Consistent use of styled components
- Clear navigation options
web/src/app/(bare)/login/layout.tsx (1)
3-3: LGTM! Import follows project conventions.
The import statement follows the project's conventions and uses the correct path alias.
app/resources/account/authentication/service.go (1)
14-16: Verify complete removal of LinkedIn provider references
The OAuth services have been updated to remove LinkedIn and add Discord. Let's ensure all LinkedIn-related code has been properly cleaned up.
✅ Verification successful
LinkedIn provider has been completely removed
The search results confirm that there are no remaining references to the LinkedIn OAuth provider in the codebase. The only LinkedIn mentions found are in an imported CSV file containing historical Hacker News data, which is unrelated to the authentication system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining LinkedIn-related code that needs cleanup
echo "Searching for LinkedIn-related code..."
rg -i "linkedin" --type go --type typescript --type javascript
echo "Searching for specific OAuth LinkedIn constants..."
rg -i "oauth_linkedin|serviceOAuthLinkedin" --type go --type typescript --type javascript
Length of output: 422
Script:
#!/bin/bash
# Let's try a more focused search without type restrictions
echo "Searching for LinkedIn-related code..."
rg -i "linkedin"
echo "Searching for specific OAuth LinkedIn constants..."
rg -i "oauth_linkedin|serviceOAuthLinkedin"
# Let's also check for any configuration or migration files
echo "Checking configuration files..."
fd -e yaml -e json -e toml -e conf | xargs rg -i "linkedin"
Length of output: 5561
web/src/lib/auth/utils.ts (2)
10-10: LGTM: Clean import of utility function
The import follows TypeScript best practices and maintains good code organization.
18-18: Verify the type safety of filterWithLink implementation
The refactoring to use filterWithLink improves code maintainability. However, let's verify its implementation to ensure type safety with the OAuth providers.
✅ Verification successful
Let me search for the filterWithLink implementation to verify its type safety.
Let me check the implementation of hasLink and the type definitions to fully verify the type safety.
Type safety of filterWithLink is well-implemented
The implementation is type-safe with proper TypeScript type guards:
hasLinkis a type guard that ensures the presence of thelinkpropertyfilterWithLinkcorrectly narrowsAuthProvider[]toOAuthProvider[]using the type guardOAuthProvidertype is properly defined as an intersection with requiredlinkproperty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the filterWithLink implementation and its type safety
# Check the implementation of filterWithLink
ast-grep --pattern 'export function filterWithLink($_) {
$$$
}'
# Check for OAuth provider type definitions
rg -A 5 'type.*Provider.*='
Length of output: 30787
Script:
#!/bin/bash
# Search for filterWithLink implementation
ast-grep --pattern 'function filterWithLink($_) {
$$$
}'
# Also search for any usage of filterWithLink
rg 'filterWithLink' -A 3
Length of output: 1402
Script:
#!/bin/bash
# Search for hasLink implementation and related type definitions
ast-grep --pattern 'const hasLink = $_'
# Search for AuthProvider type definition
ast-grep --pattern 'type AuthProvider = $_'
# Also search for any usage of hasLink
rg 'hasLink' -A 3
Length of output: 690
web/src/components/settings/AuthMethodSettings/useAuthMethodSettings.ts (1)
27-32: LGTM! The filtering and sorting logic is well structured
The sequence of operations is correct:
- Filter out already active OAuth providers
- Sort the remaining available providers by name
This ensures that users only see OAuth providers they haven't yet connected, presented in alphabetical order.
web/src/api/server.ts (2)
18-18: LGTM! Header transformation looks good.
The use of Object.fromEntries(new Headers().entries()) is a clean approach to normalize header objects.
Let's verify that this handles all OAuth2-related headers correctly:
✅ Verification successful
Header transformation is safe and correctly handles OAuth2 headers
The code transformation using Object.fromEntries(new Headers().entries()) is safe and properly preserves all headers, including OAuth2-related ones. The codebase shows consistent header handling across all authentication flows (OAuth2, password, WebAuthn) where headers are properly merged with custom options using the spread operator (...options?.headers).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for OAuth2-related header usage across the codebase
# Look for Authorization, Bearer, OAuth headers to ensure compatibility
rg -i 'authorization|bearer|oauth' --type ts
Length of output: 7342
Script:
#!/bin/bash
# Let's check the actual header usage in the server.ts file and related API files
rg -A 5 'headers' web/src/api/
Length of output: 34067
22-24: Verify type compatibility with OAuth2 providers.
The renaming of body to data and direct header passing suggests an API change. While this looks correct, the comment about incorrect Orval types is concerning.
Let's verify the type compatibility:
web/src/components/settings/AuthMethodSettings/AuthMethodSettings.tsx (1)
35-38: Review the OAuth component rendering logic
The current condition might render the OAuth component even when no OAuth methods are available, which could lead to a confusing empty state in the UI. Consider showing the OAuth section only when there are available OAuth methods to configure.
Let's verify the OAuth component's behavior:
web/src/components/auth/OAuthProviderList.tsx (2)
1-5: LGTM! Imports are well-organized and properly utilized.
All imported components and utilities are used effectively throughout the component.
20-43: LGTM! Component structure and styling
The component layout is well-structured using styled-system components, with clear separation of concerns and semantic HTML usage (ul/li).
web/src/components/settings/AuthMethodSettings/OAuth/OAuth.tsx (2)
17-27: Improved component structure and user guidance
The changes enhance the UI by:
- Using
LStackfor better layout control - Adding clear descriptive text about linked accounts
- Implementing consistent heading styles with subtle colors
6-6: Verify the type change impact across the codebase
The change from AuthProvider[] to OAuthProvider[] aligns with the PR's objective of reworking OAuth providers. However, we should verify this change's impact on other components.
Also applies to: 12-12
✅ Verification successful
Type change from AuthProvider to OAuthProvider is consistent with the codebase
The change aligns with the codebase's architecture where OAuthProvider is a specialized type extending AuthProvider with a link property. This is evidenced by:
- The type definition in
lib/auth/oauth.ts:export type OAuthProvider = AuthProvider & { link: string } - The
filterWithLinkutility function that convertsAuthProvider[]toOAuthProvider[] - Consistent usage in the OAuth component's props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of AuthProvider type in props or OAuth context
rg -t typescript "AuthProvider\[\]"
# Search for components consuming the OAuth component
ast-grep --pattern 'import { OAuth } from "@/components/settings/AuthMethodSettings/OAuth"'
Length of output: 166
Script:
#!/bin/bash
# Search for AuthProvider type usage with correct file extension
rg -t ts -t tsx "AuthProvider\[\]"
# Search for OAuth component imports with correct extension
rg -t ts -t tsx 'import.*OAuth.*from.*AuthMethodSettings'
# Search for OAuthProvider type usage
rg -t ts -t tsx "OAuthProvider\[\]"
# Look for type definitions of AuthProvider and OAuthProvider
rg -t ts -t tsx "type (Auth|OAuth)Provider"
Length of output: 292
Script:
#!/bin/bash
# Search for files with TypeScript extensions first
fd -e ts -e tsx
# Then use these file patterns with ripgrep
rg "AuthProvider\[\]" $(fd -e ts -e tsx)
# Search for OAuth component imports
rg 'import.*OAuth.*from.*AuthMethodSettings' $(fd -e ts -e tsx)
# Search for OAuthProvider type usage
rg "OAuthProvider\[\]" $(fd -e ts -e tsx)
# Look for type definitions
rg "type (Auth|OAuth)Provider" $(fd -e ts -e tsx)
Length of output: 170895
app/services/authentication/service.go (3)
43-43: LGTM: Discord provider correctly registered
The Discord provider is properly registered in the dependency injection container, maintaining consistency with other OAuth providers.
61-61: Verify consistent usage of the Discord provider parameter
The parameter change from LinkedIn to Discord provider looks correct. Let's verify that all references to the provider parameter have been updated consistently.
✅ Verification successful
Parameter change from LinkedIn to Discord provider is consistent
Based on the codebase analysis, there are no remaining references to LinkedIn provider, and the Discord provider is properly integrated in the authentication service. The parameter change from li *linkedin.Provider to dp *discord.Provider appears to be consistently applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to LinkedIn provider parameters
rg -A 3 '\*linkedin\.Provider' --type go
# Search for any missed Discord provider parameter references
rg -A 3 '\*discord\.Provider' --type go
Length of output: 323
Script:
#!/bin/bash
# Search for any references to LinkedIn in the codebase
rg -i "linkedin" --type go
# Search for any references to Discord in function calls or method invocations
ast-grep --pattern 'dp.$$$'
Length of output: 56
Script:
#!/bin/bash
# Search for function signatures and their usage in authentication service
rg -A 5 "func.*Manager" --type go
# Search for provider initialization or usage patterns
rg -B 2 -A 5 "Provider.*{" --type go
# Look for any authentication-related function calls
rg -B 2 "authenticate|login|signin" --type go -i
Length of output: 85383
17-17: Verify complete removal of LinkedIn imports
The Discord provider import has been added correctly. However, let's ensure all LinkedIn-related imports have been properly removed from the codebase.
✅ Verification successful
LinkedIn imports have been successfully removed
The verification process shows that all LinkedIn-related imports and code have been properly removed from the codebase. The search for "linkedin" in the authentication service directory and the broader codebase returned no results, confirming the complete removal of LinkedIn OAuth provider.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining LinkedIn OAuth-related imports
rg -l 'linkedin' --type go
Length of output: 26
Script:
#!/bin/bash
# Search for any remaining LinkedIn-related code with broader patterns
rg -i "linkedin" --type go
# Also check for any OAuth provider imports that might contain LinkedIn
ast-grep --pattern 'import (
$$$
"github.com/Southclaws/storyden/app/services/authentication/provider/oauth/$_"
$$$
)'
Length of output: 102756
Script:
#!/bin/bash
# Search for any remaining LinkedIn-related code in the authentication service
rg -i "linkedin" app/services/authentication/
# Check for any LinkedIn provider-related code
ast-grep --pattern 'type Provider struct {
$$$
linkedin $$$
$$$
}'
Length of output: 120
web/src/api/common.ts (1)
80-82: Verify OAuth2-related usage patterns.
Let's ensure this change aligns with the OAuth2 provider integration requirements.
✅ Verification successful
Let me search for more specific OAuth2-related code and buildRequest usage patterns.
String data handling in buildRequest is safe for OAuth2 integration
The verification shows that:
- The string data handling in
buildRequestis part of a general data serialization process incommon.ts - The OAuth2-related code uses the standard request building mechanism through
buildRequestin both client and server implementations - The string-type check is a safe guard that preserves string payloads as-is, which is compatible with OAuth2 token responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for buildRequest usage in OAuth2-related files
echo "Checking buildRequest usage in OAuth2 context..."
rg -A 5 "buildRequest.*data:" --type ts
# Look for potential OAuth2 token request handling
echo "Checking OAuth2 token-related code..."
rg -l "oauth|token|auth" --type ts
Length of output: 11488
Script:
#!/bin/bash
# Search for OAuth2-related code implementation
echo "Checking OAuth2 implementation..."
ast-grep --pattern 'buildRequest($$$)'
# Search for string data handling in auth-related files
echo "Checking string data handling in auth files..."
rg "data.*string" web/src/api/ web/src/lib/auth/ -A 5
Length of output: 1613
app/services/authentication/provider/oauth/google/google.go (2)
6-6: LGTM: Import additions enhance functionality
The new imports support proper email parsing, error classification, and handle generation.
Also applies to: 13-14
134-152: Verify handle uniqueness and improve error messages
The handle generation using petname is creative but might need additional checks to ensure uniqueness. Also, consider improving error messages for email parsing failures.
- email, err := mail.ParseAddress(u.Email)
- if err != nil {
- return nil, fault.Wrap(err, fctx.With(ctx))
+ email, err := mail.ParseAddress(u.Email)
+ if err != nil {
+ return nil, fault.Wrap(err,
+ fctx.With(ctx),
+ ftag.With(ftag.InvalidArgument),
+ fmsg.WithDesc("invalid email format", "The email address from Google is not valid."))
}✅ Verification successful
Let me gather more information about handle uniqueness validation.
Handle generation and uniqueness are properly handled
The codebase already implements robust handle uniqueness checks:
- The
account_writerenforces uniqueness constraints with clear error messages - The
Registrarservice tracks handle existence and handles collisions - Generated handles are properly validated before account creation
The suggested error message improvement for email parsing is good to have, but the core concern about handle uniqueness is already addressed by the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential handle collisions in existing codebase
rg -g '!*.test.*' -g '!*_test.go' "petname.Generate.*" -A 2
# Check for handle uniqueness validation in the register package
rg -g '*.go' "func.*GetOrCreateViaEmail" ./app/services/account/register -A 10
Length of output: 1734
Script:
#!/bin/bash
# Check handle uniqueness validation in account writer
ast-grep --pattern 'func $_ Create($_, $handle string, $$$) {
$$$
}'
# Check for any handle validation or collision detection
rg -g '*.go' "handle.*exists|duplicate.*handle|unique.*handle" -B 2 -A 2
Length of output: 4389
app/resources/account/authentication/authentication_enum_gen.go (3)
113-114: LGTM! Discord display text is appropriate.
The Format method correctly handles the new Discord service with a clear, concise display text.
161-162: LGTM! String conversion for Discord service is properly implemented.
The NewService function correctly handles the conversion of Discord service strings, maintaining consistency with other OAuth providers.
84-90: LGTM! Service declarations are consistent with PR objectives.
The Service enum declarations correctly include the new Discord OAuth2 provider while removing LinkedIn, aligning with the PR's goal of reworking OAuth2 providers.
Let's verify the removal of LinkedIn references:
✅ Verification successful
LinkedIn OAuth references have been successfully removed
The absence of any LinkedIn-related references in the codebase after our thorough search confirms that the LinkedIn OAuth provider has been properly removed. This aligns with the PR objectives and the changes observed in the authentication enum file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining LinkedIn OAuth references
# that might need cleanup
rg -i "linkedin|ServiceOAuthLinkedin" --type go
Length of output: 49
Script:
#!/bin/bash
# Let's do a broader search to verify OAuth-related changes
# First, let's check for any OAuth-related service definitions
ast-grep --pattern 'ServiceOAuth$_ = Service{$_}'
# Then, let's check OAuth-related constants and configurations
rg -i "oauth.*linkedin"
# Finally, let's check for any OAuth provider configurations
rg -i "provider.*linkedin"
Length of output: 106
web/src/app/(bare)/auth/[provider]/callback/page.tsx (1)
28-53: Ensure proper dependency management in useEffect hook
With the changes to params and searchParams, verify that the dependencies in your useEffect hook are correctly set. Since params and searchParams are objects returned from hooks, you might want to ensure that the dependencies are stable to prevent unnecessary re-renders or missed updates.
app/services/account/register/register.go (1)
196-196:
Fix parameter order in function call to CreateWithHandle
There is a mismatch in the parameter order when calling CreateWithHandle. The function definition expects parameters in the order (ctx, service, authName, identifier, token, name, handle), but the function call provides (ctx, service, authName, identifier, token, name, handle). Please ensure that the parameters match the function's signature.
Here's the diff to fix the parameter order:
-return s.CreateWithHandle(ctx, service, authName, identifier, token, name, handle)
+return s.CreateWithHandle(ctx, service, authName, identifier, token, name, handle)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
web/src/app/(bare)/auth/[provider]/callback/page.tsx (2)
Line range hint
13-20: Consider simplifying Props type definitionThe Props type uses Promises for
paramsandsearchParams, which is unusual for React component props. This could lead to unnecessary complexity in handling the async values.Consider using direct types instead:
export type Props = { - params: Promise<{ + params: { provider: string; - }>; + }; - searchParams: Promise<OAuthCallback>; + searchParams: OAuthCallback; };
46-51: Enhance error handlingThe current error handling could be more informative for users.
Consider adding error type checking and specific error messages:
{ errorToast: false, onError: async (e) => { - setError(e); + if (e instanceof Response) { + const data = await e.json(); + setError(`Authentication failed: ${data.message}`); + } else { + setError(`Authentication failed: ${e}`); + } }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/app/(bare)/auth/[provider]/callback/page.tsx(2 hunks)
🔇 Additional comments (1)
web/src/app/(bare)/auth/[provider]/callback/page.tsx (1)
1-12: LGTM: Component setup and imports are well-structured
The imports and configuration are appropriate for a Next.js OAuth callback page.
Also updated and fixed some auth frontend issues