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 (platform) : implemented Github auth #517

Closed
wants to merge 1 commit into from

Conversation

dev-palwar
Copy link

@dev-palwar dev-palwar commented Nov 3, 2024

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

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement


Description

  • Implemented GitHub OAuth login functionality in the authentication page.
  • Added a function to handle redirection after successful GitHub authentication.
  • Utilized useEffect to manage the GitHub redirect logic upon component mount.
  • Introduced a User type to handle user data received from the backend.

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

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Nov 3, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 644eedc)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    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.

    @dev-palwar dev-palwar changed the title Implementing Github auth feat (platform) : implemented Github auth Nov 3, 2024
    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Nov 3, 2024

    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]

     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]

    +const [error, setError] = useState<string | null>(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 && <div className="error-message">{error}</div>}
    +
    • 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]

    -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]

    +const [isGithubLoading, setIsGithubLoading] = useState(false);
    +
    +const handleGithubLogin = async (): Promise<void> => {
    +  setIsGithubLoading(true);
    +  try {
    +    // Existing login logic
    +  } catch (error) {
    +    console.error('GitHub login failed:', error);
    +  } finally {
    +    setIsGithubLoading(false);
    +  }
    +};
    +
     <Button
    -  onClick={() => {
    -    handleGithubLogin()
    -  }}
    +  onClick={handleGithubLogin}
    +  disabled={isGithubLoading}
     >
    -  <GithubSVG />
    +  {isGithubLoading ? <LoadingSVG /> : <GithubSVG />}
     </Button>
    • 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]

     <Input
       className="col-span-3"
    -  id="name"
    +  id="description"
       onChange={(e) => {
         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]

     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]

    -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]

    -<div className="text-brandBlue border-brandBlue/20 bg-errorRed w-[90vw] rounded-lg border p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]">
    +<div className="text-brandBlue border-brandBlue/20 bg-successGreen w-[90vw] rounded-lg border p-2 shadow-2xl backdrop-blur-3xl md:w-[20vw]">
       <p className="text-sm">
         You have been already added to the waitlist. We will notify you once
         we launch.
       </p>
     </div>
    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

    Copy link
    Member

    Choose a reason for hiding this comment

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

    I'm not sure why you would need to change the API layer

    Copy link
    Author

    Choose a reason for hiding this comment

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

    small mistake. I was tinkering with the backend and forgot to unstage the changes :)

    Copy link
    Member

    Choose a reason for hiding this comment

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

    A lot of code in this file is unnecesary and non related

    Copy link
    Member

    Choose a reason for hiding this comment

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

    This file is irrelevant aswell

    Copy link
    Contributor

    Persistent review updated to latest commit 644eedc

    @dev-palwar dev-palwar requested a review from rajdip-b November 4, 2024 09:50
    Copy link
    Member

    Choose a reason for hiding this comment

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

    All of the authentication logic is taken care of in the backend itself. You would just need to configure these things:

    • How the button works
    • What happens after the backend redirects to the oauth success page.

    I would recommend you to manually run the authentication once.

    Copy link
    Author

    Choose a reason for hiding this comment

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

    After clicking the button, the user is redirected to a GitHub auth screen. Then, an API request is made to fetch user data along with a token. I set the email state and redirect the user to the /account-details page, where everything works fine up to this point

    image

    the response includes this message

    image

    If you could explain what it means, then maybe I could do something. Otherwise, it's a dead end for me :)

    Copy link
    Member

    Choose a reason for hiding this comment

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

    I'll fix cookies give me a moment

    Copy link
    Member

    Choose a reason for hiding this comment

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

    @poswalsameer can you please look into this PR? Auth is not working as expected

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    @poswalsameer can you please look into this PR? Auth is not working as expected

    Sure.

    }
    } catch (error) {
    // eslint-disable-next-line no-console -- we need to log the error
    console.error(`Failed to fetch user data: ${error}`)
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    If you could add toast messages to handle the error states in a well-mannered way, then that would be really nice.

    Copy link
    Author

    Choose a reason for hiding this comment

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

    Yeah, I planned to do this after implementing the functionalities first, but then I got stuck and someone asked me to send the PR for review. Is it working fine on your end?

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Yeah, I planned to do this after implementing the functionalities first, but then I got stuck and someone asked me to send the PR for review. Is it working fine on your end?

    What's the problem you are facing?

    Copy link
    Author

    Choose a reason for hiding this comment

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

    After clicking the button, the user is redirected to a GitHub auth screen. Then, an API request is made to fetch user data along with a token. I set the email state and redirect the user to the /account-details page, where everything works fine up to this point

    image

    the response includes this message

    image

    If you could explain what it means, then maybe I could do something. Otherwise, it's a dead end for me :)

    this is the problem

    }
    } else {
    // eslint-disable-next-line no-console -- we need to log the error
    console.error('No authorization code found in the URL')
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Add toast here too. Go through the Figma file to know the color scheme.

    Copy link
    Author

    Choose a reason for hiding this comment

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

    got it 👍

    if (code) {
    try {
    const response = await fetch(
    `${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github/callback?code=${code}`,
    Copy link
    Contributor

    @poswalsameer poswalsameer Nov 9, 2024

    Choose a reason for hiding this comment

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

    on this line, you are hitting {{BASE_URL}}/api/auth/github/callback but in the backend, we have got {{BASE_URL}}/api/auth/github only. Attaching an image for your reference. Do it this way and let us know if you still face any errors.

    Screenshot (492)

    Copy link
    Author

    Choose a reason for hiding this comment

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

    I am hitting this url and it redirects to a page (wherever i want) with a code but to get the user details i have to fetch the data from here
    ${process.env.NEXT_PUBLIC_BACKEND_URL}/api/auth/github/callback?code=${code}

    this return a JSON object with user details and token. but the main problem is here
    image

    and this is response im gettin
    image

    Copy link
    Member

    Choose a reason for hiding this comment

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

    Here's the flow @dev-palwar:

    • On clicking the GitHub button, users should be redirected to <API_BASE_URL>/api/auth/github
    • The API handles all the logic in here and then sends the user to github auth page.
    • The user authenticates themselves on github
    • Github sends back a redirect to the API with authentication success.
    • The backend then sends back a redirect to a <FRONTEND_BASE_URL>/oauth/signin and attaches a data query object to this redirect url that has all the user data.

    @rajdip-b
    Copy link
    Member

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

    @dev-palwar
    Copy link
    Author

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

    @kriptonian1
    Copy link
    Contributor

    Hey @dev-palwar how is this issue going on

    @dev-palwar
    Copy link
    Author

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

    #517 (comment)

    @dev-palwar dev-palwar closed this Dec 2, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: Implement Google, GitLab and GitHub
    4 participants