Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in @sap-cloud-sdk/connectivity version 3.18.0 #5026

Closed
BastiInnovation opened this issue Sep 25, 2024 · 5 comments
Closed

Error in @sap-cloud-sdk/connectivity version 3.18.0 #5026

BastiInnovation opened this issue Sep 25, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@BastiInnovation
Copy link

BastiInnovation commented Sep 25, 2024

Describe the bug
After updating @sap-cloud-sdk/connectivity from version 3.17.0 to 3.18.0. The destination from getDestinationFromDestinationService() is null. In cloud foundry log i received a warning: "Cannot create cache key for client credentials token cache. The given tenant id is undefined".

To Reproduce

const getDestinationFromDestinationService = require('@sap-cloud-sdk/connectivity').getDestinationFromDestinationService;
const serviceToken = require('@sap-cloud-sdk/connectivity').serviceToken;
const getXsuaaServiceCredentials = require('@sap-cloud-sdk/connectivity/dist/scp-cf').getXsuaaServiceCredentials;
const getDestinationServiceCredentials = require('@sap-cloud-sdk/connectivity/dist/scp-cf').getDestinationServiceCredentials;
const requests = require('@sap/xssec').requests;

static async getDestination(tenant){
    return await getDestinationFromDestinationService({ 
        destinationName: 'myDestinationName',
        jwt: await this.fetchToken(tenant),
        iasToXsuaaTokenExchange: false });
  }

static async fetchToken(tenant){
    let token;
    if( getXsuaaServiceCredentials() ) {
      token = await serviceToken('xsuaa', { jwt: { iss: `https://${tenant}.localhost:8080/uaa/oauth/token` }, useCache: true });
    } else {
      token = await new Promise((resolve) => requests.requestClientCredentialsToken(tenant, getDestinationServiceCredentials(), null, (_err, tkn) => resolve(tkn)));
    }
    return token;
  }

Expected behavior
I expected to receive the destination from getDestination() because the token from fetchToken() is the right one.
I think there is a relation to 4799, but I found non solution in the related documentation.

Used Versions:
node version: v20.11.0
sap-cloud-sdk/connectivity: v3.18.0
sap-cloud-sdk/util: v3.18.0
sap/cds: v7.9.4
sap/xssec: v3.6.1

@BastiInnovation BastiInnovation added the bug Something isn't working label Sep 25, 2024
@tomfrenken
Copy link
Member

tomfrenken commented Nov 18, 2024

Hi @BastiInnovation, I agree that the issue you are facing is likely caused by the referenced PR, however, after reviewing our code I do not see any change that would lead to your issue.

The warning you are receiving indicates that the tenant look-up is failing.
Since you provide a jwt in your call we check for 3 different properties:
zid, app_tid, ext_atr.zdn, and iss in that order.

The only difference is that we now check for the zdn property ...

Could you debug what your JWT contains and perhaps share a redacted version with us?

@nocheintobi
Copy link

Hi @tomfrenken,
I'm @BastiInnovation's coworker. The token, we get issued looks fine and contains the subscriber tenant's subdomain in "ext_attr.zdn" as expected, as you can see in the redacted version below. The app_tid property is missing...

{
  "jti": "01234567890123456789012345678901",
  "ext_attr": {
    "enhancer": "XSUAA",
    "subaccountid": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
    "zdn": "mytenant"
  },
  "sub": "sb-myapp!tnnnnn",
  "authorities": [
    "uaa.resource"
  ],
  "scope": [
    "uaa.resource"
  ],
  "client_id": "sb-myapp!tnnnnn",
  "cid": "sb-myapp!tnnnnn",
  "azp": "sb-myapp!tnnnnn",
  "grant_type": "client_credentials",
  "rev_sig": "yyyyyyyy",
  "iat": 1732607862,
  "exp": 1732651062,
  "iss": "https://mytenant.authentication.eu20.hana.ondemand.com/oauth/token",
  "zid": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
  "aud": [
    "sb-myapp!tnnnnn",
    "uaa"
  ]
}

In the meantime, we fetch the destination as follows:

return await getDestinationFromDestinationService({destinationName, iss: `https://${tenant}.localhost:8080/uaa/oauth/token`})

So we do not fetch the token by ourselves manually and instead delegate this to @sap-cloud-sdk/connectivity.
Thereby we lose the capability of reusing the token during its lifetime and are no longer able to inspect it once something goes wrong, but that doesn't hurt.

But we'd anyways like to find out, why the well-known way of getting the destination doesn't work anymore.

One more info, that might be the cause for the missing app_tid: The provided token is taken from a local test environment that is bound to deployed service instances of xsuaa/destination/connectivity/sreg...

@tomfrenken
Copy link
Member

tomfrenken commented Nov 27, 2024

Hi @nocheintobi, we found the cause.

Since we introduced support for IAS tokens, we check for different properties to build our cache key, based on the type of JWT (IAS or XSUAA).
We use the iss property you provided only if it is an XSUAA JWT now, but to identify an XSUAA JWT, we check for the property jwt.ext_attr.enhancer === 'XSUAA'.

So for your specific use case, you can simply add this property to your call:

token = await serviceToken('xsuaa', { jwt: { iss: `https://${tenant}.localhost:8080/uaa/oauth/token`, ext_attr.enhancer: 'XSUAA'}, useCache: true });

@nocheintobi
Copy link

nocheintobi commented Nov 27, 2024

Thanks for your explanation @tomfrenken,
I modified your proposal to ..., ext_attr: {enhancer: 'XSUAA'}, ... for making TypeScript happy ;-)
Your proposal works fine, so the issue can be closed from my point of view @BastiInnovation .
It would have been nice, if changes like that were mentioned in the release notes or maybe a migration guide...

That modification also has a positive side-effect to our local development environment, which I'd like to point out since it makes our lives much easier here:

Before, we had to modify VCAP_SERVICES.xsuaa.credentials.uaadomain and VCAP_SERVICES.xsuaa.credentials.url in the approuter's default-env.json by prefixing both with a tenant to be used for local tests. That's not neccessary anymore (and in fact it will fail).

Thanks a lot for your support!!!

@tomfrenken
Copy link
Member

Yeah that's a good point, I think when we introduced this change, the assumption was that it wouldn't make a difference for any "real" JWT coming from either the IAS or XSUAA, therefore we didn't mention it anywhere, I supposed we didn't take into account the potential impact on local testing setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants