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

Log in using GitHub #680

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

carlos-granados
Copy link
Contributor

@carlos-granados carlos-granados commented Sep 30, 2024

Given that users can connect their GitHub account to their Pinkary account, this PR allows them to log in using their GitHub account. We show a new "log in using GitHub" link in the login page.

  • If you have already linked your GitHub account to your Pinkary account, when clicking this link you will be automatically logged in.
  • If you have not linked your GitHub account to your Pinkary account but there is a Pinkary account linked to your GitHub email, when clicking this link you will be automatically logged in and your GitHub account will be linked to your Pinkary account
  • If there is no Pinkary account that matches your GitHub email, you will be redirected to the "Register" page with the email pre-filled with your GitHub account and you will be able to fill in the rest of your details. Once you submit your registration, your GitHub account will also be linked to your PInkary Account

NOTE: Given that when setting up the application in GitHub you cannot list more than one callback URL and given that now we need to redirect the GitHub login to two different endpoints depending on whether you are logged in or not, we need to modify the URL listed in the application to only list the prefix that is common to both URLs, because GitHub will accept any callback URL that matches the prefix listed in the app definition. So instead of listing the callback URL as :
https://pinkary.com/profile/connect/github/update
It needs to be listed as:
https://pinkary.com/profile/connect/github
This will also accept the new URL:
https://pinkary.com/profile/connect/github/login/callback
This change can be done in advance, before deploying this new version and it will continue to work with the old version

NOTE: My experience with Laravel is quite limited. I have tried to follow the same principles and architecture as in the existing code but there may be cases where things could have been done in a better way. If that is the case, let me know and I will update the code

See the PR comments for more info

@@ -65,7 +65,6 @@ LEMON_SQUEEZY_PATH=
GITHUB_PERSONAL_ACCESS_TOKEN=
GITHUB_CLIENT_ID=
GITHUB_CLIENT_SECRET=
GITHUB_REDIRECT_URI=
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that there are now two different callbacks, the redirect URL is not loaded from the config, it is set up dynamically in each of the redirecting endpoints

/**
* Handles the GitHub login redirect.
*/
public function index(): RedirectResponse
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the endpoint called from the "Log in using github" in the login page, it redirects you to GitHub's authentication page, setting the right callback url


$user = User::where('email', $githubUser->getEmail())->first();
if (! $user instanceof User) {
session([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is no user with the email sent back by GitHub, we redirect the user to the "register" page, and we save in the session the github email and the username so that they can be used in that page

return to_route('register');
}
if ($user->github_username === null) {
$errors = $github->linkGitHubUser($githubUser->getNickname(), $user, $request);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user with the GitHub email is not yet linked to this GitHub account, we try to link it

if ($user->github_username === null) {
$errors = $github->linkGitHubUser($githubUser->getNickname(), $user, $request);

if ($errors !== []) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error will only happen in a very unusual situation: if there is already a user linked to this GitHub account but the email in the Pinkary account does not match the email in the GitHub account. In this case we cannot link the user or log them in, so we redirect back to login with an error

@@ -61,11 +72,12 @@ class="mt-2"
/>
<x-text-input
id="email"
class="mt-1 block w-full"
class="mt-1 block w-full {{ $githubEmail !== null ? 'text-slate-500' : '' }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we received the email from GitHub we mark it as readonly and with a lighter colour

@@ -162,6 +174,15 @@ class="mt-2"
/>
@endif

@if ($githubUsername)
<x-text-input
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We add a hidden field to be able to send back the GitHub username back to the registration controller

@@ -47,6 +48,15 @@
->middleware('throttle:two-factor');
});

Route::prefix('/profile/connect/github')->group(function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new routes for the GitHub login. They don't need authorization

@@ -17,4 +17,5 @@

$response->assertStatus(302);
$response->assertRedirectContains('https://github.com/login/oauth/authorize');
$response->assertRedirectContains('update');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We test that both callback URLs are different

], absolute: false));

expect($user->email_verified_at)->toBeNull();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you register normally, your email won't be verified

@carlos-granados
Copy link
Contributor Author

@nunomaduro any interest on this? Just wondering if I need to keep it updated. Thanks!!

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

Successfully merging this pull request may close these issues.

1 participant