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.

Sign up for GitHub

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

Jump to bottom

feat(core/providers): Add Vipps Login Provider #6713

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat(core/providers): Add Vipps Login Provider #6713

wants to merge 8 commits into from

Conversation

itschip
Copy link

@itschip itschip commented Feb 13, 2023

NOTE:

  • It's a good idea to open an issue first to discuss potential changes.
  • Please make sure that you are NOT opening a PR to fix a potential security vulnerability. Instead, please follow the Security guidelines to disclose the issue to us confidentially.

☕️ Reasoning

This PR adds Vipps Login API to next-auth. One of the most popular login methods used in Norway.

Completes #6424 (Credits to @endrekrohn for the initial PR)

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

None

📌 Resources
@vercel
Copy link

vercel bot commented Feb 13, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 12, 2023 9:52am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Apr 12, 2023 9:52am
@github-actions github-actions bot added core Refers to `@auth/core` providers labels Feb 13, 2023
ThangHuuVu
ThangHuuVu requested changes Feb 13, 2023
View reviewed changes
Copy link
Member

@ThangHuuVu ThangHuuVu left a comment

Choose a reason for hiding this comment

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

Thanks, I added some comments. Besides, could you add an option to the issue template? 🙏
packages/core/src/lib/oauth/callback.ts Show resolved Hide resolved
packages/core/src/providers/vipps.ts Show resolved Hide resolved
packages/core/src/providers/vipps.ts Outdated
}

interface AdditonalConfig {
redirectUri?: string
Copy link
Member

Choose a reason for hiding this comment

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

This isn't used anywhere
ThangHuuVu
ThangHuuVu reviewed Feb 14, 2023
View reviewed changes
packages/core/src/providers/vipps.ts Outdated
Comment on lines 55 to 56
client_id: options.clientId,
response_type: "code",
Copy link
Member

Choose a reason for hiding this comment

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

client_id and response_type is automatically added, so I think you can remove them here
packages/core/src/providers/vipps.ts
response_type: "code",
},
},
userinfo: {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant you could use the wellKnown option with idToken: true, described here: https://authjs.dev/reference/providers/oauth#userinfo-option
since this is an OIDC-compliant provider, we should be able to extract the user info without making a request to that endpoint
Copy link
Author

@itschip itschip Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Last time I checked idToken wasn't a type on OIDCConfig. Should I add it?

I read that next-auth still will try and contact said endpoint tho
packages/core/src/providers/vipps.ts
return await response.json()
},
},
profile(profile) {
Copy link
Member

Choose a reason for hiding this comment

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

profile isn't needed afaik
@itschip
Copy link
Author

itschip commented Feb 14, 2023

Ok, so after ace052b I seem to get the decoded id_token as the OAuthProfile. Is that correct?
@itschip
Copy link
Author

itschip commented Feb 14, 2023 •
edited
Loading

However, if I bring back the changes from ab8da6c

as well as including a custom request for the userinfo and adding the profile function back, I get "actual" profile data with email, name..etc

@balazsorban44 If I may ask, what was the reason for 740a2db, as that is where I took it from. Is it needed for Vipps too you think?
balazsorban44
balazsorban44 reviewed Feb 18, 2023
View reviewed changes
Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

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

I would like to test this locally before merging. I'm currently struggling with Vipps Support to check why my redirect url is not accepted. 🙏

The Vipps provider is a standard OIDC provider, so userinfo.request should be unnecessary as it's reserved for non-compliant providers as an escape hatch. I'm looking into this. 👍
@itschip
Copy link
Author

itschip commented Feb 18, 2023

I would like to test this locally before merging. I'm currently struggling with Vipps Support to check why my redirect url is not accepted. 🙏

The Vipps provider is a standard OIDC provider, so userinfo.request should be unnecessary as it's reserved for non-compliant providers as an escape hatch. I'm looking into this. 👍

Thank you! Let me know if I can help you with anything :)
@itschip
Copy link
Author

itschip commented Mar 7, 2023

@balazsorban44 did you resolve your issue with the redirect url? :)
@balazsorban44 balazsorban44 added the enhancement New feature or request label Mar 24, 2023
@vercel
Copy link

vercel bot commented Apr 2, 2023

@itschip is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.
@itschip
Copy link
Author

itschip commented Apr 2, 2023

@balazsorban44 Do you think it is fine to just use 'oauth' for now. I am currently using that for my project now, and it works fine. If not I'll try once more with 'oidc', even though I have not had any luck with it yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Refers to `@auth/core` enhancement New feature or request providers
4 participants
@itschip @balazsorban44 @ThangHuuVu @Gregor23021