openiddict / openiddict-core

Flexible and versatile OAuth 2.0/OpenID Connect stack for .NET
https://openiddict.com/
Apache License 2.0
4.47k stars 528 forks source link

Add Huawei to the list of supported providers #2095

Closed gehongyan closed 5 months ago

gehongyan commented 5 months ago

This pull request would like to add Huawei to the list of supported providers, which is also supported by AspNet.Security.OAuth.Providers - Huawei.

gehongyan commented 5 months ago

Sorry that I found you took time to check my code when I was writing this comment.

I am trying to add Huawei to the supported list, but I am not sure how to implement them.


Docs: (English supported)


What I have Tried

When I configure the provider XML to use the configuration endpoint as:

  <Provider Name="Huawei" Id="95a620d2-a362-47b0-b372-26fa9d41d20e"
            Documentation="https://developer.huawei.com/consumer/cn/doc/HMSCore-Guides/open-platform-oauth-0000001053629189">
    <Environment Issuer="https://accounts.huawei.com/"
                 ConfigurationEndpoint="https://oauth-login.cloud.huawei.com/.well-known/openid-configuration"/>
  </Provider>

And amended the grant types by:

https://github.com/gehongyan/openiddict-core/blob/96705d7da0913bd49399c8b226ff3fdc57ef46e6/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.Discovery.cs#L125-L132

Both the authorization code mode and client credentials grant mode work, but the authentication flow stops after the token request. Their configuration JSON doesn't contain the userinfo_endpoint value. No user info requests are sent.

image

image

[!NOTE] Question 1: Should I use the configuration endpoint, and modify everything to make up the procedures not listed in the JSON, instead of the manual configuration written as blow? For example, letting the user info request works.

I configured the XML as:

  <Provider Name="Huawei" Id="95a620d2-a362-47b0-b372-26fa9d41d20e"
            Documentation="https://developer.huawei.com/consumer/cn/doc/HMSCore-Guides/open-platform-oauth-0000001053629189">
    <Environment Issuer="https://accounts.huawei.com/">
      <Configuration AuthorizationEndpoint="https://oauth-login.cloud.huawei.com/oauth2/v3/authorize"
                     DeviceAuthorizationEndpoint="https://oauth-login.cloud.huawei.com/oauth2/v3/device/code"
                     TokenEndpoint="https://oauth-login.cloud.huawei.com/oauth2/v3/token"
                     UserinfoEndpoint="https://account.cloud.huawei.com/rest.php">
        <GrantType Value="authorization_code" />
        <GrantType Value="client_credentials" />
        <GrantType Value="refresh_token" />
        <GrantType Value="urn:ietf:params:oauth:grant-type:device_code" />
      </Configuration>
    </Environment>

    <Setting PropertyName="AccessType" ParameterName="access_type" Type="String" Required="false"
             Description="The value used as the 'access_type' parameter (can be set to 'offline' to retrieve a refresh token)" />
    <Setting PropertyName="Display" ParameterName="display" Type="String" Required="false"
             Description="The value used as the 'display' parameter (can be set to 'touch' to adjust the authorization page display style for mobile apps)" />
    <Setting PropertyName="FetchNickname" ParameterName="getNickName" Type="String" Required="false"
             Description="The value used as the 'getNickName' parameter (can be set to '1' to indicate that the nickname is returned preferentially)" />
  </Provider>

If I add <Scope Name="openid" Default="true" Required="true" /> as the default scope manually, the id_token validation breaks by throwing SecurityTokenSignatureKeyNotFoundException. If I leave the default scopes empty, it seems that the openid will be added automatically, and the id_token validation succeeds.

https://github.com/openiddict/openiddict-core/blob/8838ca79080d508a147a1b923749a417b86525a5/src/OpenIddict.Client/OpenIddictClientHandlers.Protection.cs#L357

[!NOTE] Question 2: If openid is the single default scope value, should I leave the XML configuration default scopes empty?

I found that I may modify either OpenIddictClientWebIntegrationHandlers.AttachAdditionalUserinfoRequestParameters or OpenIddictClientWebIntegrationHandlers.Userinfo.AttachNonStandardParameters to add non-standard parameters nsp_svc and getNickName to let it work.

image

[!NOTE] Question 3: What is the difference between these two handlers? Which handler should I append custom parameters in?

[!NOTE] Question 4: The parameter getNickName is an integer or boolean in fact, accepting values of 0 and 1. The XML property Type on Setting doesn't support them. Should we configure it as a string, and parse it in code?

When authenticating, the Huawei OAuth supports access_type to determine whether to retrieve a refresh token, and display to customize the display web UI for desktop or mobile apps. When fetching user info, it supports getNickName to determine whether to get the user nickname or the default partially redacted email address.

[!NOTE] Question 5: I am not sure how to implement PKCE mode. May I know whether there are any existing PKCE implementations of other web providers I could refer to?


Appreciate it! ❤️

kevinchalet commented 5 months ago

Question 1: Should I use the configuration endpoint, and modify everything to make up the procedures not listed in the JSON, instead of the manual configuration written as blow? For example, letting the user info request works.

The rule is that as soon as a provider offers a configuration endpoint, we should be using it (and amending the incorrect values if necessary). Using a static configuration when a dynamic configuration is available is only "possible" when the dynamic configuration is completely broken.

If I add <Scope Name="openid" Default="true" Required="true" /> as the default scope manually, the id_token validation breaks by throwing SecurityTokenSignatureKeyNotFoundException.

Yes, that's because when using a static configuration, the signing keys used by the server to protect its identity tokens cannot be retrieved automatically, so no key is available when trying to verify an id_token. This doesn't happen with a dynamic configuration because OpenIddict downloads the JWKS from Hauwei's server, which allows validating the identity token.

Question 3: What is the difference between these two handlers? Which handler should I append custom parameters in?

Both serve a similar purpose, but we only use OpenIddictClientWebIntegrationHandlers.Userinfo.AttachNonStandardParameters to attach static parameters that will never change (e.g the content type we want the server to return). On the other hand, the OpenIddictClientWebIntegrationHandlers.AttachAdditionalUserinfoRequestParameters handler is invoked earlier and is better for dynamic parameters that a user may want to customize after it's invoked.

Question 4: The parameter getNickName is an integer or boolean in fact, accepting values of 0 and 1. The XML property Type on Setting doesn't support them. Should we configure it as a string, and parse it in code?

AFAICT by looking at the docs - thanks for posting the English version of them 😄 - it seems we can get additional claims by requesting the profile and email scopes so using the userinfo endpoint shouldn't be necessary at all.

Question 5: I am not sure how to implement PKCE mode. May I know whether there are any existing PKCE implementations of other web providers I could refer to?

OpenIddict takes care of that for you: Huawei correctly lists S256 under code_challenge_methods_supported so no custom should be necessary to support PKCE 😃

Note: since there's an English version available, could you please update the documentation URI to point to it?

kevinchalet commented 5 months ago

It looks good to me but I see in the docs that both their authorization endpoint and their token endpoint return non-standard error parameters, so we'll need some mapping (specially for the token endpoint: the error they return is a JSON integer, so the response will be considered malformed by OpenIddict):

Let me know if you need additional details 😃

kevinchalet commented 5 months ago

Could you also please try to deny the authorization demand and see if they correctly return a state parameter attached to the redirect_uri?

gehongyan commented 5 months ago

Thanks for the help!

I will continue this pull request tomorrow. It's midnight here. 😄

Another question is how may I test whether PKCE and device code mode work?

kevinchalet commented 5 months ago

Thanks for the help!

My pleasure! Thanks for contributing to OpenIddict 😄

I will continue this pull request tomorrow. It's midnight here. 😄

Haha, no worries. Have a good night! 🌔

Another question is how may I test whether PKCE and device code mode work?

Testing PKCE is easy: select "authorization code flow" and take a look at the URI to which your browser is redirect: if you see code_challenge and code_challenge_method parameters attached and the whole flow is working, PKCE is working 😃

For the device code flow, it's going to be a lot more complicated, as it seems to be non-standard (I should have checked the documentation more seriously 😅):

So, if we want to support that, we'll need a lot of mapping. If you're going to use it, let's do it, but if it's not something you're going to use, let's just pretend they don't support the device authorization flow 😄

gehongyan commented 5 months ago

select "authorization code flow" and take a look at the URI to which your browser is redirect: if you see code_challenge and code_challenge_method parameters attached and the whole flow is working, PKCE is working

I tested "authorization code flow," observed the URI redirected, and found that the query parameters code_challenge and code_challenge_method do exist. So I can understand it as Huawei supports the PKCE mode, and OpenIddict will prioritize using this mode in the "authorization code flow," rather than using the "Authorization Code" mode. Hopes that my understanding is correct.

Could you also please try to deny the authorization demand and see if they correctly return a state parameter attached to the redirect_uri?

They don't return a state parameter when I deny the authorization demand.

http://localhost:49152/callback/login/huawei?error=1107&error_description=access+denied

So I noticed that the console sandbox does not react to the denied response, and the flow doesn't enter that custom OpenIddictClientWebIntegrationHandlers.HandleNonStandardFrontchannelErrorResponse handler.

For errored token responses

The Huawei OAuth response error code includes error code and sub-error code. Should we (a) simply map all error codes to Errors.InvalidRequest, or (b) use switch case clauses to map all error codes to respective circumstances including invalid_request, invalid_client, invalid_grant, unauthorized_client, unsupported_grant_type, and invalid_scope?

The current implementation is (a).

If you're going to use it, let's do it, but if it's not something you're going to use, let's just pretend they don't support the device authorization flow

I don't use it at present, but I am not sure whether there will be any developers who come here would like to use the device code flow. 😆

gehongyan commented 5 months ago

The latest commit added support for Huawei device code authorization flow.


Useful screenshots

The console app requests the device code authorization flow:

image

The token response payload before users scan (Huawei suggests the client to display the authorization page as a QR code to allow end users to scan via their mobile phones) to access the authorization page.

image

What users may see on the authorization page (_translated by Google).

image

The token response payload after users access the authorization page but have not performed the authorization.

image

After the users have performed the authorization.

image


What I did

They don't return the address of the device authorization endpoint in the discovery document.

I manually specified the DeviceAuthorizationEndpoint in OpenIddictClientWebIntegrationHandlers.Discovery.AmendEndpoints to https://oauth-login.cloud.huawei.com/oauth2/v3/device/code.

I noticed that both oauth-login.cloud.huawei.com and oauth-login1.cloud.huawei.com work. Their docs used these hosts mixedly. If the first one is the main host, we should use it instead of the latter one, which seems to be the backup host.

Their device authorization endpoint doesn't return a standard expires_in parameter: instead, it returns expire_in. Their device authorization endpoint doesn't return a standard verification_uri parameter: instead, it return verification_url. Their user verification endpoint doesn't work if you use it as-is without manually appending the user_code to the query string parameters (in the standard OAuth 2.0 device authorization grant, there's a verification_uri_complete parameter for that).

I handled the non-standard parameters in OpenIddictClientWebIntegrationHandlers.Device.MapNonStandardResponseParameters. When the response does not contain an error code, we will try to handle verification_url and expire_in. I also assembled the completed URI with the required query string demonstrated in the Huawei docs, and set it to VerificationUriComplete.

They don't use the standard urn:ietf:params:oauth:grant-type:device_code grant type. Instead they use device_code. They don't use the standard device_code parameter to flow the device code. Instead, they use code.

I changed the GrantType, Code and DeviceCode on the request in OpenIddictClientWebIntegrationHandlers.Exchange.MapNonStandardRequestParameters.

Additionally, to handle the authorization_pending state correctly, I mapped the error code 1101 and sub-error codes 20411 and 20412 to Errors.AuthorizationPending.


Hope that I did these mappings in proper handlers. 😄

If there are any non-standard code usage/format, or any weird formulations in my English code comments, please help me streamline them.

Big thanks! ❤️

kevinchalet commented 5 months ago

So I can understand it as Huawei supports the PKCE mode, and OpenIddict will prioritize using this mode in the "authorization code flow," rather than using the "Authorization Code" mode. Hopes that my understanding is correct.

Yeah, what some providers call "PKCE mode" or "PKCE flow" is nothing more than a good old authorization code flow to which you've added two authorization request parameters (code_challenge and code_challenge_method) and one token request parameter (code_verifier). When OpenIddict detects a code_challenge_methods_supported node in the configuration document, it generates the code challenge/verifier and attaches them automatically: the rest of the logic is purely identical to any other authorization code flow request.

They don't return a state parameter when I deny the authorization demand.

http://localhost:49152/callback/login/huawei?error=1107&error_description=access+denied

So I noticed that the console sandbox does not react to the denied response, and the flow doesn't enter that custom OpenIddictClientWebIntegrationHandlers.HandleNonStandardFrontchannelErrorResponse handler.

Ah crap. As you figured out, without the state parameter, OpenIddict refuses to process the response (for security reasons), so yeah, adding that mapping was sadly useless.

But before removing it, could you please give the Deezer/Mixcloud trick a try to see if it works with Huawei?

https://github.com/openiddict/openiddict-core/blob/8838ca79080d508a147a1b923749a417b86525a5/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs#L562-L614

https://github.com/openiddict/openiddict-core/blob/8838ca79080d508a147a1b923749a417b86525a5/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs#L1516-L1564

The Huawei OAuth response error code includes error code and sub-error code. Should we (a) simply map all error codes to Errors.InvalidRequest, or (b) use switch case clauses to map all error codes to respective circumstances including invalid_request, invalid_client, invalid_grant, unauthorized_client, unsupported_grant_type, and invalid_scope?

If you're feeling brave, you can try to implement the second option (it's a best effort: if you don't think a suitable standard error, using invalid_request is fine).

gehongyan commented 5 months ago

But before removing it, could you please give the Deezer/Mixcloud trick a try to see if it works with Huawei?

Yes, It will work with Huawei.

image

So maybe we can use this trick to keep the denying work.

kevinchalet commented 5 months ago

The latest commit added support for Huawei device code authorization flow.

Wooo, excellent job! 👍🏻

Useful screenshots

What users may see on the authorization page (_translated by Google).

image

What happens when you hit the Cancel button? Does Huawei return a different error from the token endpoint that we could map to a standard access_denied error (which would make the UX a lot better, as the flow would be aborted immediately)?

I noticed that both oauth-login.cloud.huawei.com and oauth-login1.cloud.huawei.com work. Their docs used these hosts mixedly. If the first one is the main host, we should use it instead of the latter one, which seems to be the backup host.

👍🏻

Hope that I did these mappings in proper handlers. 😄

Looks great!

Big thanks! ❤️

❤️

kevinchalet commented 5 months ago

So maybe we can use this trick to keep the denying work.

If authentication still works properly (that would be annoying if we broke the normal flow just to fix the errored response case 🤣), yeah, let's do it 👍🏻

gehongyan commented 5 months ago

What happens when you hit the Cancel button? Does Huawei return a different error from the token endpoint that we could map to a standard access_denied error (which would make the UX a lot better, as the flow would be aborted immediately)?

The token request will return image

So we need to map 1101-20414 to Errors.AccessDenied.

But this sub-error code is not listed in their docs.

image

Now the denying in device code mode works.

(The desktop Cancel button seems not working. I have to test it on my phone 😆 )

kevinchalet commented 5 months ago

I pushed two commits to slightly tweak the error extraction logic and indicate why we deliberately map verification_uri and verification_uri_complete to the same value.

Did you have a chance to test token refreshing and token revocation? 😃

gehongyan commented 5 months ago

Authentication Code mode

image

Device code mode

0dda745701d074a8a17bc907c0c9c7a4

image

image


Yes, refreshing and revocation still work.


I suddenly noticed that the Name claim had not been mapped. Should we map the nickname or display_name field to the Name claim?

kevinchalet commented 5 months ago

Awesome 🚀

I suddenly noticed that the Name claim had not been mapped. Should we map the nickname or display_name field to the Name claim?

Yeah, good idea. display_name sounds like a good candidate? (do the two claims have different values?)

gehongyan commented 5 months ago

The docs says that

image

About my account, both of them are the same.

gehongyan commented 5 months ago

I am not sure whether it is proper to extract the display_name property from the BackchannelIdentityTokenPrincipal.

It does work.

image

kevinchalet commented 5 months ago

I am not sure whether it is proper to extract the display_name property from the BackchannelIdentityTokenPrincipal.

That's perfect 👍🏻

If you don't plan on making additional changes, I'll merge this PR and make sure it's included in the next release 😃

gehongyan commented 5 months ago

No further changes are planned. 🎉

kevinchalet commented 5 months ago

Merged! 🎉 Thanks a lot for this great contribution! ❤️