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

Google Authentication using the new API #1036

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

CosminBd
Copy link
Contributor

@CosminBd CosminBd commented Jun 8, 2023

This PR is related to #940
Using the new Google API for authentication, which moves away from OAuth and to using JWT's for identifing users.

There are still a couple of things to be fixed, mainly:

  • the manager package requires providers to implement ProviderInterfaces for OAuth1 or OAuth2. This needs to be addressed in the root package as the class has a few redundant methods. Same thing applies for the Google User class.
  • I've tried to add testing but when moving files to separate "/src" and "/tests" directories I'm getting autoload errors from composer
  • client_secret is not needed, but required by the manager package

@stale
Copy link

stale bot commented Aug 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 12, 2023
@lucasmichot
Copy link
Member

Hello @CosminBd and thanks for your contribution PR
Can you resolve the conflict in src/Google/composer.json ?

@stale stale bot removed the stale label Aug 24, 2023
src/Google/Provider.php Show resolved Hide resolved
src/Google/README.md Outdated Show resolved Hide resolved
src/Google/GoogleExtendSocialite.php Outdated Show resolved Hide resolved
@atymic
Copy link
Member

atymic commented Aug 29, 2023

@lucasmichot

@CosminBd could you resolve conflicts?

@CosminBd
Copy link
Contributor Author

@atymic it's good to go now

src/Google/composer.json Outdated Show resolved Hide resolved
@@ -70,7 +70,7 @@ parameters:
src/Gitea: '[email protected]:SocialiteProviders/Gitea.git'
src/Gitee: '[email protected]:SocialiteProviders/Gitee.git'
src/Goodreads: '[email protected]:SocialiteProviders/Goodreads.git'
src/Google: '[email protected]:SocialiteProviders/Google-Plus.git'
src/Google: '[email protected]:SocialiteProviders/Google.git'
Copy link
Member

Choose a reason for hiding this comment

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

Note: need to rename this repo

Choose a reason for hiding this comment

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

do you mean remove? Google+ has become redundant, no?

$jwt = $this->request->credential;
$certificates = $this->fetchCertificates();
$decoded = (array) JWT::decode($jwt, $certificates);
return $this->user = $this->mapUserToObject($decoded)
Copy link
Member

Choose a reason for hiding this comment

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

line break above please


protected function getAuthUrl($state)
{
// TODO: Implement getAuthUrl() method.
Copy link
Member

Choose a reason for hiding this comment

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

No need for these comments

*/
protected function fetchCertificates(): array
{
return collect(json_decode(file_get_contents('https://www.googleapis.com/oauth2/v1/certs'), true))->map(function ($key) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if getting this endpoint fails?

@atymic
Copy link
Member

atymic commented Sep 11, 2023

@CosminBd

@alinmiron
Copy link

@CosminBd Hey man, is it possible to handle this ?
Thanks!

@atymic
Copy link
Member

atymic commented Feb 7, 2024

@CosminBd

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.

5 participants