Skip to content

Conversation

@DevTKSS
Copy link

@DevTKSS DevTKSS commented Nov 7, 2025

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

  • Added the main project file AspNet.Security.OAuth.Etsy.csproj to define the package and its dependencies for the Etsy authentication provider.
  • Introduced EtsyAuthenticationAccessType enum to distinguish between personal and commercial access types for Etsy apps.
  • Added EtsyAuthenticationConstants class with claim and scope constants for mapping Etsy user data and permissions.
  • Defined EtsyAuthenticationDefaults with all the required default values for endpoints, scheme names, and callback paths used by the Etsy authentication middleware.
  • Implemented extension methods in EtsyAuthenticationExtensions to make it easy to add Etsy authentication to an ASP.NET Core application via the authentication builder.

Documentation

  • Added a comprehensive setup and usage guide in 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 Personal because 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 the shared secret would 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

Copilot AI review requested due to automatic review settings November 7, 2025 11:24
Copy link

Copilot AI left a 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.

@DevTKSS DevTKSS requested a review from Copilot November 8, 2025 00:28
Copy link

Copilot AI left a 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.

@DevTKSS DevTKSS force-pushed the add-etsy-oauth-provider branch from ad60b5b to 36cacb2 Compare November 9, 2025 18:04
@martincostello
Copy link
Member

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.

@DevTKSS
Copy link
Author

DevTKSS commented Nov 12, 2025

@martincostello thanks for your response and time!
Until you find time, do you think from what you seen and the tests implemented by now, I could use the produced .dll locally in a ASP.NET Core API project to do an actual test call? 🤔

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 🤔
I would currently miss the information, if we then still need specific setup content for Antiforgery additionally to what's told in our:

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 code and code_verifyer or at least to Social Providers Auth ?

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 👍

@martincostello
Copy link
Member

You should be able to use our MVC sample App to test the changes locally.

@DevTKSS
Copy link
Author

DevTKSS commented Nov 21, 2025

@martincostello Hey, I am on trying the provider with the samples/Mvc.Client but a bit confused how and where to set the Callback Url, as the Etsy API requires me to pre-provide one or more Callback Url's, no dynamic Url, no dynamic Port etc.
The Options we configure in the IOptions loaded appsettings or User Secrets for the ClientId, we can provide the CallbackPath, but seems like the Mvc.Client is overwriting it? 👀

    [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?
I am not getting the exact difference between them I guess...

  • CallbackPath -> EtsyAuthenticationOptions
  • RedirectUri -> Mvc.Client AuthenticationProperties

the Value I would like to set is e.g. https://localhost:8080/etsy/callback

@martincostello
Copy link
Member

CallbackPath is where the OAuth provider will redirect the user after authenticating so that an authenticated session can be created for your application by the authentication middleware. For your example, you would set this to "etsy/callback".

RedirectUri is the URL the user is directed to in your website once they have successfully authenticated and been signed in.

@DevTKSS
Copy link
Author

DevTKSS commented Nov 21, 2025

@martincostello 🎉 🥳 Worked!!! just that the CallbackPath needed to be /callback/etsy with the / before
image

Copy link
Member

@martincostello martincostello left a 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.

@martincostello
Copy link
Member

Please resolve merge conflicts - the .sln file has been removed and replaced with an .slnx file.

DevTKSS and others added 15 commits November 27, 2025 14:09
- 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
…aming for the app Access level

chore: applying PR rewording suggestion

Co-authored-by: Martin Costello <martin@martincostello.com>

chore: remove comments

Co-authored-by: Martin Costello <martin@martincostello.com>
…are not needed

chore(EtsyAccessType): Remove AccessType
Co-authored-by: Martin Costello <martin@martincostello.com>
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
DevTKSS and others added 8 commits November 27, 2025 14:27
- 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 <175728472+Copilot@users.noreply.github.com>
@DevTKSS DevTKSS force-pushed the add-etsy-oauth-provider branch from 1fcdc17 to b855e84 Compare November 27, 2025 13:31
@DevTKSS
Copy link
Author

DevTKSS commented Nov 27, 2025

@martincostello Resolved Merge Conflicts + cleaned commits up a bit
I would have squashed some more/reworded, but the sln->slnx migration crashes git rebase -i so hope it's okay like this?

@DevTKSS DevTKSS force-pushed the add-etsy-oauth-provider branch from 9550b58 to 5b80a6b Compare December 2, 2025 17:36
Comment on lines +57 to +58
// Extract user_id from the /me response required to get detailed user info. shop_id is mapped later via ClaimActions
var userId = meRoot.GetProperty("user_id").GetInt64();
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this only needs to be done if Options.IncludeDetailedUserInfo is true.

Copy link
Author

Choose a reason for hiding this comment

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

@martincostello yes it is, but if we need to run the ClaimActions in Line 64 before the conditional (could you check if we need to?) I would not exactly know how to do this, except from you want me to make another if statement for it?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I don't follow - the value is read and stored into a local variable and only used within that block, so if it isn't entered it's a redundant read.

Are you saying that if it wasn't read here but later that would cause a bug? If so, then that makes it seem like there's "magic" happening, which is itself a problem if the way the provider function isn't immediately clear from inspecting the code in the method.

Copy link
Author

Choose a reason for hiding this comment

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

😅 no it isn't doing magic from what I understood from the mappings. The userId is used in the detailedUserInfo block some lines below, to fetch this if requested. 🤔
You remember the /users/{user_id}? This is what we need it for in the additional User details.

But it is possible that there is outdated code and the checks for the ClaimActions Types below is missing the userId which we do duplicate store in nameIdentity + Constant Map to urn:Etsy:user_id what I don't know is, if the RunClaimActions here before this is needed 🤔 if not than we might be able to only ask for the userId value once inside of the if statement

Copy link
Author

Choose a reason for hiding this comment

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

😅 no it isn't doing magic from what I understood from the mappings. The userId is used in the detailedUserInfo block some lines below, to fetch this if requested. 🤔
You remember the /users/{user_id}? This is what we need it for in the additional User details.

But it is possible that there is outdated code and the checks for the ClaimActions Types below is missing the userId which we do duplicate store in nameIdentity + Constant Map to urn:Etsy:user_id what I don't know is, if the RunClaimActions here before this is needed 🤔 if not than we might be able to only ask for the userId value once inside of the if statement

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm still not following your reply here.

The local variable userId is only used on line 69, which is after an if. Therefore the declaration and assignment of the variable can be moved from here to line 68.

if (Options.IncludeDetailedUserInfo)
{
    var userId = meRoot.GetProperty("user_id").GetInt64();
    using var detailedPayload = await GetDetailedUserInfoAsync(tokens, userId);
}

/// <returns>A <see cref="JsonDocument"/> containing the detailed user information.</returns>
protected virtual async Task<JsonDocument> GetDetailedUserInfoAsync([NotNull] OAuthTokenResponse tokens, long userId)
{
var userDetailsUrl = $"{Options.DetailedUserInfoEndpoint}{userId}";
Copy link
Member

Choose a reason for hiding this comment

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

Feels prudent to handle if DetailedUserInfoEndpoint doesn't end in a / here.

Copy link
Author

Choose a reason for hiding this comment

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

@martincostello not sure what you mean by "prudent" sorry 😅 do you mean that we need to add a validation like .EndsWith('/') or what do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

If DetailedUserInfoEndpoint was overridden and the user forgot to add the trailing /, then the URL would be malformed.

Comment on lines 49 to 51
/// <remarks>
/// The placeholder for <c>user_id</c> needs to be <c>"{0}"</c> and will be replaced with the authenticated user's ID.
/// </remarks>
Copy link
Member

Choose a reason for hiding this comment

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

This is outdated.

@martincostello martincostello changed the title Add etsy oauth provider Add Etsy OAuth provider Dec 9, 2025
DevTKSS and others added 4 commits December 9, 2025 18:21
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add Etsy oauth2

2 participants