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: Add Naver To Provider #1251

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

Conversation

hoeseong19
Copy link

What kind of change does this PR introduce?

Add Naver to external provider

What is the current behavior?

Nothing. It's a new feature.

What is the new behavior?

The users can select Naver as an external provider.

Additional context

Naver is a Google-like portal site that supports social login for most services in Korea.
Naver has just as many users in Korea as Kakao, so I was surprised that there was no feature request to add Naver as a provider in the issues.
If Naver is added as an external provider, Koreans, including me, will be very happy.

@hoeseong19 hoeseong19 requested a review from a team as a code owner September 15, 2023 17:46
@hoeseong19 hoeseong19 changed the title Feat: Add Naver To provider Feat: Add Naver To Provider Sep 15, 2023
@hoeseong19
Copy link
Author

How to get client_id and client_secret can be found in the guide at the link.

However, NAVER DEVELOPERS, where the guide page is located, does not support English. (I don't really understand why either,,,)

On the following site, a blogger has recorded the process of using the NAVER Login API, which may be helpful. https://medium.com/@monsoonkhadka8/naver-login-api-interworking-php-in-english-df14b66c824e

If you need it, you need to request an official guide in English from the site.

@minyoon
Copy link

minyoon commented Nov 17, 2023

Can we please merge this PR?

  1. Naver is the biggest web service in South Korea: https://www.similarweb.com/website/naver.com/#overview
  2. NextAuth supports Naver Provider: https://next-auth.js.org/providers/naver

Copy link

@minyoon minyoon left a comment

Choose a reason for hiding this comment

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

Why do you need the merge commit (77797d3)?

A single feature commit should be sufficient for the PR: 3cc95b8.

Comment on lines +169 to +184
// func (ts *ExternalTestSuite) TestSignupExternalNaverErrorWhenVerifiedFalse() {
// tokenCount, userCount := 0, 0
// code := "authcode"
// emails := `[{"email":"[email protected]", "primary": true, "verified": false}]`
// server := NaverTestSignupSetup(ts, &tokenCount, &userCount, code, emails)
// defer server.Close()

// u := performAuthorization(ts, "naver", code, "")

// v, err := url.ParseQuery(u.Fragment)
// ts.Require().NoError(err)
// ts.Equal("unauthorized_client", v.Get("error"))
// ts.Equal("401", v.Get("error_code"))
// ts.Equal("Unverified email with naver", v.Get("error_description"))
// assertAuthorizationFailure(ts, u, "", "", "")
// }
Copy link

Choose a reason for hiding this comment

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

Please remove the commented-out code

Copy link
Author

Choose a reason for hiding this comment

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

Oops, I just commented it out.

I'll remove it.

@hoeseong19
Copy link
Author

Why do you need the merge commit (77797d3)?

A single feature commit should be sufficient for the PR: 3cc95b8.

That happened when I tried to keep the pull request up to date.

As you said, it's a commit unrelated to my pull request.

@LeoMilly
Copy link

LeoMilly commented Apr 6, 2024

is there any update on this? Koreans will be very happy if this PR is merged.

@inspirit941
Copy link

@minyoon any updates on this pr?

@minyoon
Copy link

minyoon commented Jul 12, 2024

@inspirit941 I am not affiliated with Supabase, so I cannot merge this PR.

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.

4 participants