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: save gh email when login #25

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

thormengkheang
Copy link
Collaborator

fix #24

Copy link

vercel bot commented Mar 14, 2024

Someone is attempting to deploy a commit to a Personal Account owned by @invisal on Vercel.

@invisal first needs to authorize it.

@invisal
Copy link
Collaborator

invisal commented Mar 14, 2024

This only work if the user has their email public.
https://stackoverflow.com/questions/35373995/github-user-email-is-null-despite-useremail-scope

You need to make extra request to get the email https://api.github.com/user/emails

@thormengkheang
Copy link
Collaborator Author

please check again. I check if the email is not available will fetch email from the API

@rin-yato
Copy link
Collaborator

I think it might be better to group all route handlers into api/, otherwise we would be mixing pages and api routes together, this affect both the file structure and also the url.

I suggest this:

  • GET /login/github -> GET /api/auth/github
  • GET /login/github/callback -> GET /api/auth/github/callback

What do you think?

@rin-yato
Copy link
Collaborator

rin-yato commented Mar 14, 2024

I think it might be better to group all route handlers into api/, otherwise we would be mixing pages and api routes together, this affect both the file structure and also the url.

I suggest this:

  • GET /login/github -> GET /api/auth/github
  • GET /login/github/callback -> GET /api/auth/github/callback

What do you think?

Now that I look at it, it's out of scope for this issue, was bong @invisal the one who added the initial GITHUB auth?

Copy link
Collaborator

@rin-yato rin-yato left a comment

Choose a reason for hiding this comment

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

Inside scope of the issue, everything looks good to me!

@invisal
Copy link
Collaborator

invisal commented Mar 14, 2024

I think it might be better to group all route handlers into api/, otherwise we would be mixing pages and api routes together, this affect both the file structure and also the url.
I suggest this:

  • GET /login/github -> GET /api/auth/github
  • GET /login/github/callback -> GET /api/auth/github/callback

What do you think?

Now that I look at it, it's out of scope for this issue, was bong @invisal the one who added the initial GITHUB auth?

I am following the Lucia document. But I think it is fine, The /api folder should focus on API (which perform some action) and get some data. /login/github does not get any data. It is basically just doing the redirect.

Copy link

vercel bot commented Mar 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
libsql-viewer ⬜️ Ignored (Inspect) Visit Preview Mar 14, 2024 1:21pm

Copy link
Collaborator

@invisal invisal left a comment

Choose a reason for hiding this comment

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

LGTM

@invisal invisal merged commit 3d09c8b into outerbase:develop Mar 14, 2024
2 of 3 checks passed
invisal pushed a commit that referenced this pull request Mar 19, 2024
* add email column

* db migration

* save gh email to user table

* add user email scope

* fetch email when is not public
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.

Record Github email when login
3 participants