Shopify provider #207
Shopify provider #207
Conversation
| // Shopify OAuth differs from most (all?) others in that you need to know the host name of the | ||
| // shop in order to construct the authorization endpoint. This can be aquired either from the | ||
| // user directly, or provided by the shopify app store during app install/activation. | ||
| var authProps = new ShopifyAuthenticationProperties("ctest-205") // Put your shop name here. |
There was a problem hiding this comment.
Could you put all the Shopify-specific code into its own method and do if Shopify (do other thing)? Otherwise the code reads as it's mainly for Shopify, rather than the other way around.
There was a problem hiding this comment.
Hey Martin, sorry to have let this sit so long. I was diagnosed with leukemia about 3 days after I submitted the PR. I'm still only working half time and have about a year's worth of backlogged tasks. I really appreciate all your comments! I'll try to find some time for this asap.
There was a problem hiding this comment.
No problem, Chris - I've only recently become a maintainer myself, and I went through all the PRs and issues to either push them forwards or close them off.
A future update to re-implement this would be great if you manage find the time 😃
There was a problem hiding this comment.
I was diagnosed with leukemia about 3 days after I submitted the PR.
@ChrisInSeattle I'm really sorry to hear that. I wish you a speedy recovery. Take care of you! 🤞
|
|
||
| .AddShopify(options => | ||
| { | ||
| options.ApiKey = "9489b7555256142d9d723711c4614df7"; |
There was a problem hiding this comment.
Would you be OK with your keys living in the sample and being used by potentially anyone?
| <FileVersion>2.0.0.0</FileVersion> | ||
| <SignAssembly>false</SignAssembly> | ||
| <AssemblyOriginatorKeyFile>key.snk</AssemblyOriginatorKeyFile> | ||
| <Version>1.0.1</Version> |
There was a problem hiding this comment.
A lot of these properties are redundant as they defined centrally in the source code.
| <Version>1.0.1</Version> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'"> |
There was a problem hiding this comment.
Delete this PropertyGroup.
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="JetBrains.Annotations" Version="11.1.0" /> |
There was a problem hiding this comment.
Versions are defined centrally in MSBuild properties.
| /// <summary> | ||
| /// Claim valueType used for boolean claims. | ||
| /// </summary> | ||
| public const string XmlSchemaBoolean = "http://www.w3.org/2001/XMLSchema#boolean"; |
There was a problem hiding this comment.
This is a built-in framework constant, we shouldn't need to redefine it: https://github.com/dotnet/corefx/blob/e12bfba58e3581dc475e804688bef1c66d70226c/src/System.Security.Claims/src/System/Security/Claims/ClaimValueTypes.cs#L16
| /// <summary> | ||
| /// Claim valueType used for dateTime claims (iso format) | ||
| /// </summary> | ||
| public const string XmlSchemaDateTime = "http://www.w3.org/2001/XMLSchema#dateTime"; |
There was a problem hiding this comment.
This is a built-in framework constant, we shouldn't need to redefine it: https://github.com/dotnet/corefx/blob/e12bfba58e3581dc475e804688bef1c66d70226c/src/System.Security.Claims/src/System/Security/Claims/ClaimValueTypes.cs#L18
| /// <summary> | ||
| /// An alias for <see cref="OAuthOptions.ClientId"/> | ||
| /// </summary> | ||
| public string ApiKey |
There was a problem hiding this comment.
Do we really need these aliases?
|
|
||
| private void SetProperty(string propName, string value) | ||
| { | ||
| if (Items.ContainsKey(propName)) |
| /// to authorize. This must either be gotten from the user or sent from Shopify during App store | ||
| /// installation. | ||
| /// </param> | ||
| public ShopifyAuthenticationProperties(string shopName) |
There was a problem hiding this comment.
Constructor before properties for code consistency please.
|
Thanks for the PR @ChrisInSeattle - 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? It looks like it also needs re-basing with the |
|
#280 has been merged to |
|
Closing due to inactivity and merge conflicts - please re-open and rebase if you would like to pursue adding a provider for Shopify in a future release. |
I wrote a Shopify provider. My first try at this, sooo.. that's my excuse if there's anything wacky here.
Unfortunately, I had to pollute AuthenticationController.cs in the MVC sample to demo this. Shopify is wonky in that you need the shop host name before you can construct the authorization uri.
Thanks,
Chris