-
-
Notifications
You must be signed in to change notification settings - Fork 964
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: update Yandex and VK OIDC #4158
base: master
Are you sure you want to change the base?
Conversation
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.
Looks very good! Just one small question.
if len(userInfo.User.Phone) > 0 && userInfo.User.Phone[0] != '+' { | ||
userInfo.User.Phone = "+" + userInfo.User.Phone |
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.
🤔 I'm not sure if this is the correct normalization to do here. I quickly checked VK docs, but couldn't find anything about the format.
If it is not clear, I'd prefer to do nothing instead and leave it to the consumer of the data.
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.
All I've got is two examples from the docs, as well as a confirmation that it works that way (I've tested on my VK account that is registered with a Russian phone number). I didn't find any detailed description of the format either.
The examples from the docs:
{
"user": {
"user_id": "1234567890", // Идентификатор пользователя.
"first_name": "Ivan", // Имя пользователя.
"last_name": "I.", // Первая буква фамилии пользвателя.
"phone": "79903922415", // Телефон пользователя.
"avatar": "https://pp.userapi.com/60tZWMo4SmwcploUVl9XEt8ufnTTvDUmQ6Bj1g/mmv1pcj63C4.png", // Ссылка на фото профиля пользователя.
"email": "[email protected]", // Адрес почты пользователя.
"sex": 2, // Пол. Возможные значения: Возможные значения: 1 — женский, 2 — мужской, 0 — пол не указан.
"is_verified": false, // Признак того, что аккаунт пользователя подтвержден. Если это так, возвращается параметр true.
"birthday": "01.01.2000" // Дата рождения пользователя.
}
}
{
"user": {
"user_id": "1234567890",
"first_name": "Ivan",
"last_name": "I.",
"phone": "79991234567",
"avatar": "http://avatar.com/12345678",
"email": "[email protected]",
"sex": 2,
"verified": false,
"birthday": "01.01.2000"
}
}
79903922415
and 79991234567
are Russian phone numbers in the international format, but without a leading +
sign.
As far as I understand, Ory Kratos uses international format, but with a leading +
sign.
I'm not sure whether all the phones are returned in the same format or not. For example I can't test with non-Russian phones. This is why I added condition if there's no plus sign, then add it
.
If you insist, I can remove that code block (lines 162-164). It shouldn't be the problem to do the same action within a .jsonnet
file though.
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.
LGTM as well! I just wonder if we can enable PKCE for VK by default?
@@ -27,6 +27,7 @@ type Provider interface { | |||
type OAuth2Provider interface { | |||
Provider | |||
AuthCodeURLOptions(r ider) []oauth2.AuthCodeOption | |||
AccessTokenURLOptions(r *http.Request) []oauth2.AuthCodeOption |
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.
As this method is only relevant for VK, we could also extract it into a separate interface that only the VK provider implements. Then, in strategy.go
we assert on the interface and add the options if the provider implements it.
That way, we don't have to give empty implementations to every other provider. WDYT?
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.
I can add a new interface, but I need some directions. How to name this new interface?
I used the same interface because AuthCodeURLOptions
isn't in a separate interface either (there's empty implementations for this method too).
Endpoint: oauth2.Endpoint{ | ||
func (p *ProviderVK) oauth2(ctx context.Context) *oauth2.Config { | ||
var endpoint oauth2.Endpoint | ||
if p.config.PKCE == "force" { |
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.
Is there a reason / use case for not using PKCE with VK? Ideally, we would use PKCE by default for all providers that support it.
The explicit config flag for PKCE should only be necessary for the generic provider.
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.
I can enable PKCE for pkce == "auto"
too. I thought auto
should enable PKCE only if /.well-known/openid-configuration
returns that it's supported, that's why I didn't enable PKCE for auto
.
I don't know the reason why we shouldn't use PKCE. But enabling non-PKCE configuration when pkce
is explicitly set to never
seems logical.
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.
I just remembered the reason why PKCE shouldn't be enabled by default.
Ory Kratos uses different URLs for PKCE and non-PKCE callbacks:
/self-service/methods/oidc/callback/vk
(PKCE disabled)/self-service/methods/oidc/callback
(PKCE enabled)
If we enable PKCE by default, the clients that were created before this PR is merged won't work (because they were created with URL /self-service/methods/oidc/callback/vk
)
Changes
VK provider: Support PKCE
It seems like VK doesn't provide
/.well-known/openid-configuration
(VK doesn't support OpenID).So to use PKCE it's needed to be enabled with:
VK provider: Support returning
phone_number
inclaims
VK offers different endpoints for PKCE and non-PKCE configuration.
Token endpoint that is used in non-PKCE configuration can return
email
but notphone
.Extra endpoint
https://id.vk.com/oauth2/user_info
can be used to retrieveemail
andphone
, but it doesn't acceptaccess_token
that was returned from non-PKCE token endpoint.So to retrieve a phone, PKCE is required to be enabled:
Additional info:
I added
AccessTokenURLOptions(r *http.Request) []oauth2.AuthCodeOption
to OAuth2Provider interface.It works like
AuthCodeURLOptions(r ider) []oauth2.AuthCodeOption
but it's dedicated to update a token URL.VK requires passing
device_id
URL param (that is received by a callback) to PKCE token endpoint. Without this change it's impossible.I updated all the other OIDC providers to comply with the updated interface.
Yandex provider: Support returning
phone_number
inclaims
No extra configuration is needed.
Related issue(s)
Fixes: #4147
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments