SAP / cloud-sdk-js

Use the SAP Cloud SDK for JavaScript / TypeScript to reduce development effort when building applications on SAP Business Technology Platform that communicate with SAP solutions and services such as SAP S/4HANA Cloud, SAP SuccessFactors, and many others.
Apache License 2.0
162 stars 56 forks source link

tenant information is not send to destination service for authentication type 'OAuth2ClientCredentials' #1810

Closed dinurp closed 2 years ago

dinurp commented 2 years ago

Describe the bug

Cloud SDK API getDestination is not sending tenant information to destination service when the authentication type is OAuth2ClientCredentials. Because of this destination service is not able to return valid authTokens when the destination is configured with tokenServiceURLType as Common. In this case destination Service returns error messages instead of valid tokens:

Retrieval of OAuthToken failed due to: You must send the X-tenant header when the tokenServiceURLType property is Common

To Reproduce

Same as described in https://github.com/SAP/cloud-sdk-js/issues/1806 Inspection of authTokens in destination returned with userJwt for provider and authentication type as OAuth2ClientCredentials shows the error.

Expected behavior Valid authTokens should be fetched from destination service by sending the user context, either in X-tenant, in case the destination is configured with tokenServiceURLType property is Common.

Used Versions: v14.18.1

Code Examples

same setup as in https://github.com/SAP/cloud-sdk-js/issues/1806. Inspect the returned accessTokens

> var out = await sdk.getDestination("CAP_Incidents",{userJwt:process.env.access_token})
> out
{
  originalProperties: [Object],
  authTokens: [Array],
  certificates: [],
  name: 'CAP_Incidents,
  type: 'HTTP',
  url: 'https://httpbin.cfapps.eu10.hana.ondemand.com/anything',
  authentication: 'OAuth2ClientCredentials',
  proxyType: 'Internet',
  clientId: 'something',
  tokenServiceUrl: 'https://authentication.eu10.hana.ondemand.com/oauth/token',
  clientSecret: 'something',
  headers: [Object],
  queryParameters: [Object],
  isTrustingAllCertificates: false
}
> out.authTokens[0]
{
  type: '',
  value: '',
  expiresIn: '0',
  error: 'Retrieval of OAuthToken failed due to: You must send the X-tenant header when the tokenServiceURLType property is Common',
  http_header: undefined
}

Log file None

Impact / Priority Same as in https://github.com/SAP/cloud-sdk-js/issues/1806

Additional context Same as in https://github.com/SAP/cloud-sdk-js/issues/1806

FrankEssenberger commented 2 years ago

Hello @dinurp,

So could you try to adjust the tokenServiceUrl. We have never set the X-tenant header explicitly in our E2E tests and it worked just fine.

There is a {tenant} placeholder. In a multi tenant scenario you can use the placeholder {tenant} in the URL so that the right value is filled in at runtime. This is explained here.

In principle leaving it out should lead the same as putting the subdomain in the beginning of the URL, but perhaps this is not working.

Best Frank

dinurp commented 2 years ago

I tried adding {tenant} like you suggested:

> var out = await sdk.getDestination("http_bin_echo",{userJwt:process.env.access_token})
undefined
> out.authTokens[0]
{
  type: '',
  value: '',
  expiresIn: '0',
  error: 'Retrieval of OAuthToken failed due to: You must send the X-tenant header when the tokenServiceURLType property is Common',        
  http_header: undefined
}
> out
{
  originalProperties: [Object],
  authTokens: [Array],
  certificates: [],
  name: 'http_bin_echo',
  type: 'HTTP',
  url: 'https://httpbin.cfapps.eu10.hana.ondemand.com/anything',
  authentication: 'OAuth2ClientCredentials',
  proxyType: 'Internet',
  clientId: 'sb-cds-mtx-project!b71209',
  tokenServiceUrl: 'https://{tenant}.authentication.eu10.hana.ondemand.com/oauth/token',
  clientSecret: 'something - somethng,
  headers: [Object],
  queryParameters: [Object],
  isTrustingAllCertificates: false
}
dinurp commented 2 years ago

Dear @FrankEssenberger , Do you set tokenServiceURLType as Common for any test case? This is the case when X-tenant is required. Regards, Dinu

FrankEssenberger commented 2 years ago

Hi @dinurp,

I had a look at our test landscape and we have destination with dedicated and common in a provider and subscriber scenario. However, we did not test OAuth2ClientCredentials for a subscriber/provider case i.e. thetokenServiceURLType is set to common.

I wanted then to investigate where this error comes from and found the docs on the destination service API:

This header is only taken into account for destinations with auth type set to OAuth2ClientCredentials. It represents the subdomain of the tenant, on behalf of which to fetch an access token, using the configured credentials. The header is required when tokenServiceURLType is Common and the value will be used to replace the tenant placeholder (or will be added as a subdomain if no placeholder is present). If not provided, an error will be returned. If tokenServiceURLType is Dedicated, the use of this header is forbidden and an error will be returned if provided in the request. If both this header and X-user-token are provided, the request will fail with HTTP 400 Bad Request.

So indeed the header is needed only for OAuth2ClientCredentials flow and common. However, if I read the docs the use case does not make sense to me. So you configure a destination with a fixed clientId/secret which correspond to a specific tenant. As described in the docs these credentials are used to fetch a clientCrendentialGrant. So I wonder why you should not simply use the dedicated tokenServiceURLType and put the tenant to which the credentials correspond in the tokenServiceUrl field right away?

Could you perhaps explain if my understanding is correct that the value passed as X-tenant header is the same and corresponds to the crendentials and what is the benefit of passing this as a header and not as part of the destination. If I understand this better I can see if and how we could support this better.

Best Frank

dinurp commented 2 years ago

Dear @FrankEssenberger ,

I'm not sure if I understood your question: the value passed as X-tenant header is the same and corresponds to the credentials

The value of X-tenant should be the tenant (subdomain of XUSAA) for which destination is requested. It is just like OAuth2UserTokenExchange. Only that in the case of oAuth2ClientCredentails, the subsequent call is a system call; the token returned is with no user context. It gets full scope from the authority of client credentials. The value of X-tenant is used to fill a pattern for the token URL. This would be like (for XSUAA) https://{tenant}.authentication.[domain]/oauth/token Fill tenant in the pattern and you get the token URL for the tenant.

The reason and logic is similar to what is done in SDK for accessing destination service: the subdomain of the token URL (for XSUAA) determines which tenant the request is getting access to. The URL of the multitenant service that is called remains same. The initiating user may (or should) not have access to the service directly. So passing user context would result in denying the service for lack of scopes. The necessary scopes for accessing the service is granted to the client credentials configured. The same client credentials provide access to all the tenants. Access to different tenants is setup during subscription, just like for destination service. This same logic applies for all multitenant services and business services: Business Rules, Workflow, AIN, IoT, etc. Same applies to all business services based on XSUAA.

The advantage is that the credentials remain with the provider; there is no need to distribute this to subscribers. It remains in one place, so credentials rotation is easier.

Regards, Dinu

FrankEssenberger commented 2 years ago

Hi @dinurp,

thanks for explaining that to me. I do not know all the cases (provider, subscriber, ...) and the SDK seems not to support this one yet. So to make it 100% clear I try to summarize it in my own words:

The reason one does it this way is that, you maintain a destination once in the provider account which can then be used from all subscribers. If I got it right I think it is an easy fix. We would add a case here, where we check:

I know we have a call this afternoon, I will also join there.

Best Frank

FrankEssenberger commented 2 years ago

Here the issue in our internal backlog: https://github.com/SAP/cloud-sdk-backlog/issues/456 @dinurp as a colleague you should be able to see even details.

erictangsapcom commented 2 years ago

Hi @FrankEssenberger

I see your PR for fixing X-tenant to sdk js 2.0. will you provide hotfix for version 1.5.x. ? since the sdk 2.0 is planned to release in Jan 2020. which is mentioned https://sap.github.io/cloud-sdk/docs/js/announcing-version-2.

there is still one month gap from now to Jan 2022.

Regards Eric

FrankEssenberger commented 2 years ago

Yes, we migrated this to the version 1.X.Y, but it took me a bit longer but here is the PR: https://github.com/SAP/cloud-sdk-js/pull/1867. We will make a release of the 1.X.Y version in this week. Besides that you can use the canary tag to use the latest build before the release.

FrankEssenberger commented 2 years ago

Version 1.53.0 is released, which should fix the issue.

FrankEssenberger commented 2 years ago

I close the issue to keep our board tidy. If you experience problems please reopen or open a new issue.

dinurp commented 2 years ago

Works great. Thanks