adonisjs / ally

AdonisJS Social Authentication Provider
MIT License
159 stars 53 forks source link

Github driver doesn't take in account user with only a `login` and no `name` #150

Closed Bricklou closed 4 months ago

Bricklou commented 7 months ago

Package version

5.0.2

Describe the bug

Related to this discussion on discord: https://discord.com/channels/423256550564691970/785280284920643615/1228372633066799114 The user @kirs1748 has an issue related to the fact that he doesn't have any name on his profile, only a login.

It means that when he tries to log in, Ally will fetch a null name (since there is none configured) instead of fetching the login login. (https://docs.github.com/en/rest/users/users?apiVersion=2022-11-28#get-the-authenticated-user)

A possible fix would be to make field in Ally match like the following:

return {
    id: body.id,
    nickName: body.name,
    email: body.email, // May not always be there
    emailVerificationState: (body.email
      ? 'verified'
      : 'unsupported') as AllyUserContract<any>['emailVerificationState'],
-    name: body.name,
+    name: body.login,
    avatarUrl: body.avatar_url,
    original: body,
}

Reproduction repo

No response

RomainLanz commented 7 months ago

Hey @Bricklou! 👋🏻

If the name property can be undefined, if would be better to use name and fallback to login.

Something like:

{
  name: body.name ?? body.login
}

Do you have any documentation with the available fields?

Bricklou commented 7 months ago

I did some researches to understand what fields were returned, the best i managed to find was the response schema from my second link.

thetutlage commented 7 months ago

As @RomainLanz mentioned, a fallback to login seems reasonable in this case.

Bricklou commented 5 months ago

Has any fix already been done yet? Or should I open a Pull Request?

thetutlage commented 5 months ago

Please open a pull request