-
-
Notifications
You must be signed in to change notification settings - Fork 546
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
Add Yandex and VK ID web providers #2244
Conversation
2. Attach client_id for VkId web provider throw user info proccess
2. Change exception text for empty device_id
Hey, Thanks for your PR. Were you able to confirm both providers are working fine? Can you please post a screenshot of the resulting claims table for both providers? (feel free to blur the most sensitive details) Cheers. |
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.Userinfo.cs
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationProviders.xml
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationProviders.xml
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationProviders.xml
Outdated
Show resolved
Hide resolved
…ationHandlers.Userinfo.cs Co-authored-by: Kévin Chalet <[email protected]>
…ationHandlers.cs Co-authored-by: Kévin Chalet <[email protected]>
…ationProviders.xml Co-authored-by: Kévin Chalet <[email protected]>
…ationProviders.xml Co-authored-by: Kévin Chalet <[email protected]>
…ationProviders.xml Co-authored-by: Kévin Chalet <[email protected]>
I sent an invitation to the DefaultIdentityDict test project to check the driver's performance. |
Documentation="https://yandex.ru/dev/id/doc/en/"> | ||
<Environment Issuer="https://oauth.yandex.ru/"> | ||
<Configuration AuthorizationEndpoint="https://oauth.yandex.ru/authorize" | ||
RevokationEndpoint="https://oauth.yandex.ru/revoke_token" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming Yandex's documentation is correct, their endpoint is not a standard OAuth 2.0 revocation endpoint: it uses an access_token
parameter instead of the standard token
(see https://yandex.ru/dev/id/doc/en/tokens/token-invalidate).
Can you please try with the sandbox console app to see if it works or not? If it doesn't, I'll add an event handler to take care of the standard -> non-standard parameter mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you give it a try? 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, the token update method returned an error
Ah yeah, sorry: that's because the two <GrantType>
nodes are not under <Configuration>
(they are currently under <Environment>
, which is not valid). Fix that and the error should go away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VK ID doesn`t allow any ports except 443.
According to their documentation, it seems they also allow HTTP port 80 during development:
You can force the OpenIddict.Sandbox.Console.Client
sample to listen on specific ports by using options.UseSystemIntegration().SetAllowedEmbeddedWebServerPorts(80)
. If you get an error, make sure the port is not busy/taken by a different app when starting the demo app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the authentication property as mentioned in the message doesn't work? See #2244 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my mistake. All works fine👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if (context.Registration.ProviderType is ProviderTypes.VkId)
{
context.TokenRequest["device_id"] = context.GrantType switch
{
GrantTypes.AuthorizationCode or GrantTypes.Implicit => context.Request["device_id"],
_ when context.Properties.TryGetValue(VkId.Properties.DeviceId, out string? identifier) &&
!string.IsNullOrEmpty(identifier) => identifier,
_ => throw new InvalidOperationException(SR.GetResourceString(SR.ID0467))
};
}
Hmmm, for now this code make an exception. For refresh method only in VK ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, for now this code make an exception. For refresh method only
You added #2244 (comment) so the device identifier is attached as an authentication property, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's all ok. Sorry please, i use previous version - this missed. I checked everything again, all works fine 👍
Hum, VK ID wraps userinfo responses in a Lines 409 to 412 in a18433d
Note: since these providers don't use the standard OIDC userinfo claims, you'll also need to update this handler to map the claims: openiddict-core/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs Lines 1292 to 1541 in a18433d
|
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs
Outdated
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationProviders.xml
Show resolved
Hide resolved
src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationProviders.xml
Show resolved
Hide resolved
…sole app 2. Update code review changes
@kevinchalet, thanks for yor patience) and amazing OpenIddict library. |
Haha, no problem. Thanks for your contribution!
Thanks. Unfortunately, I don't have a VK ID or Yandex account, so I won't be able to test it myself. Can you please post a screenshot of the claims table for each provider? (feel free to hide sensitive values) Please also test the token refreshing scenario for both providers and token revocation for Yandex. |
Note: don't worry about the git conflict, I'll take care of it when merging the PR. |
Help me please with revoke token action. How can i check it? I check this issue, but this take no effect. TokenManager.RevokeByAuthorizationIdAsync returns 0 and i stay login. |
You can test both token refreshing (your 2 screenshots are the initial authentication, not a token refreshing) and token revocation using the |
Merged! Thanks a lot for your great contribution and the time you spent testing my commits 😁 With these 2 new providers, OpenIddict now supports (exactly) 100 web services! 🎉 |
My big thanks to you for support 🥇 |
Hello!
Added support for VK ID and for Yandex
API documentation for VK ID is available at this link
API documentation for Yandex is available at this link