search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
212 stars 107 forks source link

feat (platform) : implemented Github auth #517

Open dev-palwar opened 3 weeks ago

dev-palwar commented 3 weeks ago

User description

Description

implemented github based authentication for the platform app. btw for testing put GITHUB_CALLBACK_URL="http://localhost:3025/auth"

Fixes #307

Dependencies

Installed the types for passport

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

If changes are made in the code:

Documentation Update

PR Type

enhancement

Description

Changes walkthrough 📝

Relevant files
Enhancement
page.tsx
Implement GitHub OAuth login and redirect handling             
apps/platform/src/app/auth/page.tsx
  • Added GitHub OAuth login functionality.
  • Implemented redirect handling after GitHub authentication.
  • Utilized useEffect to manage GitHub redirect logic.
  • Added User type for handling user data.
    		•  +50/-2   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    codiumai-pr-agent-free[bot] commented 3 weeks ago

    PR Reviewer Guide 🔍

    (Review updated until commit https://github.com/keyshade-xyz/keyshade/commit/644eedc344e3da43ee8b01e5e004accc4090ee95)

    Here are some key observations to aid the review process:

    **🎫 Ticket compliance analysis 🔶** **[307](https://github.com/keyshade-xyz/keyshade/issues/307) - Partially compliant** Fully compliant requirements: - Implement GitHub endpoint for OAuth login - Redirect to /auth/account-details page after successful authentication Not compliant requirements: - Implement Google and GitLab endpoints for OAuth login - Implement the authentication flow according to the provided Figma designs
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The error handling in the GitHub authentication flow could be improved. Currently, errors are only logged to the console, which may not be sufficient for a production environment. Type Safety
    The `User` type is imported but not defined in this file. Ensure that the `User` type is correctly defined and imported from the appropriate location. Incomplete Implementation
    The PR only implements GitHub authentication, while the ticket requires Google and GitLab authentication as well. The implementation for these providers is missing.
    codiumai-pr-agent-free[bot] commented 3 weeks ago

    PR Code Suggestions ✨

    Latest suggestions up to 644eedc Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Implement CSRF protection in the GitHub authentication process using the state parameter ___ **Use a more secure method to handle the GitHub authentication code, such as using the
    state parameter to prevent CSRF attacks.** [apps/platform/src/app/auth/page.tsx [55-92]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-877fb58ece20e228e7c1cb834472fb2ea0357ecfc751fd3d2e29d0f6906632dfR55-R92) ```diff const handleGithubLogin = (): void => { setIsLoading(true) - window.location.href = `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github` + const state = generateRandomString() // Implement this function to generate a random string + sessionStorage.setItem('githubAuthState', state) + window.location.href = `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github?state=${state}` } const handleGithubRedirect = async () => { const urlParams = new URLSearchParams(window.location.search) const code = urlParams.get('code') + const state = urlParams.get('state') + const storedState = sessionStorage.getItem('githubAuthState') - if (code) { + if (code && state === storedState) { try { const response = await fetch( - `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github/callback?code=${code}`, + `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github/callback?code=${code}&state=${state}`, { method: 'GET', credentials: 'include' } ) // ... rest of the function } catch (error) { // ... error handling } } else { + console.error('Invalid state or no authorization code found in the URL') // ... error handling } + sessionStorage.removeItem('githubAuthState') } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: This suggestion addresses a critical security concern by implementing CSRF protection. It significantly enhances the security of the authentication process, which is crucial for protecting user data and preventing unauthorized access.
    		9
    Enhancement
    Implement error handling and user feedback for failed GitHub authentication attempts ___ **Implement error handling for the GitHub login process, including user feedback for
    failed authentication attempts.** [apps/platform/src/app/auth/page.tsx [60-92]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-877fb58ece20e228e7c1cb834472fb2ea0357ecfc751fd3d2e29d0f6906632dfR60-R92) ```diff +const [error, setError] = useState(null); + const handleGithubRedirect = async () => { const urlParams = new URLSearchParams(window.location.search) const code = urlParams.get('code') if (code) { try { const response = await fetch( `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github/callback?code=${code}`, { method: 'GET', credentials: 'include' } ) if (!response.ok) { - throw new Error('Network response was not ok') + throw new Error('Authentication failed') } const data = (await response.json()) as User setEmail(data.email) if (response.status === 200) { router.push('/auth/account-details') } } catch (error) { - // eslint-disable-next-line no-console -- we need to log the error console.error(`Failed to fetch user data: ${error}`) + setError('GitHub authentication failed. Please try again.') } } else { - // eslint-disable-next-line no-console -- we need to log the error console.error('No authorization code found in the URL') + setError('GitHub authentication failed. Please try again.') } } +// In the JSX: +{error &&
    {error}
    } + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: This suggestion significantly improves user experience by providing clear feedback on authentication failures. It also enhances error handling, which is crucial for robust authentication systems.
    		8
    Encapsulate authentication logic in a custom hook for better code organization and reusability ___ **Consider using a custom hook to encapsulate the GitHub authentication logic,
    improving code organization and reusability.** [apps/platform/src/app/auth/page.tsx [55-96]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-877fb58ece20e228e7c1cb834472fb2ea0357ecfc751fd3d2e29d0f6906632dfR55-R96) ```diff -const handleGithubLogin = (): void => { - setIsLoading(true) - window.location.href = `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github` -} +const useGithubAuth = () => { + const router = useRouter(); + const [, setEmail] = useAtom(authEmailAtom); -const handleGithubRedirect = async () => { - const urlParams = new URLSearchParams(window.location.search) - const code = urlParams.get('code') + const handleGithubLogin = (): void => { + window.location.href = `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github`; + }; - if (code) { - try { - const response = await fetch( - `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github/callback?code=${code}`, - { - method: 'GET', - credentials: 'include' + const handleGithubRedirect = async () => { + const urlParams = new URLSearchParams(window.location.search); + const code = urlParams.get('code'); + + if (code) { + try { + const response = await fetch( + `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github/callback?code=${code}`, + { + method: 'GET', + credentials: 'include' + } + ); + + if (!response.ok) { + throw new Error('Network response was not ok'); } - ) - if (!response.ok) { - throw new Error('Network response was not ok') + const data = (await response.json()) as User; + setEmail(data.email); + + if (response.status === 200) { + router.push('/auth/account-details'); + } + } catch (error) { + console.error(`Failed to fetch user data: ${error}`); } + } else { + console.error('No authorization code found in the URL'); + } + }; - const data = (await response.json()) as User - setEmail(data.email) + useEffect(() => { + void handleGithubRedirect(); + }, []); - if (response.status === 200) { - router.push('/auth/account-details') - } - } catch (error) { - // eslint-disable-next-line no-console -- we need to log the error - console.error(`Failed to fetch user data: ${error}`) - } - } else { - // eslint-disable-next-line no-console -- we need to log the error - console.error('No authorization code found in the URL') - } -} + return { handleGithubLogin }; +}; -useEffect(() => { - void handleGithubRedirect() -}, []) +// In the component: +const { handleGithubLogin } = useGithubAuth(); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: This suggestion improves code organization and reusability by extracting the GitHub authentication logic into a custom hook. It enhances maintainability and separation of concerns, which is particularly valuable for complex authentication flows.
    		7
    Add a loading state to the GitHub login button for better user feedback during authentication ___ **Consider using a loading state for the GitHub login button to provide visual
    feedback during the authentication process.** [apps/platform/src/app/auth/page.tsx [113-118]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-877fb58ece20e228e7c1cb834472fb2ea0357ecfc751fd3d2e29d0f6906632dfR113-R118) ```diff +const [isGithubLoading, setIsGithubLoading] = useState(false); + +const handleGithubLogin = async (): Promise => { + setIsGithubLoading(true); + try { + // Existing login logic + } catch (error) { + console.error('GitHub login failed:', error); + } finally { + setIsGithubLoading(false); + } +}; + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: This suggestion improves user experience by providing visual feedback during the authentication process. While not critical, it enhances the overall usability and responsiveness of the interface.
    		6

    💡 Need additional feedback ? start a PR chat

    Previous suggestions

    Suggestions up to commit 5bf82d3
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Correct input field attributes to match their intended purpose ___ **The id attribute for the Description input is set to "name", which is incorrect and
    could lead to confusion. Change it to "description" for clarity and consistency.** [apps/platform/src/app/(main)/page.tsx [144-154]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-d6c7ffe7a9ea578ea4b0a5587f2759a7a1eac9a7c7d6dfae25ddd0bae48ea8e2R144-R154) ```diff { setNewProjectData((prev) => ({ ...prev, description: e.target.value })) }} - placeholder="Enter the name" + placeholder="Enter the description" /> ```
    Suggestion importance[1-10]: 9 Why: The suggestion addresses a critical issue where the `id` attribute for the Description input is incorrectly set to "name". Correcting this to "description" prevents potential confusion and ensures consistency, significantly improving code correctness.
    		9
    Best practice
    Properly handle asynchronous functions in useEffect hooks ___ **The handleGithubRedirect function is an async function, but it's called without
    awaiting in the useEffect hook. Consider using an IIFE (Immediately Invoked Function
    Expression) to properly handle the asynchronous nature of this function.** [apps/platform/src/app/auth/page.tsx [94-96]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-877fb58ece20e228e7c1cb834472fb2ea0357ecfc751fd3d2e29d0f6906632dfR94-R96) ```diff useEffect(() => { - void handleGithubRedirect() -}, []) + (async () => { + await handleGithubRedirect(); + })(); +}, []); ```
    Suggestion importance[1-10]: 8 Why: The suggestion to use an IIFE for handling the asynchronous `handleGithubRedirect` function in the `useEffect` hook is a best practice that ensures proper handling of asynchronous operations, improving code reliability and maintainability.
    		8
    Use more descriptive variable names to improve code readability ___ **Consider using a more descriptive variable name instead of fileIcon. For example,
    newProjectIcon would better convey its purpose in the context of creating a new
    project.** [apps/platform/src/app/(main)/page.tsx [30]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-d6c7ffe7a9ea578ea4b0a5587f2759a7a1eac9a7c7d6dfae25ddd0bae48ea8e2R30-R30) ```diff -import fileIcon from '../../assets/Group 12.png' +import newProjectIcon from '../../assets/Group 12.png' ```
    Suggestion importance[1-10]: 6 Why: The suggestion to rename `fileIcon` to `newProjectIcon` enhances code readability by providing a more descriptive name, which clarifies the variable's purpose in the context of creating a new project.
    		6
    Enhancement
    Use distinct background colors for error and success messages to improve user experience ___ **The error and success toast messages use the same CSS class for background color
    (bg-errorRed). Consider using a different class for success messages, such as
    bg-successGreen, to visually distinguish between error and success states.** [apps/web/src/components/hero/index.tsx [38-43]](https://github.com/keyshade-xyz/keyshade/pull/517/files#diff-7613ce5c9e898c20a2427ba5bf13810f61bb0a532139896e00a54cd0285ad40cR38-R43) ```diff -
    +

    You have been already added to the waitlist. We will notify you once we launch.

    ```
    Suggestion importance[1-10]: 7 Why: The suggestion to use different background colors for error and success messages enhances user experience by providing clear visual feedback, which is an important aspect of UI design.
    		7
    codiumai-pr-agent-free[bot] commented 3 weeks ago

    Persistent review updated to latest commit https://github.com/keyshade-xyz/keyshade/commit/644eedc344e3da43ee8b01e5e004accc4090ee95

    rajdip-b commented 2 weeks ago

    @dev-palwar hey bro, any news on this?

    dev-palwar commented 2 weeks ago

    @rajdip-b yeah i was busy with my practicals. Will start working on it now!

    kriptonian1 commented 3 days ago

    Hey @dev-palwar how is this issue going on

    dev-palwar commented 2 days ago

    I was stuck at this and did not get any response.

    https://github.com/keyshade-xyz/keyshade/pull/517#discussion_r1842350878