Skip to content

Conversation

@ajupov
Copy link

@ajupov ajupov commented Jun 8, 2018

Added provider for Odnoklassniki (https://ok.ru).

@kevinchalet
Copy link
Member

Thanks for your contribution. I'll take a look and merge it once 1.0 and 2.0 ship (next week).

@kevinchalet kevinchalet added this to the 2.1.0 milestone Jun 8, 2018
@mokeev1995
Copy link

@PinpointTownes Hey, will it be added in 3.0 or some other release?

@Vertigo093i
Copy link

It is already available here: https://github.com/Digillect/AspNetCoreOAuthProviders.


private static string GetMd5([NotNull] string value)
{
var md5 = MD5.Create();
Copy link
Member

Choose a reason for hiding this comment

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

MD5 implements IDisposable, so this needs to be in a using block.


private string PrepareUserInfoAddress([NotNull] string accessToken, [NotNull] string sig)
{
var address = QueryHelpers.AddQueryString(Options.UserInformationEndpoint, "application_key", Options.ApplicationKey);
Copy link
Member

Choose a reason for hiding this comment

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

Could use a dictionary like many of the other providers do and do this as a single operation?

@martincostello
Copy link
Member

Thanks for the PR - as part of the preparation for ASP.NET Core 3.0 support, tests are being added to help make things easier to maintain and validate going forwards (see #292).

Once tests are merged into the dev branch, could you copy the approach to add tests for the new provider into this PR please?

@martincostello
Copy link
Member

#280 has been merged to dev now. If you rebase and update any tests as appropriate, we can look at merging this PR soon.

@martincostello martincostello removed this from the 2.1.0 milestone May 24, 2019
@martincostello
Copy link
Member

Closing due to inactivity and merge conflicts - please re-open and rebase if you would like to pursue adding a provider for ok.ru in a future release.

@martincostello
Copy link
Member

martincostello commented Jun 1, 2019

Superseded by #311.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants