SAP / cloud-sdk-js

Use the SAP Cloud SDK for JavaScript / TypeScript to reduce development effort when building applications on SAP Business Technology Platform that communicate with SAP solutions and services such as SAP S/4HANA Cloud, SAP SuccessFactors, and many others.
Apache License 2.0
167 stars 57 forks source link

getTargetUri in /packages/core/src/connectivity/scp-cf/xsuaa-service.ts automatically appends /oauth/token to the uri #816

Closed blaart closed 3 years ago

blaart commented 3 years ago

We are using a token server of Microsoft for a user token grant request. the url is like: https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token

see also: https://docs.microsoft.com/en-us/graph/auth-v2-user#3-get-a-token

We really want to use the sdk to resolve our destinations, but it does not work for this url as the getTargetUri function in /packages/core/src/connectivity/scp-cf/xsuaa-service.ts automatically appends /oauth/token to the uri, which results in https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token/oauth/token. Which results in a 404 Not Found.

I expect the given Token Service URL in the destination not te be changed.

blaart commented 3 years ago

I now see that this was fixed a few days ago in 1.33

artemkovalyov commented 3 years ago

We're happy it worked for you @blaart. Let us know if anything else comes up.

blaart commented 3 years ago

Hi Artem, Thanks for your reply, I based my conclusion on the changelog. While testing I still see that /oauth/token is added. I think this is the problem: In /packages/core/src/connectivity/scp-cf/xsuaa-service.ts

In the post function the getTargetUri is still called: https://github.com/SAP/cloud-sdk-js/blob/70c0b652b63764b2a74f46bb1a6b2d568c6a37bf/packages/core/src/connectivity/scp-cf/xsuaa-service.ts#L231

Should it be fixed when you move this call to here: https://github.com/SAP/cloud-sdk-js/blob/70c0b652b63764b2a74f46bb1a6b2d568c6a37bf/packages/core/src/connectivity/scp-cf/xsuaa-service.ts#L221 ?

jjtang1985 commented 3 years ago

Hi @blaart, the fix that you proposed makes sense, which should work as we planned a similar way. However, to do that, we would like to build our end to end test as our quality assurance, which was also planned in the backlog.

Now, since the previous workaround does not solve your issue, I would like ask whether your application is a PoC or live system so that we can discuss again in the refinement meeting.

Best regards, Junjie

blaart commented 3 years ago

Hi @jjtang1985, We are trying to improve our live system by introducing the SDK for integration questions. When this is solved together with https://github.com/SAP/cloud-sdk-js/pull/792 we can use the SDK for every integration/destination we use. Kind regards, Peter

FrankEssenberger commented 3 years ago

Good morning @jjtang1985 @blaart,

I would suggest the following: We try to release #792 quickly and also make a fix for the token url bug you are experiencing. Once these are merged you can immediately (a few minutes for the publish to npm) use the canary version and try it out. In the meantime we can work on the E2E to cover also the new token flows to ensure quality on the 1.34.0 release.

blaart commented 3 years ago

Yeah no problem, I can use the canary version to see if it works for me

FrankEssenberger commented 3 years ago

Hello @blaart,

sorry I forget to write you last week. We merged to two PRs #792 and #823 which should make it possible to use OAuth2UserTokenExchange and also fix the URL issue. You can use the canary version to try it our. I am currently including E2E tests and I hope that everything works fine. Also if you experience issue let us know.

Best Frank

blaart commented 3 years ago

Hi Frank, sorry for my late response, I have a few days off. But I will take a look at this asap. I see I can use version 1.34 right?

blaart commented 3 years ago

Is also the OAuth2UserTokenExchange added in the v1.34? (it is not mentioned in the changelog)

marikaner commented 3 years ago

Hey @blaart,

Yes, OAuth2UserTokenExchange was added in v1.34.0. The entry in the changelog was accidentally added to a previous version. I fixed that. As it was extensively tested by my colleagues, I will now assume that the provided fix solves this issue and I will close it. In case you still encounter issues, let us know.

blaart commented 3 years ago

Thanks guys, the OAuth2UserTokenExchange works fine! Happy with that! For the OAuth2ClientCredentials I opened a new issue as (for microsoft's token server) the scope is required.