feat: Add getIasToken() and getIasDestination() convenience functions#6431
feat: Add getIasToken() and getIasDestination() convenience functions#6431davidkna-sap wants to merge 9 commits intomainfrom
getIasToken() and getIasDestination() convenience functions#6431Conversation
b9f591b to
982d8e0
Compare
982d8e0 to
ec8ff90
Compare
KavithaSiva
left a comment
There was a problem hiding this comment.
Initial round of comments.
| /** | ||
| * @internal | ||
| */ | ||
| async function resolveIdentityService( |
There was a problem hiding this comment.
[req] resolveIdentityService is unnecessarily async.
There was a problem hiding this comment.
resolveServiceBinding is async, I think this should stay as-is.
There was a problem hiding this comment.
@KavithaSiva I thought the same at first, but I this is useful, so that the second non-async part of the function does not need to be wrapped in a promise.
Example:
Imagine this function (pass in boolean a, if it is true return 1 otherwise 0):
const test = (a: boolean): Promise<any> => (a ? Promise.resolve(1) : 0); # won't work
const test = (a: boolean): Promise<any> => (a ? Promise.resolve(1) : Promise.resolve(0)); # will work
const test = async (a: boolean): Promise<any> => (a ? Promise.resolve(1) : 0); # will work| /** | ||
| * Returns an IAS token from the Identity Authentication Service. | ||
| * Supports both technical user (OAuth2ClientCredentials) and business user (OAuth2JWTBearer) flows. | ||
| * @param service - Service credentials, a service type string (e.g., 'identity'), or a {@link Service} instance. |
There was a problem hiding this comment.
[q] Should we also add:
Passing raw ServiceCredentials, Service directly is only recommended for environments where service bindings are unavailable as they hardcode the credentials?
identity string is the best way as the SDK resolves the credentials from the environment automatically.
| * @returns An {@link IasTokenResult} containing the access token, expiration, and optional refresh token. | ||
| */ | ||
| export async function getIasToken( | ||
| service: ServiceCredentials | string | Service, |
There was a problem hiding this comment.
| service: ServiceCredentials | string | Service, | |
| service: ServiceCredentials | 'identity' | Service, |
[q] Can't we narrow the type?
Co-Authored-By: KavithaSiva <32287936+kavithasiva@users.noreply.github.com>
0d20ea3 to
1d0ce3f
Compare
| service: ServiceCredentials | 'identity' | Service = 'identity', | ||
| options?: IasTokenOptions |
There was a problem hiding this comment.
What do you think about putting IasTokenOptions first with service having a sane default?
There was a problem hiding this comment.
Hmm, while I see the point, semantically I think it is better as is.
marikaner
left a comment
There was a problem hiding this comment.
I find it a little odd that we have extra functions for the IAS case. I assume you thought about how it could be covered seamlessly in the existing functionality?
| service: ServiceCredentials | 'identity' | Service = 'identity', | ||
| options?: IasTokenOptions |
There was a problem hiding this comment.
Hmm, while I see the point, semantically I think it is better as is.
| /** | ||
| * @internal | ||
| */ | ||
| async function resolveIdentityService( |
There was a problem hiding this comment.
@KavithaSiva I thought the same at first, but I this is useful, so that the second non-async part of the function does not need to be wrapped in a promise.
Example:
Imagine this function (pass in boolean a, if it is true return 1 otherwise 0):
const test = (a: boolean): Promise<any> => (a ? Promise.resolve(1) : 0); # won't work
const test = (a: boolean): Promise<any> => (a ? Promise.resolve(1) : Promise.resolve(0)); # will work
const test = async (a: boolean): Promise<any> => (a ? Promise.resolve(1) : 0); # will work| import { getClientCredentialsToken, getUserToken } from './xsuaa-service'; | ||
| import type { Service } from './environment-accessor'; | ||
| import { fetchIasToken, getIasAppTid } from './identity-service'; | ||
| /* eslint-disable import/no-internal-modules -- avoid circular imports via destination barrel */ |
There was a problem hiding this comment.
[req] Please consider to only disable this for a specific line. Alternatively we could also generally allow it for specific modules in the (local) eslint config.
There was a problem hiding this comment.
I've moved this to the eslint config, the following imports also need this and per-line comments would have required this:
// eslint-disable-next-line import/no-internal-modules
} from './destination/ias-types';There was a problem hiding this comment.
If this helps with circular dependencies, maybe it is better in the config even.
Co-authored-by: Marika Marszalkowski <marika.marszalkowski@sap.com>
Closes https://github.com/SAP/cloud-sdk-backlog/issues/1291
Related https://github.com/SAP/ai-sdk-js-backlog/issues/466
This PR adds the convenience functions
getIasToken()(returning token, refresh token (if available) and expiration) andgetIasDestination()(returning a callable destination).Both accept a service
Service | string | ServiceCredentials, unliketransformIasBindingToDestinationthey allow bareServiceCredentials(e.g. justclientid,clientsecretandurl).Note:
getIasToken()only returns the token as a string, rather than a parsed jwt, because it may not be a jwt.To support this more flexible usage, existing convenience logic (e.g.
app_tidresolution) has been refactored out oftransformIasBindingToDestination.Token caching now relies on the cached token flows added three days ago to
@sap/xssec4.13.0 (getClientCredentialsToken/getJwtBearerToken) instead of the SDK-level IAS client credentials cache to allow for caching closer at lower levels of the implementation.This means business user tokens are now cached too (https://github.com/SAP/ai-sdk-js-backlog/issues/466).
The
useCachetoggle is still exposed, but it is currently not possible to change the cache size or implementation via SAP Cloud SDK APIs.