-
Notifications
You must be signed in to change notification settings - Fork 550
Add etsy oauth provider #1126
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
base: dev
Are you sure you want to change the base?
Add etsy oauth provider #1126
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.
Pull Request Overview
This PR adds a new Etsy OAuth 2.0 authentication provider to the AspNet.Security.OAuth.Providers library. The implementation supports Etsy's Authorization Code flow with PKCE, personal access type (public client), and optional detailed user information fetching.
- Implements complete OAuth 2.0 flow with PKCE requirement
- Supports optional detailed user profile enrichment via configurable flag
- Includes comprehensive validation for required scopes and configuration
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationOptions.cs |
Configuration options with validation logic for Etsy OAuth requirements |
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationHandler.cs |
OAuth handler implementing token exchange and user info retrieval |
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationExtensions.cs |
Extension methods for registering Etsy authentication |
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationDefaults.cs |
Default endpoint URLs and configuration values |
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationConstants.cs |
Claim types and scope constants |
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationAccessType.cs |
Enum defining access types (Personal) |
test/AspNet.Security.OAuth.Providers.Tests/Etsy/EtsyTests.cs |
Integration tests for authentication flow and claims |
test/AspNet.Security.OAuth.Providers.Tests/Etsy/EtsyAuthenticationOptionsTests.cs |
Unit tests for options validation |
test/AspNet.Security.OAuth.Providers.Tests/Etsy/bundle.json |
Test fixture data for HTTP interception |
docs/etsy.md |
Comprehensive usage documentation with examples |
AspNet.Security.OAuth.Providers.sln |
Solution file updated with new project references |
Comments suppressed due to low confidence (2)
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationHandler.cs:51
- This assignment to userId is useless, since its value is never read.
var userId = meRoot.GetProperty("user_id").GetInt64();
src/AspNet.Security.OAuth.Etsy/EtsyAuthenticationHandler.cs:52
- This assignment to shopId is useless, since its value is never read.
var shopId = meRoot.GetProperty("shop_id").GetInt64();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/AspNet.Security.OAuth.Etsy/AspNet.Security.OAuth.Etsy.csproj
Outdated
Show resolved
Hide resolved
test/AspNet.Security.OAuth.Providers.Tests/Etsy/EtsyAuthenticationOptionsTests.cs
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ad60b5b to
36cacb2
Compare
|
Thanks for responding to the various comments - I'll take a look at this again when I get the time. Shipping the release for .NET 10 was the main priority this week. |
|
@martincostello thanks for your response and time! I am not sure in this, because I have had lots of trouble with implementing oAuth in there, because of Antiforgery and Cors 😓 the Official MS Docs for oAuth2 are horribly lacking on guidiance when there is no exiting provider like Google or something OIDC that supports discovery endpoint, which the Etsy API doesn't have 👀 We do have the Auth Code Exchange implemented here, which is against the Cross Origin Forgery Attacking 🤔 And in the Authentication/Authorization docs for minimal api here:
Its not told in our Readme files, if we do need Cookie to be added, or if the oAuth Base Extension our Providers here are building on do this already for us by default. In case we need the dev user (like me then) to add this for e.g. have something like the Refreshing of the Token implemented (which I seen in none of the existing Providers here (?), so did not add it in the Etsy Provider too) eventually it would be usefull to add a Link to the Cookie Auth ms docs eventually to the OIDC docs which contains a auth flow diagram with BUT while on the social login providers docs do link to the External Providers Page, which also links to this repo (thats how I found it 😉) this would be missing information about Refreshing Tokens too 🤷 If you would like me to open another issue / PR for this to improve these spots, let me know 👍 |
|
You should be able to use our MVC sample App to test the changes locally. |
|
@martincostello Hey, I am on trying the provider with the [HttpPost("~/signin")]
public async Task<IActionResult> SignIn([FromForm] string provider)
{
// Note: the "provider" parameter corresponds to the external
// authentication provider chosen by the user agent.
if (string.IsNullOrWhiteSpace(provider))
{
return BadRequest();
}
if (!await HttpContext.IsProviderSupportedAsync(provider))
{
return BadRequest();
}
// Instruct the middleware corresponding to the requested external identity
// provider to redirect the user agent to its own authorization endpoint.
// Note: the authenticationScheme parameter must match the value configured in Startup.cs
return Challenge(new AuthenticationProperties { RedirectUri = "/" }, provider);
}Could you maybe take a quick look and let me know if I just misunderstood this and the CallbackPath set in Options can be used as expected?
the Value I would like to set is e.g. |
|
|
|
@martincostello 🎉 🥳 Worked!!! just that the |
martincostello
left a 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.
I haven't fully reviewed this PR yet, or thought about responding to the specific questions yet, but I've done a quick pass. I'll come back to this another time next week.
src/AspNet.Security.OAuth.Etsy/AspNet.Security.OAuth.Etsy.csproj
Outdated
Show resolved
Hide resolved
|
Please resolve merge conflicts - the |
- Added `EtsyAuthenticationDefaults` default value fields like `EtsyBaseUri` and `UserDetailsPath` - Added xml docs onto all `EtsyAuthenticationDefaults` fields - Added `EtsyAuthenticationOptions.IncludeDetailedUserInfo` Property - removed `EtsyPostConfigureOptions` as Etsy does not have dynamic endpoints or domain like GitHub - integrated validation logik into `EtsyAuthenticationOptions.Validate` override, like used for issue 610 fix
…d IncludeDetailedUserInfo Handler
…aming for the app Access level chore: applying PR rewording suggestion Co-authored-by: Martin Costello <[email protected]> chore: remove comments Co-authored-by: Martin Costello <[email protected]>
…are not needed chore(EtsyAccessType): Remove AccessType
Co-authored-by: Martin Costello <[email protected]>
there is no endpoint this could get from and as its in the table format in etsy api, it gets also not inserted into the api spec .json for CLI tools like Kiota
…y CA1863 suggestions, apply xml docs changes
…Configuration and add Extension for simpler ImageUriClaim
…d code changes in Etsy Provider
…k to defaults in handler
- openapi instead of just api - reverted formated string in bundle.json
… const strings could be used in attributes, not static readonly strings
User side Post configure is later than provider side post configure. this causes the Scope not to be evaluated and the Mappings are not applyed automatically HACK: add them all manually for the test postConfigure until decision is made this could be put in validate override or always consumer side?
TODO: Should be checked, we still need the full methods when using the attribute LoggerMessage. Maybe we can delete the non attribute using method then?
…e docs docs(PostConfigure): Add Warning that he needs to add the claims himself for detailed user info if he wants to use `PostConfigure` himself, which runs after the Provider side chore: apply PR reword suggestions chore: Update Etsy.md TOC chore: Apply PR suggestions
Co-authored-by: Copilot <[email protected]>
1fcdc17 to
b855e84
Compare
|
@martincostello Resolved Merge Conflicts + cleaned commits up a bit |
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.
Thanks for being so comprehensive, but I think there's a little too much detail here from a maintenance perspective.
A lot of the content here should be evident from reading ASP.NET Core OAuth documentation and Etsy's own documentation (e.g. the various setup steps needed in the Etsy developer portal).
Typically we just document the bare minimum required to set things up with a code snippet, highlighting any optional code (example), point out anything that's unusual compared to other providers, and document the purpose of any non-optional settings that aren't part of the underlying OAuthHandler from ASP.NET Core (for example, we don't typically document UsePkce, SaveTokens or CallbackPath as they aren't our properties).
For example, mapping additional claims isn't specify to Etsy, it can be done with any of the providers.
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.
@martincostello the etsy api has not very much information as they are only telling about JavaScript way to do this and even this is known to be outdated + incomplete. They did at least add the /users/me endpoint in the past, so the users will any longer have to inspect the raw html to get their shop ID 😅
What I am not sure about, can you tell me, if you know any reference, what the default paths in Cookie Auth are? I did not find them honestly. The MVC.Client Sample does specify them, but the ASP.NET Core Docs are not clear about this and never mapping them 🤔
Refering to the Minimal API quick Reference"](https://learn.microsoft.com/de-de/aspnet/core/fundamentals/minimal-apis?view=aspnetcore-10.0#authorization) or [Use cookie authentication without ASP.NET Core Identity`, I did not find this information, so while I was trying to set the provider up in my own API, which uses Minimal API, I have no idea, if I now need to explicitly register it or if not need to 😅
Same for implementing Refresh Token 🤔
- Do we need in ASP.NET Core to have some kind of
TokenRefreshHandlerover the here shown claims actions in the Consumer WebApi from your knowledge or does the Framework handle this automatically? - If it does not, but I potentially just didn't find what the Name of the Event or Method is, we would need to call and the social Auth guide in ASP.NET Core (which would be those providers here too?) only tells about:
- "Apply Authorization" (using MVC, no minimal API) by only adding the attribute, no sample content at all so assuming we might be fine with calling this
ChallengeAsynclike the Mvc client does? - Then
Save Tokensby defining a routeOnGetAsync()which I dont see in our sample (any additionals to be known for our providers repo I missed?) 🤔 - And then it's just telling about Sign Out which endpoint we also have in our sample, BUT if I click on the Logout Button in it the comment would tell me, the cookie with the
access_tokenorrefresh_tokenwe eventually automatically stored, does get deleted (?) so I should be asked to approve sign in of this app again if I click sign in, and not have to delete the cookies for Etsy before it prompts me again 🤔 - BUT their
SocialSampledoes define such route 👀
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.
@martincostello updated guide, would you check if this fits now?
src/AspNet.Security.OAuth.Etsy/AspNet.Security.OAuth.Etsy.csproj
Outdated
Show resolved
Hide resolved
… of CompositeFormat
…ostConfiguration and validate with tests
9550b58 to
5b80a6b
Compare

This pull request adds initial support for Etsy OAuth authentication to the codebase. It introduces the core implementation for an ASP.NET Core authentication provider, including configuration, constants, and documentation. The changes provide all the necessary building blocks and guidance for integrating Etsy login into an ASP.NET Core application.
Etsy OAuth Provider Implementation
AspNet.Security.OAuth.Etsy.csprojto define the package and its dependencies for the Etsy authentication provider.EtsyAuthenticationAccessTypeenum to distinguish between personal and commercial access types for Etsy apps.EtsyAuthenticationConstantsclass with claim and scope constants for mapping Etsy user data and permissions.EtsyAuthenticationDefaultswith all the required default values for endpoints, scheme names, and callback paths used by the Etsy authentication middleware.EtsyAuthenticationExtensionsto make it easy to add Etsy authentication to an ASP.NET Core application via the authentication builder.Documentation
docs/etsy.md, detailing configuration, required/optional settings, claims mapping, and sample code for integrating Etsy OAuth authentication.Note
The AccessType enum does so far only have one member
Personalbecause I only have this level of App registration and can not see or tell if the commercial Access Apps do somehow support confidential clients against the guidelines of the Etsy Authentication API. I asked their support some while ago which did tell that theshared secretwould be used at refresh, but there is no note at all for this in their docs, so this provider is adhering to the existing docs and oAuth standards RFC 6749 and does treat it as Public Client.Recognizing the #610 Issue in the past, I directly implemented the same fix up from the start, because even if the user sets the value, we don't have to require it, so the provider should also not fail because of not setting this.
Closes: #1082