search

openiddict / openiddict-core

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

Add Feishu (Lark) to the list of supported providers #2097

Closed gehongyan closed 5 months ago

gehongyan commented 5 months ago

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

gehongyan commented 5 months ago

Hints

Docs (English avaliable)

Feishu

Lark

Lark Suite is the global version of Feishu. It seems that all the docs are almost the same as above. The API host is changed to open.larksuite.com from open.feishu.cn.

Current Implementation

What I Did

I assume that Feishu and Lark can be two different environments, so I added two environments to the XML configuration.

I tried naming them Feishu and Lark, but the compiler complains that the environment Production is not found. So at present, I named them Production and Lark.

[!TIP] Question 1: Is it property to name them as Production and Lark? Or is it better to consider them as the same environment (because both of them are production environments), add another Setting to allow users to modify the API host, and assemble all API request URI in codes when the user code specifies the non-default Lark platform? Or even should we split them into two providers?

Their token request requires a Authorization header with the value of Bearer 'access_token', where the access_token should be a app_access_token. Here are the docs how to obtain them:

Obtain access tokens - Server API - Documentation - Lark Developer Get custom app app_access_token - Server API - Documentation - Lark Developer

Hence, I provided a setting to allow the user code to set the app_access_token.

The header value is set in OpenIddictClientWebIntegrationHandlers.Exchange.AttachNonStandardBasicAuthenticationCredentials.

They respond the token request with a non-standard JSON, so I extracted the error code and successful data in OpenIddictClientWebIntegrationHandlers.Exchange.MapNonStandardResponseParameters.

They also return a nested data object, which will be handled in OpenIddictClientWebIntegrationHandlers.Userinfo.UnwrapUserinfoResponse.

They use a different token endpoint when refreshing tokens, which will be handled in OpenIddictClientWebIntegrationHandlers.OverrideTokenEndpoint.

Issues not Fixed Well

When users deny an authorization request, they do redirect back with a state parameter:

http://localhost:49152/callback/login/feishu?error=access_denied

If I append the state as a query parameter to the redirect URI just like what you taught me yesterday in Huawei OAuth implementation, the denying will work, 

http://localhost:49152/callback/login/feishu?state=G_8BHA5dv*******************hobtjP5Nw&error=access_denied

but they will redirect back with an empty state parameter when users authorize:

http://localhost:49152/callback/login/feishu?code=ca0i847***************31fqhi25u&state=

This breaks the normal authorization flow.

[!TIP] Question 2: How do we deal with it better?

kevinchalet commented 5 months ago

Hey,

Thanks for your PR!

I took a look at the docs and Good Lord, that OAuth 2.0 implementation is just terrible. Some companies are really good at inventing their own non-standard horrors 🤣

Question 1: Is it property to name them as Production and Lark? Or is it better to consider them as the same environment (because both of them are production environments), add another Setting to allow users to modify the API host, and assemble all API request URI in codes when the user code specifies the non-default Lark platform? Or even should we split them into two providers?

Using environments is likely not the best option here (as you figured out, we require a Production environment, so it would be weird to call one Production and the other Lark when both are actually production environments).

We have two options:

Their token request requires a Authorization header with the value of Bearer 'access_token', where the access_token should be a app_access_token. Here are the docs how to obtain them:

Obtain access tokens - Server API - Documentation - Lark Developer Get custom app app_access_token - Server API - Documentation - Lark Developer

What a horrible/non-standard thing... I wanted to kill myself after reading the docs 🤣

Are we sure that "app access token" (or should we use a tenant access tenant, their docs says they are merging the two?!) doesn't expire? If it does expire, we won't be able to use a setting for that, as the provider would stop working after a short time 😭

image

* **Extract data from the token response**

They respond the token request with a non-standard JSON, so I extracted the error code and successful data in OpenIddictClientWebIntegrationHandlers.Exchange.MapNonStandardResponseParameters.

That works, but the manual mapping is a bit cumbersome. We'll probably want to have an UnwrapTokenResponse handler that is an equivalent of UnwrapUserinfoResponse to simplify things here 😄

kevinchalet commented 5 months ago

Question 2: How do we deal with it better?

We can't do anything on our side, sadly. Feishu/Lark will have to fix their broken implementation to improve things for users.

Edit: or maybe we could try an hybrid approach by not removing the state parameter even if we added to the redirect_uri? I.e don't call this line for Feishu? https://github.com/openiddict/openiddict-core/blob/7194d58132de99812a3d0af0b713c993a5f4022a/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.cs#L1589

kevinchalet commented 5 months ago

The docs referenced by AspNet.Security.OAuth.Feishu implementation deprecated and is not recommended. See Login Process - Developer Guides - Documentation - Feishu Open Platform (English avaliable).

OMG, that implementation was fairly standard! Why did they replace it by a completely non-standard thing?! Makes no sense at all...

Do we know if they plan to phase their deprecated APIs out?

Edit:

image

🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣 🤣

gehongyan commented 5 months ago

are they operated by the same organization?

Both of them are organized by ByteDance, but I am not familiar with their internal management. According to information on search engines, it seems that Lark was published to the Global market except for China mainland earlier. After half a year, they published the China mainland version with a new name - Feishu.

Based on my observations of the documentation and management console for both products, I am inclined to believe that they are the same product, just deployed in different regions to serve different regions. Hence, I prefer to provide a setting of Region (CN and Global) or Product (Feishu or Lark), instead of splitting them into two different providers at present.

If the provider is named Feishu, should we consider the CN version as the default? Conversely, if the Global version is the default, perhaps the provider should be named Lark instead.

Are we sure that "app access token" doesn't expire?

The app_access_token does expire.

image

should we use a tenant access tenant, their docs says they are merging the two

Yes, the docs say they are merging of them with a single tenant_access_token. At present, the API above returns the same values of these fields under my circumstances. The docs still explicitly declare it needs the app_access_token. Should we use the tenant one instead in advance?

If it does expire, we won't be able to use a setting for that, as the provider would stop working after a short time

It means that the setting should be a static value instead of a dynamic one. So, do we have a better way to support it? Or we are not able to achieve it at present.

We'll probably want to have an UnwrapTokenResponse handler that is an equivalent of UnwrapUserinfoResponse to simplify things here

If we can solve the app_access_token to continue this pull request, I will try to create a new handler to simplify the unwarp action.

gehongyan commented 5 months ago

or maybe we could try an hybrid approach by not removing the state parameter even if we added to the redirect_uri? I.e don't call this line for Feishu?

Nice, it works for both authorization and deny actions.

image

Ingenious workaround. 👍

kevinchalet commented 5 months ago

If the provider is named Feishu, should we consider the CN version as the default? Conversely, if the Global version is the default, perhaps the provider should be named Lark instead.

Since it seems Lark was released first, I guess it makes sense to name it Lark and use the global region?

So, do we have a better way to support it? Or we are not able to achieve it at present.

If we wanted to support that, we'd need to acquire that stupid app token before making the token request using an HttpClient...

Another option - my favorite to be honest - would be to ignore the deprecation and target the old - standard! - API that doesn't require any of the workarounds we need with the "new" version. If they ever decide to remove it, we'll still be able to migrate to the new one at a later date 😃

gehongyan commented 5 months ago

Do we know if they plan to phase their deprecated APIs out?

Maybe not, but they should leave the deprecated version still working without breaking legacy applications. The docs say that this deprecated version does not support the scope parameter. I checked the developer console, and found that they support tons of scopes to authorize a third-party app to invoke APIs. If we implement the deprecated version, perhaps user codes will only to get who authorized the OAuth flow without more modern abilities in the future.

image

For example, if user code applies these scopes:

image

The authorization page will be:

image

which reflects the scopes requested.

kevinchalet commented 5 months ago

Hum. What happens when targeting the old OAuth 2.0 endpoints (i.e without scope support)? Do you get an unrestricted access?

gehongyan commented 5 months ago

Hum. What happens when targeting the old OAuth 2.0 endpoints (i.e without scope support)? Do you get an unrestricted access?

Let me take some time to test it out.

gehongyan commented 5 months ago

It seems that the legacy version of OAuth authorization supports scopes 🚀

The authorization page URI is like:

https://passport.feishu.cn/suite/passport/oauth/authorize?client_id=cli_**************&redirect_uri=http%3A%2F%2Flocalhost%3A49152%2Fcallback%2Flogin%2Ffeishu&response_type=code&scope=contact%3Auser.phone%3Areadonly%20contact%3Auser.email%3Areadonly&state=ODrV********************uK6e_iE

Before I authorized the app with email and mobile phone numbers, the user info API responded with:

image

If I authorized the app of those scopes:

image

The authorization page displayed what scopes the app requested:

image

And redirection works:

image

Now, the same user info API responded with the phone number and email:

image

The authorization detail in my account also shows these authorized scopes:

However, when users deny the authorization demand, the legacy OAuth doesn't redirect back to the redirct_uri.

image

kevinchalet commented 5 months ago

It seems that the legacy version of OAuth authorization supports scopes 🚀

🤣

In this case, let's use the legacy API: if they ever remove it, we'll update both the aspnet-contrib and OpenIddict providers, but in the meantime, the standard OAuth 2.0 API seems to be a much better choice 😄

However, when users deny the authorization demand, the legacy OAuth doesn't redirect back to the redirct_uri.

It's quite frequent. I guess it's a reasonable compromise 😃

Did you have a chance to test the Lark flavor and see if scopes also work with the legacy version?

gehongyan commented 5 months ago

In this case, let's use the legacy API: if they ever remove it, we'll update both the aspnet-contrib and OpenIddict providers, but in the meantime, the standard OAuth 2.0 API seems to be a much better choice 😄

Nice, I will update the codes to apply these suggestions.

It's quite frequent. I guess it's a reasonable compromise 😃

So we have to accept that denying does not work because the legacy version is a better choice from an overall perspective. 👍

Did you have a chance to test the Lark flavor and see if scopes also work with the legacy version?

Yes, I tested Lark as well, scopes of which also work with the legacy version.

image

gehongyan commented 5 months ago

(If I use the Feishu client_id and client_secret to create authorization demands on Lark endpoints:

image

The page shows my Feishu authentication credentials do not have permissions for the app created by my Lark account. Now, I may reasonably assume that their data are not completely separated and isolated.

kevinchalet commented 5 months ago

Merged, thanks for your contribution! 🎉

gehongyan commented 5 months ago

Thanks for your help! ❤️