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.

Sign up for GitHub

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

fix: check proxy and sap connectivity auth tokens for caching #5554

Draft
wants to merge 6 commits into
base: v3-main
Choose a base branch
from
Draft

fix: check proxy and sap connectivity auth tokens for caching #5554

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e60f75b
fix: check proxy auth andsap connectivity auth tokens for setting cac…
ZhongpinWang Feb 19, 2025
4294f65
chore: add unit tests
ZhongpinWang Feb 24, 2025
963a283
Changes from lint:fix
Feb 24, 2025
c984423
Merge branch 'v3-main' into fix-cache-retrieved-destination-expiration
ZhongpinWang Feb 24, 2025
ad4d5df
chore: add changeset
ZhongpinWang Feb 24, 2025
529c1ec
Update .changeset/thick-glasses-lick.md
ZhongpinWang Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/thick-glasses-lick.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sap-cloud-sdk/connectivity': minor
---

[Fix Issue] Check `Proxy-Authorization` and `SAP-Connectivity-Authentication` tokens to determine cache expiration time.
107 changes: 107 additions & 0 deletions packages/connectivity/src/scp-cf/destination/destination-cache.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
mockJwtBearerToken,
mockServiceToken
} from '../../../../../test-resources/test/test-util/token-accessor-mocks';
import { signedJwtForVerification } from '../../../../../test-resources/test/test-util';
import { destinationServiceCache } from './destination-service-cache';
import {
alwaysProvider,
Expand Down Expand Up @@ -806,6 +807,112 @@ describe('destination cache', () => {
});
});

it('should return undefined when Proxy-Authorization token expires first', async () => {
jest.useFakeTimers();
const twoMinutesTokenLifetime = 2 * 60;
const fourMinutesTokenLifetime = 4 * 60;
const sixMinutesTokenLifetime = 6 * 60;
const dummyJwt = { user_id: 'user', zid: 'tenant' };

const proxyAuthorizationToken = signedJwtForVerification({
...decodeJwt(providerServiceToken),
exp: twoMinutesTokenLifetime
});

const sapConnectivityAuthenticationToken = signedJwtForVerification({
...decodeJwt(providerUserToken),
exp: fourMinutesTokenLifetime
});

const destination = {
...destinationOne,
authTokens: [
{
expiresIn: sixMinutesTokenLifetime.toString()
} as DestinationAuthToken
],
proxyConfiguration: {
...connectivityProxyConfigMock,
headers: {
'Proxy-Authorization': `Bearer ${proxyAuthorizationToken}`,
'SAP-Connectivity-Authentication': `Bearer ${sapConnectivityAuthenticationToken}`
}
}
};

await destinationCache.cacheRetrievedDestination(
dummyJwt,
destination,
'tenant-user'
);

const retrieveDestination = () =>
destinationCache.retrieveDestinationFromCache(
dummyJwt,
destination.name!,
'tenant-user'
);

await expect(retrieveDestination()).resolves.toEqual(destination);

jest.advanceTimersByTime(twoMinutesTokenLifetime * 1000 + 1);

await expect(retrieveDestination()).resolves.toBeUndefined();
});

it('should return undefined when SAP-Connectivity-Authentication authorization token expires first', async () => {
jest.useFakeTimers();
const twoMinutesTokenLifetime = 2 * 60;
const fourMinutesTokenLifetime = 4 * 60;
const sixMinutesTokenLifetime = 6 * 60;
const dummyJwt = { user_id: 'user', zid: 'tenant' };

const proxyAuthorizationToken = signedJwtForVerification({
...decodeJwt(providerServiceToken),
exp: fourMinutesTokenLifetime
});

const sapConnectivityAuthenticationToken = signedJwtForVerification({
...decodeJwt(providerUserToken),
exp: twoMinutesTokenLifetime
});

const destination = {
...destinationOne,
authTokens: [
{
expiresIn: sixMinutesTokenLifetime.toString()
} as DestinationAuthToken
],
proxyConfiguration: {
...connectivityProxyConfigMock,
headers: {
'Proxy-Authorization': `Bearer ${proxyAuthorizationToken}`,
'SAP-Connectivity-Authentication': `Bearer ${sapConnectivityAuthenticationToken}`
}
}
};

await destinationCache.cacheRetrievedDestination(
dummyJwt,
destination,
'tenant-user'
);

const retrieveDestination = () =>
destinationCache.retrieveDestinationFromCache(
dummyJwt,
destination.name!,
'tenant-user'
);

await expect(retrieveDestination()).resolves.toEqual(destination);

jest.advanceTimersByTime(twoMinutesTokenLifetime * 1000 + 1);

await expect(retrieveDestination()).resolves.toBeUndefined();
});

describe('custom destination cache', () => {
// Cache with expiration time
const testCacheOne = new TestCache();
Expand Down
39 changes: 34 additions & 5 deletions packages/connectivity/src/scp-cf/destination/destination-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createLogger, first } from '@sap-cloud-sdk/util';
import { getTenantId, userId } from '../jwt';
import { decodeJwt, getTenantId, userId } from '../jwt';
import { AsyncCache } from '../async-cache';
import type { JwtPayload } from '../jsonwebtoken-type';
import type { AsyncCacheInterface } from '../async-cache';
Expand Down Expand Up @@ -217,11 +217,40 @@ async function cacheRetrievedDestination<T extends DestinationCacheInterface>(
throw new Error('The destination name is undefined.');
}

const key = getDestinationCacheKey(token, destination.name, isolation);
const expiresIn = first(destination.authTokens || [])?.expiresIn;
const expirationTime = expiresIn
? Date.now() + parseInt(expiresIn) * 1000
const authToken = first(destination.authTokens || []);
const proxyAuthorizationToken =
destination.proxyConfiguration?.headers?.['Proxy-Authorization']?.match(
/^Bearer (.+)/
)?.[1];
const sapConnectivityAuthenticationToken =
Copy link
Contributor

@deekshas8 deekshas8 Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

[req/q] I'm not sure if we should cache based on SAP-Connectivity-Authentication. This token is basically the subscriberToken?.userJwt.encoded, which is passed by the user (would fail the verify jwt step if its expired). We have no control on how this is fetched and refreshed by the user. So basing our caching on this seems wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

[pp] I would have preferred the approach to refresh only the proxy token if its expired. Its a CC grant token, whereas for fetching the whole destination again we make two additional calls to the destination service and the destination service itself might do additional exchange calls to the target system.
We're also mixing concerns between destinationCache and clientCredentialTokenCache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[req/q] I'm not sure if we should cache based on SAP-Connectivity-Authentication. This token is basically the subscriberToken?.userJwt.encoded, which is passed by the user (would fail the verify jwt step if its expired). We have no control on how this is fetched and refreshed by the user. So basing our caching on this seems wrong.

I understand your concern. I don't have strong opinion on this. Only from a pure technical point of view, users are allowed to set destination cache with setDestinationCache() method. And both headers are defined as part of the Destination / ProxyConfigurationHeaders interface. Technically, it means we also can't be sure, in the end, how user sets SAP-Connectivity-Authentication token. Therefore, I would say we should consider both headers when calculating the expiration time.

If exp of SAP-Connectivity-Authentication token is shorter than the authTokens for destination service and got expired, the Destination object itself is anyway invalid.

[pp] I would have preferred the approach to refresh only the proxy token if its expired. Its a CC grant token, whereas for fetching the whole destination again we make two additional calls to the destination service and the destination service itself might do additional exchange calls to the target system.
We're also mixing concerns between destinationCache and clientCredentialTokenCache.

Thanks for this cool suggestion, and ideally yes, we should try renew the proxy auth token. And I tried out.

We can add a check when retrieveDestinationFromCache. But since we store only expires time in a CacheEntry, which is calculated based on the first authTokens expiresIn, it lacks expiration time for proxy authentication (and, if applicable, sap connectivity authentication). We will have to modify CacheEntry interface to store this additional information. But I feel CacheEntry is designed rather in a generic way, and adding information regarding proxy is not appropriate.

So by cache design, we anyway need to determine a expiration time, which should invalidate the cache if its entry cannot be used anymore. Judging from a cached Destination entry, we have no clue, if the proxy authentication token has expired or not, since it only provides us a time interval and we don't know the start time when retrieving the destination.

Based on the above consideration, I would accept, at this moment, the fact that we may make few additional calls to Destination service. Unless in general, beyond the caching concept, we always renew the proxy auth token if it got expired.

Copy link
Contributor

@deekshas8 deekshas8 Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can you try what happens when we move the below line before we return the destination? I think this will automatically refresh the proxy auth and the other token in case they expired.

const withProxySetting = await da.addProxyConfiguration(destination);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please have a look at #5563.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this PR is not totally correct as exp of the token is actually seconds after epoch, not expires in.

destination.proxyConfiguration?.headers?.[
'SAP-Connectivity-Authentication'
]?.match(/^Bearer (.+)/)?.[1];

const authTokenExpiresIn = authToken?.expiresIn
? parseInt(authToken.expiresIn)
: undefined;
const proxyAuthorizationExpiresIn = proxyAuthorizationToken
? decodeJwt(proxyAuthorizationToken).exp
: undefined;
const sapConnectivityAuthenticationExpiresIn =
sapConnectivityAuthenticationToken
? decodeJwt(sapConnectivityAuthenticationToken).exp
: undefined;

const expiresIn = Math.min(
...[
authTokenExpiresIn,
proxyAuthorizationExpiresIn,
sapConnectivityAuthenticationExpiresIn
].filter(e => e !== undefined)
);
const expirationTime =
expiresIn && expiresIn !== Infinity
? Date.now() + expiresIn * 1000
: undefined;

const key = getDestinationCacheKey(token, destination.name, isolation);
cache.set(key, { entry: destination, expires: expirationTime });
}

Expand Down
Loading