Implement VK ID OAuth2 provider #962
Conversation
HScokies
force-pushed
the
provider/vk-id
branch
from
d2d0c66
to
5f5fa20
Compare
…bled InvertIf for ExchangeCodeAsync
HScokies
force-pushed
the
provider/vk-id
branch
2 times, most recently
from
1728088
to
96f572f
Compare
HScokies
force-pushed
the
provider/vk-id
branch
from
96f572f
to
e482f8e
Compare
| var dataProtector = (IDataProtector)DataProtectionProvider.Create("test"); | ||
| var fakeDataProtector = Substitute.For<IDataProtector>(); | ||
|
|
||
| fakeDataProtector.Protect(Arg.Any<byte[]>()) | ||
| .Returns(x => dataProtector.Protect(x.Arg<byte[]>())); | ||
| fakeDataProtector.Unprotect(Arg.Is<byte[]>(x => !x.SequenceEqual(Array.Empty<byte>()))) | ||
| .Returns(x => dataProtector.Unprotect(x.Arg<byte[]>())); | ||
|
|
||
| // Use fake DP with empty AuthenticationProperties for ExchangeCodeAsync state validation | ||
| fakeDataProtector.Unprotect(Arg.Is<byte[]>(x => x.SequenceEqual(Array.Empty<byte>()))) | ||
| .Returns([1, 0, 0, 0, 0, 0, 0, 0]); |
There was a problem hiding this comment.
Could you make this use a slightly more realistic value that matches a non-empty scope value in the payload?
| } | ||
|
|
||
| // OAuth2 10.12 CSRF | ||
| if (ValidateCorrelationId(properties) is false) |
There was a problem hiding this comment.
It's not technically wrong, but !ValidateCorrelationId(properties) would be easier to read (and consistent with the other providers).
| var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync(Context.RequestAborted)); | ||
|
|
||
| // Code exchange response should always contain state parameter. | ||
| if (!payload.RootElement.TryGetProperty("state", out var state) || |
There was a problem hiding this comment.
state in token responses is not really a standard thing and it's not clear what security benefits validating the state here provides. I'd just remove that part to simplify things.
| [NotNull] AuthenticationProperties properties, | ||
| [NotNull] string redirectUri) | ||
| { | ||
| var parameter = Options.Scope; |
There was a problem hiding this comment.
Looks like these temp variables aren't very useful: feel free to remove them and do the formatting directly in the dictionary initializer to save a few lines.
|
(I find it funny that VK claims they are implementing "OAuth 2.1" - that hasn't been officially adopted yet - when all they're doing is implementing yet another non-standard version of OAuth with non-standard concepts like HTTP 200 errors and |
|
yea, VK is a funny company... |
There was a problem hiding this comment.
Looks good, thanks a lot for your contribution! 👍🏻
I'll let @martincostello merge it in case he'd have additional feedback 😃
|
Before merging this, can you confirm you've tested the provider against the production VK ID service and that you were able to successfully authenticate? |
|
Oh, and could you update this to AspNet.Security.OAuth.Providers/eng/Versions.props Lines 5 to 6 in 8bff342 |
Everything seems to work fine, yes |
|
Thanks again for your contribution - the new provider 📦 is now available from NuGet.org: https://www.nuget.org/packages/AspNet.Security.OAuth.VkId/8.3.0 |
Implement VK ID OAuth2.