Github SSO #191
Github SSO #191
Conversation
Deploying frontend-mu-nuxt with
|
| Latest commit: |
fa83d9f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1aebb22c.frontend-mu-staging.pages.dev |
| Branch Preview URL: | https://163-github-login.frontend-mu-staging.pages.dev |
|
TODO: Add error handling on failed GitHub sign in. Reasons for failure:
|
WalkthroughThe changes in this pull request enhance the authentication functionality within the Nuxt.js application. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Auth
participant Server
User->>Auth: oAuthLogin(provider)
Auth->>Server: Request login URL
Server-->>Auth: Return login URL
Auth->>User: Redirect to login URL
User->>Auth: Login success
Auth->>Server: Fetch current user data
Server-->>Auth: Return user data (including provider, external_identifier)
Auth->>User: Display user profile
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: 4
🧹 Outside diff range and nitpick comments (6)
packages/frontendmu-nuxt/utils/helpers.ts (2)
39-39: Improvedfull_nameconstruction looks good!The use of the nullish coalescing operator enhances the robustness of the
full_nameconstruction. This change ensures that even ifuser.last_nameis undefined, thefull_namewill be properly formatted.Consider using optional chaining for
user.first_nameas well, to handle cases where it might be undefined:const full_name = user?.full_name ? user.full_name : `${user?.first_name ?? ''} ${user?.last_name ?? ''}`.trim()This change would make the function more resilient to potential undefined values in the
userobject.
Line range hint
39-57: Summary: Changes align well with GitHub SSO implementationThe modifications to the
mapToValidUserfunction in this file are well-aligned with the PR's objective of implementing GitHub SSO. The improvedfull_nameconstruction and the addition ofproviderandexternal_identifierproperties enhance the function's capability to handle user data from different authentication sources.As you continue with the SSO implementation:
- Ensure consistent error handling across the application for cases where GitHub sign-in might fail.
- Consider adding type checks or assertions for the
userobject to catch potential type mismatches early.- Update relevant interfaces or types to reflect these new properties, ensuring type safety throughout the application.
- Document the expected format and source of the
providerandexternal_identifierproperties for future maintainability.packages/frontendmu-nuxt/components/auth/LoggedUser.vue (1)
Line range hint
1-132: Summary: GitHub SSO implementation progresses, but needs error handling enhancementsOverall, the changes in this file successfully implement parts of the GitHub SSO functionality and significantly improve code readability. Key points:
- The GitHub avatar handling has been implemented, addressing part of the PR objectives.
- UI enhancements, such as the Provider Hint, improve the user experience.
- Code formatting changes throughout the file enhance readability and maintainability.
However, to fully meet the PR objectives and ensure robustness:
- Implement comprehensive error handling for GitHub sign-in failures, including cases where the user's email is not public.
- Add fallback mechanisms for unknown authentication providers in the Provider Hint section.
- Ensure all potential avatar sources and provider icons are properly handled to avoid UI inconsistencies.
These enhancements will improve the reliability and user experience of the SSO feature.
Consider implementing a centralized error handling mechanism for authentication-related issues. This could include:
- A dedicated error handling service that can be injected into components.
- Standardized error messages and UI components for displaying authentication errors.
- Logging of authentication errors for monitoring and debugging purposes.
This approach would make it easier to manage and update error handling across the application as new authentication methods are added.
packages/frontendmu-nuxt/components/auth/RsvpAttendeeList.vue (1)
99-105: LGTM! Consider adding error handling for image loading.The addition of GitHub-specific avatar handling is a good improvement and aligns well with the GitHub SSO implementation. However, to enhance robustness, consider adding error handling for cases where the image fails to load.
Here's a suggestion to add error handling:
<template v-if="attendee.provider === 'github'"> <img :src="`https://github.com/${attendee.external_identifier}.png`" :alt="attendee.name" @error="handleImageError" class="rounded-full mx-auto w-28 h-28 aspect-square" > </template>Add a method to handle the error:
const handleImageError = (event: Event) => { // Fall back to default avatar or show an error message (event.target as HTMLImageElement).src = '/path/to/default/avatar.png' }This approach ensures a fallback if the GitHub avatar fails to load, improving the user experience.
packages/frontendmu-nuxt/utils/types.ts (1)
39-40: LGTM! Consider adding JSDoc comments for clarity.The addition of
providerandexternal_identifierproperties to theUserinterface is appropriate for implementing GitHub SSO. These optional properties allow for storing the authentication provider and an external identifier, which is consistent with the PR objectives.Consider adding JSDoc comments to explain the purpose of these new properties:
/** The authentication provider (e.g., 'github', 'google') */ provider?: string; /** An identifier provided by the external authentication service */ external_identifier?: string;This will improve code readability and provide context for other developers.
packages/frontendmu-nuxt/auth-utils/useAuth.ts (1)
Line range hint
105-125: Enhance error handling in 'loginWithSSO' to inform users upon failureThe
loginWithSSOfunction logs errors to the console but doesn't provide feedback to the user when the SSO login fails. Consider displaying an error message usinguseToast()to improve user experience by informing them of the issue.Apply this diff to enhance error handling:
catch (error) { console.error('Could not get access token from refresh token') console.log(error) + useToast().show({ + title: 'Login Failed', + message: 'Unable to log in using Single Sign-On. Please try again or contact support.', + type: 'ERROR', + visible: true, + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/frontendmu-nuxt/auth-utils/useAuth.ts (3 hunks)
- packages/frontendmu-nuxt/components/auth/LoggedUser.vue (3 hunks)
- packages/frontendmu-nuxt/components/auth/LoginForm.vue (5 hunks)
- packages/frontendmu-nuxt/components/auth/MyProfile.vue (8 hunks)
- packages/frontendmu-nuxt/components/auth/RsvpAttendeeList.vue (1 hunks)
- packages/frontendmu-nuxt/utils/helpers.ts (2 hunks)
- packages/frontendmu-nuxt/utils/types.ts (2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/frontendmu-nuxt/components/auth/LoginForm.vue
- packages/frontendmu-nuxt/components/auth/MyProfile.vue
🧰 Additional context used
🔇 Additional comments (11)
packages/frontendmu-nuxt/utils/helpers.ts (1)
56-57: New properties for GitHub SSO look good, but consider adding error handling.The addition of
providerandexternal_identifierproperties aligns well with the PR's objective of implementing GitHub SSO. These properties will be crucial for identifying users across different authentication methods.To ensure robustness, consider adding error handling for cases where these properties might be undefined. This aligns with the PR comment about handling GitHub sign-in failures. Here's a script to verify the usage of these new properties throughout the codebase:
This script will help identify where these new properties are used and if there's any existing error handling for them.
packages/frontendmu-nuxt/components/auth/LoggedUser.vue (6)
19-22: Improved button formatting enhances readabilityThe formatting changes to the BaseButton components improve code readability without altering functionality. This is a positive change that aligns with best practices for template structure in Vue components.
Also applies to: 28-32
37-44: Enhanced menu structure improves code organizationThe formatting changes to the logged-in user menu structure improve the overall organization and readability of the template. This change makes the component's structure more clear and maintainable.
83-86: Improved transition component formattingThe formatting changes to the transition component attributes enhance code readability without altering functionality. This change is in line with best practices for maintaining clean and organized templates.
96-97: Enhanced NuxtLink formatting in menu itemsThe formatting changes to the NuxtLink components within MenuItems improve code readability and maintainability. The proper alignment and line breaks make the template structure more clear without affecting functionality.
Also applies to: 105-106
116-119: Improved avatar image formatting in user profileThe formatting changes to the avatar image display in the user profile section enhance code readability. The proper indentation of the img tag attributes aligns with best practices for template structure.
63-76: Provider Hint enhances UI, consider fallback for unknown providersThe addition of the Provider Hint section is a great UI enhancement, providing users with visual feedback about their authentication method. However, to ensure robustness:
- Consider adding a fallback icon for unknown or unsupported providers.
- Ensure that the
carbon:logo-${user?.provider}icon exists for all supported providers to avoid broken images.Let's verify the supported providers:
packages/frontendmu-nuxt/components/auth/RsvpAttendeeList.vue (1)
99-111: Overall, good improvements with minor suggestions for enhancement.The changes effectively implement GitHub-specific avatar handling, which aligns well with the GitHub SSO objective. The standardization of image class attributes improves consistency. Consider implementing the suggested error handling for image loading and verifying the
external_identifierusage to further enhance the robustness of this feature.packages/frontendmu-nuxt/utils/types.ts (1)
39-40: Verify usage of new properties in related components.The changes to the
UserandAttendeeinterfaces are well-aligned with the PR objectives for implementing GitHub SSO. These new properties will allow for better integration with external authentication providers.To ensure full implementation:
- Verify that components handling user authentication and attendee management are updated to use these new properties.
- Check if any database schemas or API endpoints need to be updated to accommodate these new fields.
- Ensure that the error handling mentioned in the PR description is implemented, particularly for cases where GitHub sign-in fails.
Run the following script to identify potential areas that might need updates:
This will help identify areas of the codebase that might need to be updated to fully utilize the new properties.
Also applies to: 220-221
packages/frontendmu-nuxt/auth-utils/useAuth.ts (2)
117-117: LGTM: Adding 'mode' parameter enhances the SSO login processThe addition of
mode: 'cookie'in the request body ensures that the refresh token cookie is correctly sent during the SSO login process.
175-175: Include 'external_identifier' and 'provider' in user data retrievalAdding
'external_identifier'and'provider'toACCOUNT_SETTINGS_FIELDSensures these fields are included when fetching the current user's data, which is essential for handling multiple OAuth providers.Also applies to: 180-180
| class="inline-flex relative items-center w-full justify-center gap-x-1.5 rounded-full text-sm font-semibold text-verse-900 dark:text-verse-100 shadow-sm ring-gray-300 hover:bg-gray-50" | ||
| > | ||
| <div v-if="avatarUrl"> | ||
| <img class="w-10 aspect-square rounded-full" :src="avatarUrl"> | ||
| <template v-if="user?.provider === 'github'"> | ||
| <img | ||
| class="w-10 aspect-square rounded-full" | ||
| :src="`https://github.com/${user?.external_identifier}.png`" | ||
| > | ||
| </template> | ||
| <template v-else> | ||
| <div v-if="avatarUrl"> | ||
| <img | ||
| class="w-10 aspect-square rounded-full" | ||
| :src="avatarUrl" | ||
| > | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
💡 Codebase verification
GitHub SSO avatar handling implemented correctly, but error cases need attention
The codebase analysis confirms that the GitHub SSO avatar handling has been implemented as intended. The LoggedUser.vue component correctly renders the GitHub avatar using the external_identifier:
<template v-if="user?.provider === 'github'">
<img
class="w-10 aspect-square rounded-full"
:src="`https://github.com/${user?.external_identifier}.png`"
>
</template>However, the initial concern about error handling remains valid. To address potential error cases, consider the following recommendations:
- Implement a fallback avatar for cases where the GitHub avatar URL is invalid or inaccessible.
- Add error handling for scenarios where the GitHub sign-in fails or the user's email is not set to public in their GitHub settings.
- In the
useAuth.tsfile, enhance theavatarUrlcomputed property to handle potential errors:
const avatarUrl = computed(() => {
if (user.value?.avatar_url) {
return user.value.avatar_url
} else if (rawUser.value?.github_username) {
return `https://github.com/${rawUser.value.github_username}.png`
} else if (user.value?.provider === 'github' && user.value?.external_identifier) {
return `https://github.com/${user.value.external_identifier}.png`
} else {
return '/path/to/fallback/avatar.png' // Provide a fallback avatar
}
})- In the
LoggedUser.vuecomponent, add error handling for the avatar image:
<img
class="w-10 aspect-square rounded-full"
:src="`https://github.com/${user?.external_identifier}.png`"
@error="handleAvatarError"
alt="User avatar"
>Add a method to handle avatar loading errors:
function handleAvatarError(event: Event) {
const img = event.target as HTMLImageElement;
img.src = '/path/to/fallback/avatar.png'; // Set a fallback avatar
}These changes will improve the robustness of the avatar handling and provide a better user experience in case of errors.
🔗 Analysis chain
GitHub SSO avatar handling implemented, but error cases need attention
The addition of conditional rendering for user avatars, especially the GitHub-specific handling, is a good step towards implementing the GitHub SSO functionality. However, as per the PR objectives, we need to address potential error cases.
Consider adding error handling for scenarios where:
- The GitHub sign-in fails
- The user's email is not set to public in their GitHub settings
- The avatar URL is invalid or inaccessible
To ensure we're handling all potential avatar sources correctly, let's verify the user object structure:
Correction Needed: Shell script errors detected during user object verification
The previous verification script encountered errors due to unrecognized file types and a typo in the directory path. Please use the corrected script below to verify the structure of the user object:
Further Verification Needed: Unable to locate user object definitions
The previous attempts to verify the user object structure did not yield any results. To ensure comprehensive coverage, please execute the following scripts to search for any occurrences of user and its declarations within the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the structure of the user object in auth-related files
# Test: Search for user object definitions or usages
rg --type vue --type ts 'user\s*:\s*\{' packages/frontendmu-nuxt
Length of output: 197
Script:
# Description: Verify the structure of the user object in auth-related files
# Corrected Test: Search for user object definitions or usages in .vue and .ts files
rg 'user\s*:\s*\{' packages/frontendmu-nuxt --glob '*.vue' --glob '*.ts'
Length of output: 74
Script:
# Description: Broad search for any occurrences of 'user' in .vue and .ts files
rg 'user' packages/frontendmu-nuxt --glob '*.vue' --glob '*.ts'
Length of output: 17940
Script:
# Description: Use ast-grep to find user object declarations in .vue and .ts files
ast-grep --lang typescript --pattern $'const user: $_Type = { $$$ }' packages/frontendmu-nuxt
Length of output: 94
| <template v-if="attendee.provider === 'github'"> | ||
| <img | ||
| :src="`https://github.com/${attendee.external_identifier}.png`" | ||
| alt="" | ||
| class="rounded-full mx-auto w-28 h-28 aspect-square" | ||
| > | ||
| </template> |
There was a problem hiding this comment.
💡 Codebase verification
Potential issue with using 'external_identifier' for GitHub avatars
Upon investigation, we couldn't verify that attendee.external_identifier always corresponds to the GitHub username. This inconsistency could lead to incorrect avatar displays for some users. Consider the following points:
- There's no clear relationship between 'external_identifier' and 'github_username' in the codebase.
- Other parts of the application seem to use 'github_username' for similar purposes.
- The population of the 'attendee' object and the setting of 'external_identifier' are not clearly visible in the current context.
To address this issue:
- Consider changing the code to use a more explicit property, such as
attendee.github_username, if available. - If 'external_identifier' is intended to be used for multiple provider types, implement a check to ensure it's only used for GitHub providers.
- Conduct a manual review of how attendee data is populated and how GitHub-specific information is handled throughout the application.
Example of a more robust implementation:
<template v-if="attendee.provider === 'github'">
<img
:src="getGithubAvatarUrl(attendee)"
alt=""
class="rounded-full mx-auto w-28 h-28 aspect-square"
>
</template>function getGithubAvatarUrl(attendee) {
if (attendee.github_username) {
return `https://github.com/${attendee.github_username}.png`;
} else if (attendee.provider === 'github' && attendee.external_identifier) {
return `https://github.com/${attendee.external_identifier}.png`;
}
return 'path/to/default/avatar.png'; // Fallback to a default avatar
}This approach provides more clarity and fallback options, reducing the risk of displaying incorrect avatars.
🔗 Analysis chain
Verify the usage of 'external_identifier' for GitHub avatars.
The code assumes that attendee.external_identifier corresponds to the GitHub username. While this is likely correct, it's worth verifying to ensure it always provides the correct GitHub username for avatar URLs.
To confirm this, you can run the following script:
This script will help identify how external_identifier is set for GitHub users, allowing us to confirm its correctness for avatar URL construction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of 'external_identifier' for GitHub users
# Search for the definition or assignment of 'external_identifier'
echo "Searching for 'external_identifier' definition or assignment:"
rg --type vue --type typescript 'external_identifier.*=' -A 5
# Search for GitHub-related user data handling
echo "\nSearching for GitHub-related user data handling:"
rg --type vue --type typescript 'github.*user' -A 10
Length of output: 424
Script:
#!/bin/bash
# Description: Verify the usage of 'external_identifier' for GitHub users
# Search for the definition or assignment of 'external_identifier'
echo "Searching for 'external_identifier' definition or assignment:"
rg --glob '*.vue' --glob '*.ts' 'external_identifier\s*=' -A 5
# Search for GitHub-related user data handling
echo "\nSearching for GitHub-related user data handling:"
rg --glob '*.vue' --glob '*.ts' 'github.*user' -A 10
Length of output: 38046
Script:
#!/bin/bash
# Description: Further investigation of 'external_identifier' and GitHub user data
# Search for any relation between 'external_identifier' and 'github_username'
echo "Searching for relation between 'external_identifier' and 'github_username':"
rg --glob '*.{vue,ts,js}' -i 'external_identifier.*github_username|github_username.*external_identifier' -A 5 -B 5
# Search for the definition or population of the 'attendee' object
echo "\nSearching for 'attendee' object definition or population:"
rg --glob '*.{vue,ts,js}' 'attendee\s*=|attendee:' -A 10
# Search for GitHub API calls or user data fetching
echo "\nSearching for GitHub API calls or user data fetching:"
rg --glob '*.{vue,ts,js}' 'github.*api|fetch.*github' -A 10
Length of output: 2560
| provider: string | ||
| external_identifier: string |
There was a problem hiding this comment.
Consider making new properties optional for consistency.
The addition of provider and external_identifier properties to the Attendee interface is appropriate for implementing GitHub SSO. However, unlike in the User interface, these properties are not marked as optional.
To maintain consistency with the User interface and ensure backward compatibility, consider making these properties optional:
provider?: string;
external_identifier?: string;This change will prevent potential issues with existing code or data that doesn't include these new properties.
| function oAuthLogin(provider: string) { | ||
| if (provider === 'google') { | ||
| const currentPage = new URL(window.location.origin) | ||
| return `${DIRECTUS_URL()}/auth/login/google?redirect=${currentPage}redirect` | ||
| } | ||
| else if (provider === 'github') { | ||
| const currentPage = new URL(window.location.origin) | ||
| return `${DIRECTUS_URL()}/auth/login/github?redirect=${currentPage}redirect` | ||
| } | ||
| else if (provider === 'discord') { | ||
| const currentPage = new URL(window.location.origin) | ||
| return `${DIRECTUS_URL()}/auth/login/discord?redirect=${currentPage}redirect` | ||
| } | ||
|
|
||
| console.log('Provider not found') | ||
|
|
||
| return false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor 'oAuthLogin' function to eliminate code duplication
The oAuthLogin function contains repetitive code for each provider. Consider refactoring to eliminate duplication and improve maintainability. You can use a mapping of providers to their respective paths and handle the redirect logic more concisely.
Apply this diff to refactor the function:
function oAuthLogin(provider: string) {
- if (provider === 'google') {
- const currentPage = new URL(window.location.origin)
- return `${DIRECTUS_URL()}/auth/login/google?redirect=${currentPage}redirect`
- }
- else if (provider === 'github') {
- const currentPage = new URL(window.location.origin)
- return `${DIRECTUS_URL()}/auth/login/github?redirect=${currentPage}redirect`
- }
- else if (provider === 'discord') {
- const currentPage = new URL(window.location.origin)
- return `${DIRECTUS_URL()}/auth/login/discord?redirect=${currentPage}redirect`
- }
-
- console.log('Provider not found')
-
- return false
+ const currentPage = window.location.origin + '/redirect'
+ const providers = ['google', 'github', 'discord']
+ if (providers.includes(provider)) {
+ return `${DIRECTUS_URL()}/auth/login/${provider}?redirect=${currentPage}`
+ } else {
+ console.error('Provider not found')
+ return false
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function oAuthLogin(provider: string) { | |
| if (provider === 'google') { | |
| const currentPage = new URL(window.location.origin) | |
| return `${DIRECTUS_URL()}/auth/login/google?redirect=${currentPage}redirect` | |
| } | |
| else if (provider === 'github') { | |
| const currentPage = new URL(window.location.origin) | |
| return `${DIRECTUS_URL()}/auth/login/github?redirect=${currentPage}redirect` | |
| } | |
| else if (provider === 'discord') { | |
| const currentPage = new URL(window.location.origin) | |
| return `${DIRECTUS_URL()}/auth/login/discord?redirect=${currentPage}redirect` | |
| } | |
| console.log('Provider not found') | |
| return false | |
| function oAuthLogin(provider: string) { | |
| const currentPage = window.location.origin + '/redirect' | |
| const providers = ['google', 'github', 'discord'] | |
| if (providers.includes(provider)) { | |
| return `${DIRECTUS_URL()}/auth/login/${provider}?redirect=${currentPage}` | |
| } else { | |
| console.error('Provider not found') | |
| return false | |
| } | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor