feat: improve support for IAS tokens #4799
Conversation
packages/connectivity/src/scp-cf/destination/destination-from-service.ts
Outdated
Show resolved
Hide resolved
| getSubdomain(this.subscriberToken?.serviceJwt?.decoded) || | ||
| getSubdomain(this.subscriberToken?.userJwt?.decoded); |
There was a problem hiding this comment.
Previously we only looked at the subdomain from the user JWT. However, if the user passed a separate iss property this should be the actual subdomain and it should take precedence (right?).
If the iss property was passed, the service JWT was fetched with that subdomain and should have the same subdomain as it is an XSUAA token. If it was not possible to fetch the service token with this subdomain, I assume the subdomain isn't valid anyways.
packages/connectivity/src/scp-cf/destination/destination-from-service.ts
Outdated
Show resolved
Hide resolved
packages/connectivity/src/scp-cf/destination/destination-from-service.ts
Outdated
Show resolved
Hide resolved
| DestinationFromServiceRetriever.checkDestinationForCustomJwt(destination); | ||
| } | ||
|
|
||
| // Case 1 Destination in provider and JWT issued for provider account, but no custom JWT given -> no extra x-user-token header needed | ||
| // Case 1: subscriber account is the provider accound, user JWT is from XSUAA | ||
| // x-user-token header not needed |
There was a problem hiding this comment.
why not? I can't think of a reason why subscriber/provider should have anything to do with the question if we should forward the token or not.
There was a problem hiding this comment.
Just FYI: I did not change the logic, only improved the comment here.
I would also assume we just send the header in all the cases, however according to this documentation: https://help.sap.com/docs/connectivity/sap-btp-connectivity-cf/exchanging-user-jwts-via-oauth2usertokenexchange-destinations
In this exact case it doesn't happen and we have to do the exchange on our own and send it as authentication header. I remember we tried this once in the E2E tests and passing it in the header in fact didn't work.
There was a problem hiding this comment.
I don't see any restrictions in the documentation why sending the header should not work for this auth type specifically. In fact, the Java SDK always sends the header, including for this auth type. So at least I am not aware of any reason, why this shouldn't work (assuming the OAuth service trusts and accepts the given user JWT).
If we have some test, where an XSUAA based token is not accepted in this manner, I'd like to look at it and potentially report that to the destination service.
There was a problem hiding this comment.
I am really not sure what the problem was, but I remember, that something did not work, I just don't know what. I would be willing to try this out, but definitely on a separate PR and probably after my vacation, if priorities allow it.
| ); | ||
| } catch (err) { | ||
| logger.warn( | ||
| 'Failed to fetch subscriber service token for destination. This is only relevant if you are using subscriber destinations.' |
There was a problem hiding this comment.
currently this discards the error, maybe print it to debug to give users some options to get to the root cause, in case they need to? At least that's what we typically do in Java..
There was a problem hiding this comment.
Also, I'm wondering what the reason for this change is, considering that this threw before. What changed?
There was a problem hiding this comment.
currently this discards the error, maybe print it to debug to give users some options to get to the root cause, in case they need to? At least that's what we typically do in Java..
I think it is a nice idea to add the error. Debug would hide it even more. I could just add it to the warning. What do you think?
Also, I'm wondering what the reason for this change is, considering that this threw before. What changed?
This is not obvious from the code at first glance, but previously this request was only executed (and thereby potentially throwing) when there was an iss property set or the given token was an XSUAA token. Now it would be executed for all tokens, if given.
There might be cases where I don't care about subscriber destinations, therefore I wouldn't need a subscriber token and wouldn't care about it failing.
I considered rethrowing the error in case the iss property was set or the token was an XSUAA token instead. Do you think that would be better?
There was a problem hiding this comment.
I still don't understand this "subscriber" aspect, so I don't know what this call is supposed to do, and in which cases this call failing can have a reasonable recovery..
There was a problem hiding this comment.
This function is called to fetch a token for the subscriber. As we currently ALWAYS retrieve a subscriber token, when a user JWT is available, it is possible that we call this, although it isn't needed, because users are actually in the provider case. This is a very obvious flaw, but I don't dare touching this at the moment.
No worries! Thanks for the review! 😊 |
…service.ts Co-authored-by: Matthias Kuhr <[email protected]>
| @@ -70,7 +70,7 @@ describe('getSubscriberToken()', () => { | |||
| expect(serviceTokenSpy).toHaveBeenCalledWith( | |||
| 'destination', | |||
| expect.objectContaining({ | |||
| jwt: { iss: onlyIssuerXsuaaUrl } | |||
| jwt: { ext_attr: { zdn: 'subscriber-only-iss' } } | |||
There was a problem hiding this comment.
Previously we mimicked a token with an issuer here. I changed this to be reflected in the zdn property, because the issuer should only be checked in the case we have an XSUAA token. If it is an IAS token, the subdomain of the tenant cannot be parsed from the issuer. Therefore we now first look at the zdn property and only if this is not there look at the issuer, if this is an XSUAA token.
packages/connectivity/src/scp-cf/destination/get-subscriber-token.ts
Outdated
Show resolved
Hide resolved
| @@ -13,7 +16,8 @@ export async function exchangeToken( | |||
| options: DestinationOptions | |||
| ): Promise<string> { | |||
| const xsuaaService = getXsuaaService({ | |||
| disableCache: !options.cacheVerificationKeys | |||
| disableCache: !options.cacheVerificationKeys, | |||
| credentials: getDestinationServiceCredentials() | |||
There was a problem hiding this comment.
Since my knowledge of IAS is rather limited I don't know why we do this, why exactly do we pass the destination service credentials as XSUAA service credentials here?
There was a problem hiding this comment.
because we are retrieving this token only for the destination service, so this should be enough. Honestly, we need to clarify, whether there are cases where we would need to do this for connectivity. I am even thinking splitting this off this PR.
This reverts commit 18a2fdc.
# Conflicts: # packages/connectivity/src/scp-cf/jwt.ts # packages/connectivity/src/scp-cf/tenant.ts # packages/connectivity/src/scp-cf/xsuaa-service.ts
packages/connectivity/src/scp-cf/destination/destination-service.ts
Outdated
Show resolved
Hide resolved
This includes generalized handling of tokens so that XSUAA, IAS and other custom tokens.
I fixed some places to make sure the difference between subdomain (aka. tenant, aka. tenant name) and tenant ID (aka. zone ID, aka. app_tid) is hopefully more obvious and in some cases more correct.
Especially the subdomain needs to be handled differently between XSUAA and IAS.
Closes SAP/cloud-sdk-backlog#1219.
Closes SAP/cloud-sdk-backlog#1217.