Skip to content

Conversation

@gehongyan
Copy link
Contributor

This pull request would like to add osu! to the list of supported providers.

Copilot AI and others added 2 commits December 8, 2025 12:24
* Initial plan

* Add osu! OAuth 2.0 provider to OpenIddict WebIntegration

Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>

* Fix osu! provider: add client_credentials scope, mode parameter, remove defaults

Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>

* Reorder osu! handlers to follow alphabetical order by ProviderTypes

Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>

* Fix OSU! ASCII art banner to correctly spell the provider name

Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>

* Update osu! banner and add default mode description

Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>

* Use mode as path parameter in UserInfo endpoint URL

Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>

* Wrap ternary expression in parentheses for C# template string

Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: gehongyan <21241496+gehongyan@users.noreply.github.com>
Signed-off-by: Gehongyan <gehongyan1996@126.com>
@gehongyan
Copy link
Contributor Author

Docs

Hints

  • The Client Credentials Grant requires the scope parameter with the value public.

Note

Question 1: Is it proper to force setting it in the AttachAdditionalTokenRequestParameters?

  • The user info endpoint supports an optional mode parameter. osu! has four game modes: osu!standard, osu!taiko, osu!catch, and osu!mania (in the commonly used order). In osu! API, catch is called fruit because this mode lets players catch fruits, and the game mode is called Ruleset.

Note

Question 2: Which naming do you prefer for parameter and constant names: GameMode, Mode, or Ruleset? For the osu!catch constant name, do you prefer Catch (more familiar) or Fruits (consistent with the API)? For ordering, do you prefer the familiar/custom order or switching to alphabetical order?

Screenshots

  • OAuth App Managing
image
  • Authentication Page
image
  • Denying Result
image
  • Allowing Result (very long, with some flattened arrays)
image image
  • Refreshing
image
  • Client Credentials Granting Result
image

@kevinchalet
Copy link
Member

Hey @gehongyan! Thanks for this new PR! 👏🏻

Question 1: Is it proper to force setting it in the AttachAdditionalTokenRequestParameters?

That definitely works, tho' we could also add a new dedicated OverrideScopes event handler in OpenIddictClientWebIntegrationHandlers.cs - just before the AttachAdditionalTokenRequestParameters handler, with an order of AttachTokenRequestParameters.Descriptor.Order - 500 - that mutate context.Scopes before it is attached to the token request.

That said, I see in the docs that the public scopes seems to be also supported for the code flow. Maybe we should instead just declare it Default="true" Required="false" in OpenIddictClientWebIntegrationProviders.xml so that it's always added if the user doesn't add any other scope, independently of the flow? (client credentials or code flow).

Question 2: Which naming do you prefer for parameter and constant names: GameMode, Mode, or Ruleset? For the osu!catch constant name, do you prefer Catch (more familiar) or Fruits (consistent with the API)? For ordering, do you prefer the familiar/custom order or switching to alphabetical order?

Re-using the official names is generally the most reasonable approach, but if find the names currently used in your PR are clearer, I don't mind keeping them as-is 😃

Note: you'll probably want to update MapCustomWebServicesFederationClaims to map the user ID/name/email to their WS-Federation equivalent for consistency with the other providers.

Thanks!

- Extracted OverrideScope handler
- Maped WS-Federation properties
- Updated XML definitions

Signed-off-by: Gehongyan <gehongyan1996@126.com>
@gehongyan
Copy link
Contributor Author

Hi, thanks for helping.

Maybe we should instead just declare it Default="true" Required="false" in OpenIddictClientWebIntegrationProviders.xml so that it's always added if the user doesn't add any other scope, independently of the flow?

I’ve tested the behavior when the public scope is added by default.

As shown in the screenshot below, the permission Read public data on your behalf is requested during the code flow. This allows apps to access endpoints requiring the public scope (indicated by the green tag in the documentation, such as Get Beatmap Pack).

image

However, I couldn't find a clean way to remove this scope if the client application specifically doesn't want to request it. Given this, should we still add it by default, or is there a specific method I missed that allows for removing default scopes?

Also, it seems that the Client Credentials flow does not automatically pick up the public scope even if it is configured as a default.

In the meantime, assuming the public scope shouldn't be forced via configuration, I’ve gone ahead and implemented the OverrideScope handler as you suggested. However, setting the order to AttachTokenRequestParameters.Descriptor.Order - 500 causes OverrideScope to execute before AttachTokenRequestParameters, where context.TokenRequest is initialized. Before this handler, context.TokenRequest is still null. Do you have any further suggestions? Would AttachTokenRequestParameters.Descriptor.Order + 250 be appropriate here?

Re-using the official names is generally the most reasonable approach, but if find the names currently used in your PR are clearer, I don't mind keeping them as-is.

I furtherly checked how osu!web and SDKs name it.

Given that most SDKs use the names GameMode and Catch, I decided to stick with the more familiar Catch. Since it's not a Flags enum, using the singular form (instead of GameModes) seems more reasonable and consistent with common usage.

you'll probably want to update MapCustomWebServicesFederationClaims to map the user ID/name/email to their WS-Federation equivalent for consistency with the other providers.

The ID is mapped from id, and the name is mapped from username now. The email is not provided.

image

@kevinchalet
Copy link
Member

However, I couldn't find a clean way to remove this scope if the client application specifically doesn't want to request it. Given this, should we still add it by default, or is there a specific method I missed that allows for removing default scopes?

The idea behind default scopes is that they are not requested if the user explicitly added any other value (they exist because some services require at least one scope to be defined, so default scopes can be considered "fallback values").

Also, it seems that the Client Credentials flow does not automatically pick up the public scope even if it is configured as a default.

Ah yeah, good point. You're right, the scopes configured there are only used for user-interactive flows.

In the meantime, assuming the public scope shouldn't be forced via configuration, I’ve gone ahead and implemented the OverrideScope handler as you suggested. However, setting the order to AttachTokenRequestParameters.Descriptor.Order - 500 causes OverrideScope to execute before AttachTokenRequestParameters, where context.TokenRequest is initialized. Before this handler, context.TokenRequest is still null. Do you have any further suggestions? Would AttachTokenRequestParameters.Descriptor.Order + 250 be appropriate here?

Instead of setting context.TokenRequest.Scope directly, consider just adding the value to context.Scopes: OpenIddict will later attach that collection to the token request without requiring any additional code.

Given that most SDKs use the names GameMode and Catch, I decided to stick with the more familiar Catch. Since it's not a Flags enum, using the singular form (instead of GameModes) seems more reasonable and consistent with common usage.

👍🏻

Thanks!

Signed-off-by: Gehongyan <gehongyan1996@126.com>
@gehongyan
Copy link
Contributor Author

The idea behind default scopes is that they are not requested if the user explicitly added any other value.

I agree that. But the osu! API documentation states that identify is the default scope for the Authorization Code Grant and always implicitly provided. The Client Credentials Grant does not currently have any default scopes.

Furthermore, the scope parameter is optional because when no scope is specified, osu!web only requests identify permissions (Identify you and read your public profile), and the code flow still functions correctly without any explicit scopes. In this case, do we still need to set a default value for Scope in the XML?

image

Instead of setting context.TokenRequest.Scope directly, consider just adding the value to context.Scopes: OpenIddict will later attach that collection to the token request without requiring any additional code.

Thank you for the tip; AttachTokenRequestParameters.Descriptor.Order - 500 works correctly now.

However, I suddenly discovered another issue. Some scopes seemed to be only specified in the Client Credentials mode, including delegate, forum.write_manage and group_permissions (can be confirmed here. How should the developer's user code specify these scopes for Client Credentials authentication if we might add them in the OverrideScopes handler? It seems that specifying Scopes via Properties is not quite appropriate.

@kevinchalet
Copy link
Member

In this case, do we still need to set a default value for Scope in the XML?

If the scope parameter is really optional for the code flow, let's not add that to the XML file. The event handler will take care of the client credentials scenario.

How should the developer's user code specify these scopes for Client Credentials authentication if we might add them in the OverrideScopes handler? It seems that specifying Scopes via Properties is not quite appropriate.

Scopes registered via OpenIddictClientRegistration.Scopes (or via options.UseWebProviders().Add[provider name]().AddScopes(...) for a web provider) are only use for interactive flows like the code flow or the device authorization flow. For other flows like password or the client credentials grant, developers are expected to attach the scopes to the Scopes property:

// Ask OpenIddict to authenticate the client application using the client credentials grant.
await _service.AuthenticateWithClientCredentialsAsync(new()
{
    CancellationToken = stoppingToken,
    ProviderName = provider,
    Scopes = ["scope1", "scope2"]
});

if (context.GrantType is GrantTypes.ClientCredentials &&
context.Registration.ProviderType is ProviderTypes.Osu)
{
context.Scopes.Add("public");
Copy link
Member

@kevinchalet kevinchalet Dec 15, 2025

Choose a reason for hiding this comment

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

Let's try something: try requesting one of the delegate, forum.write_manage and group_permissions scopes without adding public: if it works, it means public is not strictly required but that osu requires at least one scope to be set (what OpenIddict calls "default scope").

If it works without public being present, we may want to only add it if the context.Scopes collection is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a Chat Bot account, so I cannot request delegate or other scopes that require delegation; my personal account receives the error 'Delegation with Client Credentials is only available to chat bots.' when requesting such permissions.

However, the documentation mentions: 'When using delegation, scopes that support delegation cannot be used together with scopes that do not support delegation. Delegation is only available to Chat Bots.'

This indicates that when a developer requests delegation scopes, appending public is incorrect, because the Scopes section points out that public is not marked as Can Delegate.

The source code of osu!web also confirms this:

https://github.com/ppy/osu-web/blob/c2c9404146a934bd985d40a11a5a96fda7261338/app/Models/OAuth/Token.php#L27
https://github.com/ppy/osu-web/blob/c2c9404146a934bd985d40a11a5a96fda7261338/app/Models/OAuth/Token.php#L236-L245

In a Client Credentials request, if the scopes contain elements from SCOPES_REQUIRE_DELEGATION, all elements must be included in the SCOPES_REQUIRE_DELEGATION.

Therefore, I agree with your suggestion to add public to the context.Scopes only when it is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to create an issue on the osu!web later regarding the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

…tly specified

Signed-off-by: Gehongyan <gehongyan1996@126.com>
Signed-off-by: Gehongyan <gehongyan1996@126.com>
Signed-off-by: Gehongyan <gehongyan1996@126.com>
@gehongyan gehongyan marked this pull request as ready for review December 20, 2025 16:41
Copilot AI review requested due to automatic review settings December 20, 2025 16:41
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 pull request adds osu! (a popular rhythm game platform) as a supported OAuth authentication provider to the OpenIddict client web integration library.

Key Changes:

  • Added osu! provider configuration with OAuth 2.0 endpoints for authorization, token exchange, and user information retrieval
  • Implemented custom scope handling for client credentials grant flow (defaults to "public" scope when none specified)
  • Added support for optional game mode parameter in userinfo endpoint queries (osu, taiko, fruits, mania)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
OpenIddictClientWebIntegrationProviders.xml Adds osu! provider definition with OAuth endpoints, game mode constants, and property mappings
OpenIddictClientWebIntegrationHandlers.cs Implements OverrideScope handler for client credentials flow, dynamic userinfo endpoint construction with game mode support, and integrates username/user ID extraction logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kevinchalet
Copy link
Member

kevinchalet commented Dec 28, 2025

Hey @gehongyan,

FYI: OpenIddict just moved from PolySharp to Polyfill, which allows us to use a lot more polyfills thanks to .NET 10's extensions-everything model 😃

To make sure your PR can benefit from that change, I merged the dev branch and I updated the new OverrideScopes handler to use ThrowIfNull() and GetValueOrDefault().

Cheers 😄

@gehongyan
Copy link
Contributor Author

Nice work! 😄

@kevinchalet
Copy link
Member

@gehongyan do you think we can merge your PR or are there changes you'd still like to make depending on ppy/osu-web#12631 outcome? 😃

@gehongyan
Copy link
Contributor Author

@kevinchalet
The documentation changes align with the analysis we made based on the code previously, so there shouldn't be anything that needs to be changed at the moment. I think this PR can now be completed. If there are any other issues in the future, I will open new tickets. 🚀

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants