-
Notifications
You must be signed in to change notification settings - Fork 65
feat: IAS App-To-App Auth #6185
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
Conversation
c0cc0e8 to
a541e18
Compare
0165671 to
fdb3d35
Compare
packages/connectivity/src/scp-cf/destination/destination-from-vcap.ts
Outdated
Show resolved
Hide resolved
marikaner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I finally got through. Big PR => Many comments. Good work 😄
packages/connectivity/src/scp-cf/client-credentials-token-cache.ts
Outdated
Show resolved
Hide resolved
packages/connectivity/src/scp-cf/client-credentials-token-cache.ts
Outdated
Show resolved
Hide resolved
packages/connectivity/src/scp-cf/destination/service-binding-to-destination.ts
Outdated
Show resolved
Hide resolved
| return options?.jwt?.app_tid; | ||
| } | ||
|
|
||
| requestAs satisfies never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q] What is the purpose of this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should type-check that all possible values of requestAs are handled before reaching else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Thanks!
| */ | ||
| app_tid?: string; | ||
| /** | ||
| * IAS tokens don't have scope property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q] Should it then be something like never or undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to stay compatible with the existing ClientCredentialsResponse, but considered that something of a breaking change (also XSUAA will likely always have this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| let urn = `urn:sap:identity:application:provider:clientid:${resource.providerClientId}`; | ||
| if (resource.providerTenantId) { | ||
| urn += `:apptid:${resource.providerTenantId}`; | ||
| } | ||
| return urn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I see let I get the urge to remove it. Not sure this is better though. Just an idea. Feel free to keep your version.
| let urn = `urn:sap:identity:application:provider:clientid:${resource.providerClientId}`; | |
| if (resource.providerTenantId) { | |
| urn += `:apptid:${resource.providerTenantId}`; | |
| } | |
| return urn; | |
| const resourceUrn = [resource.provderClientId, resource.providerTenantId].filter(val=>val).join(':apptid'); | |
| return `urn:sap:identity:application:provider:clientid:${resourceUrn}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it with a slightly different approach.
| * @returns A promise resolving to the client credentials response. | ||
| * @internal | ||
| */ | ||
| async function getIasClientCredentialsTokenImpl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[req] Every function is an implementation of something. The name currently sounds like we get some implementation. I assume this is not what is meant. Can we just remove the Impl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name is fine getIasClientCredentialsToken is the inner implementation of getIasClientCredentialsToken which iirc calls it with additional resilience.
KavithaSiva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also add tests for getDestinationFromServiceBinding with the authentication type Oauth2JwtBearer.
Currently, only this method supports this authentication type, transformServiceBindingToDestination always creates a destination with Oauth2ClientCredentials. Could you also please confirm if the transform function for other services also always create a Oauth2ClientCredentials destination?
| ); | ||
| } | ||
|
|
||
| return buildIasDestination(response.access_token, service, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[req] I checked the flow again here and when iasOptions.authenticationType is set to OAuth2JWTBearer, the built IasDestination is still always one with OAuth2ClientCredentials which is incorrect as we do the access token retrieval using the jwt bearer token flow.
So, the authentication type OAuth2JWTBearer should be retained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this by allowing authenticationType overrides for buildClientCredentials, and forwarding it in buildIasDestination.
|
Only Should I also forward the refresh token to the destination object if available? This would mean extending I've also noticed that none of the |
I didn't understand this fully, in which API do you want to do this? Where is the refresh token available? |
I think the best place to put refresh tokens would be |
I think that's fine because the Here we are creating destinations by transforming a service binding, I am not sure if they really add any value if set. |
Understood, let's do this in the next IAS follow-up ticket, which may need and use the refresh token. |
KavithaSiva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last round of minor comments to address and then it's good to go.
Code looks much cleaner and readable now, thanks for all the effort and good work.
packages/connectivity/src/scp-cf/destination/service-binding-to-destination.ts
Outdated
Show resolved
Hide resolved
packages/connectivity/src/scp-cf/destination/service-binding-to-destination.ts
Show resolved
Hide resolved
KavithaSiva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's go!
Handles SAP/ai-sdk-js-backlog#347.