fix: check proxy and sap connectivity auth tokens for caching #5554
Conversation
ZhongpinWang
changed the title
ZhongpinWang
changed the title
| destination.proxyConfiguration?.headers?.['Proxy-Authorization']?.match( | ||
| /^Bearer (.+)/ | ||
| )?.[1]; | ||
| const sapConnectivityAuthenticationToken = |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
[req/q] I'm not sure if we should cache based on
SAP-Connectivity-Authentication. This token is basically thesubscriberToken?.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.
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Please have a look at #5563.
There was a problem hiding this comment.
Also this PR is not totally correct as exp of the token is actually seconds after epoch, not expires in.
Co-authored-by: Tom Frenken <[email protected]>
Closes SAP/cloud-sdk-backlog#1255.
Fix #5131
v3-mainnow, which is not for v4 development.mainbranch (for v4), I've updated (V4-Upgrade-Guide.md)[./V4-Upgrade-Guide.md] in case my change has any implications for users updating to SDK v4Use the minimum of authentication token,
Proxy-Authorizationtoken, andSAP-Connectivity-Authenticationtoken to determine the expiration time.