jcailan / cdse

CDS Extension for External Service Consumption
MIT License
9 stars 6 forks source link

Credentials issue with destination #4

Open hschaefer123 opened 3 years ago

hschaefer123 commented 3 years ago

Hi Jhodel, i was just externalizing my

package.json

{
  "cds": {
    "requires": {
        "API_SALES_ORDER_SRV": {
                "kind": "odata",
                "model": "srv\\external\\API_SALES_ORDER_SRV",
                "credentials": {
                    "destination": "udina_commerce_shop",
                    "path": "/sap/opu/odata/sap/API_SALES_ORDER_SRV",
                    "requestTimeout": 30000
                }
            }
        }
    }
}

while using a default-env.json to externalize the credentials

default-env.json

{
    "VCAP_SERVICES": {},
    "destinations": [
        {
            "name": "udina_commerce_shop",
            "url": "http://xxx.sap.de:1234",
            "username": "xxx",
            "password": "yyy"
        }
    ]
}

if i am using your

const cdseSrv = await cdse.connect.to('API_SALES_ORDER_SRV');

i got errors

[ERROR] VError: No service matches destination
    at Object.getServices (C:\VSC\@udina\udina.trial\cap\node_modules\sap-cf-destconn\node_modules\@sap\xsenv\lib\xsservices.js:50:15)
    at getService (C:\VSC\@udina\udina.trial\cap\node_modules\sap-cf-destconn\dist\destination.js:120:35)
    at C:\VSC\@udina\udina.trial\cap\node_modules\sap-cf-destconn\dist\destination.js:28:48
    at Generator.next (<anonymous>)
    at C:\VSC\@udina\udina.trial\cap\node_modules\sap-cf-destconn\dist\destination.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (C:\VSC\@udina\udina.trial\cap\node_modules\sap-cf-destconn\dist\destination.js:4:12)
    at readDestination (C:\VSC\@udina\udina.trial\cap\node_modules\sap-cf-destconn\dist\destination.js:27:12)
    at C:\VSC\@udina\udina.trial\cap\node_modules\cdse\index.js:17:4
    at new Promise (<anonymous>)

I think, you currently do not take care of the env destination settings.

Running with inline credentials and commenting "_destination" service is working, but i want to use it the sames way like the productive szenario.

Would be great, if you would support local destinations inside env for local testing. Also this way is more secure, because default-env.json is ignored by git.

BTW: Very nice implementation for CAP! ;-)

Best Regards Holger

jcailan commented 3 years ago

Hi Holger @hschaefer123

Thanks for raising this issue and providing your findings!

I will look into it and see what I can do to add support for default-env.json.

Cheers, Jhodel

jcailan commented 3 years ago

Hi Holger @hschaefer123

I looked into this today, tested it locally while connecting to SCP Destination. See my answer to the below question:

https://answers.sap.com/questions/13219399/cdse-is-not-working-cdseconnecttonorthwind.html?childToView=13222574#answer-13222574

My guess is that your use case is slightly different -- am I right?

You want to avoid putting the credentials on the CDS configuration (in package.json), is my assumption correct?

hschaefer123 commented 3 years ago

Hi Jhodel, yes you are right!

Since package.json it in git commit, this is dangerous. The SAP solution supporting .env or default-env.json is more safe, because this files are autexcluded in .gitignore.

I think remotely in CF, your code also works as expected.

Locally, i am trying to figure out some pros/cons for different solutions!

With the above setup,

cds

const soSrv = await cds.connect.to('API_SALES_ORDER_SRV');

will take care of default-env.json (and maybe also .env) to use local destination configuration (merging cds and env)

cdse

const cdseSrv = await cdse.connect.to('API_SALES_ORDER_SRV');

runs into the above error i am using cdse, because i need to POST data with X-CRSF-TOKEN (the reason for your package ;-) i have your code side-by-side to the cds version, so i recognized the different behavior. would be great, if you can handle it similar to cds.connect

cloud sdk using vdm

const SalesOrder = require('@sap/cloud-sdk-vdm-sales-order-service').SalesOrder
const destination = { destinationName: 'udina_commerce_shop' }
const result = await SalesOrder.requestBuilder().execute(destination)

also works fine without any changes, supporting default-env.json

cap using cloud sdk generic rest client

const { executeHttpRequest } = require('@sap-cloud-sdk/core')
const destination = { destinationName: 'udina_commerce_shop' }
const result = await executeHttpRequest(destination, { method: 'get', url: url, params: params });

also works fine without any changes, supporting default-env.json

BTW: did you also thaught about utilizing the executeHttpRequest? https://sap.github.io/cloud-sdk/docs/js/features/connectivity/generic-http-client/ Even if this is used for the OData client, in case of native calls, it does not support ETAG and CRSF.

Please let me know, if you need more input.

Best Regards Holger

jcailan commented 3 years ago

Hi Holger @hschaefer123

I tried cloud sdk before but it got stuff there that I didn't find in sync with CAP -- like:

So your suggestion is to configure the credetials in default-env.json like below:

{
    "VCAP_SERVICES": {},
    "destinations": [
        {
            "name": "udina_commerce_shop",
            "url": "http://xxx.sap.de:1234",
            "username": "xxx",
            "password": "yyy"
        }
    ]
}

I suggest to keep the default-env.json to SAP and use .env file for CDSE instead. Sample config on package.json:

{
  "cds": {
    "requires": {
        "API_SALES_ORDER_SRV": {
                "kind": "odata",
                "model": "srv\\external\\API_SALES_ORDER_SRV",
                "credentials": {
                    "url": "http://xxx.sap.de:1234",
                    "username": "{{API_USERNAME}}",
                    "password": "{{API_PASSWORD}}"
                }
            }
        }
    }
}

The values in here will get replaced by the one configured on .env file:

                    "username": "{{API_USERNAME}}",
                    "password": "{{API_PASSWORD}}"

Your thoughts?

hschaefer123 commented 3 years ago

Hi Jhodel, that does not help in my case!

I will explain it a little bit more.

The original package/cds contains

packages.json | .cdsrc.json

{
  "cds": {
    "requires": {
        "API_SALES_ORDER_SRV": {
                "kind": "odata",
                "model": "srv\\external\\API_SALES_ORDER_SRV",
                "credentials": {
                    "destination": "udina_commerce_shop",
                    "path": "/sap/opu/odata/sap/API_SALES_ORDER_SRV",
                    "requestTimeout": 30000
                }
            }
        }
    }
}

This will be the productive setting, that will be deployed to CF. Also the path is relevant, if you want to use the sames destination for SAPCloudSDK and CAP external service.

Inside the credentials will be at least the destination name, and if the destination names is not used, CAP will use the property name "API_SALES_ORDER_SRV" instead.

Local Testing

I think you formerly also commented it out like "_destination" for local testing, but using it the productive way, you do not have to touch the packages.json for local development.

For local testing, only the node env setting is relevant.

You have two option:

All this kinds are supported by CAP.

I am prefering default-env.json, because the settings can be easily declared as an object. Also if you will use tremotely destinations, you would copy the VCAP_SERVICES sections directly from CF and then getting the destination services into account (which is not working for onPremise scenarios, so i always have to tunnel it locally using VPN).

default-env.json

{
    "VCAP_SERVICES": {},
    "destinations": [
        {
            "name": "udina_commerce_shop",
            "url": "http://xxx.sap.de:1234",
            "username": "xxx",
            "password": "yyy"
        }
    ]
}

.env

destinations=[{"name":"udina_commerce_shop", "url":"...", ...}]

This is the same behavior, like the @sap/approuter is working

Note: In case destination with the same name is defined both in environment destination and destination service, the destination configuration will load from the environment. Destination service available only in Cloud Foundry. * Destinations on destination service instance level are supported.

CAP is just accessing process.env.destinations and merging multiple sources into it.

Would be great, if you would use the same logic chain, like CAP/Destination service:

Anyhow which env injection you prefer, all of them should/can be supported, because you will have them availabe inside process.env.destinations. I think you just need to check if process.env.destinations[targetDestinationName] is defnied and the use it. If not, apply your current logic.

Also default-*.json and .env are part of .gitignore so credentials are safe from beeing committed. I do not like the recommendation inside CAP docu, because it is not quit clear!

What do you mean?

Best Regards Holger

jcailan commented 3 years ago

Hi Holger @hschaefer123

I think I get what you mean. However, I don't see from your approach that you attempted to use CAP's concept of configuration profiles. Configuration profiles keeps your local settings separate from your productive setting.

See example below:

image

If you use this approach then there shouldn't be any problem with using .env file. I'm trying to steer clear from using default-env.json because that approach is trying to mixin with the CF concepts wherein the introduced property destinations is practically a foreign entity.

hschaefer123 commented 3 years ago

Hi, yes you are right, if your need to handle config different for prd/test, this is the prefered way, but having a look into cap internals

cds-runtime/lib/rest/service.js

const _checkProduction = destination => {
  if (!destination && process.env.NODE_ENV === 'production') {
    throw new Error('In production mode it is required to set `options.destination`')
  }
}

also handles this and later on

if (!this.destination) this.destination = getDestination(this.model, this.datasource, this.options)

I do not do something specific, because this is supported out-of-the-box by cap. I was just replacing cds.connect with cdse.connect because i think this was the intention from you to use it the same way like in cap, but supporting CSRF tokens. I like this approach very much and it would be great, if you would follow the same getDestination chain like cap.

Generally i am trying using as much as possible from the core, but in this special situation, i see the need for your little nice CDSE project ;-)

The other way around would be the need to extend the orinal CAP to support CSRF for POST like you did.

In my scenario, i do not need to differ between production and local, because cap supports it directly. I just need to hav a different local env to overcome destination and use locally defined one.

But what would be the difference in your upper config? Just to have the same config, but one config without the destination settings? I still do not want to have my local credential settings inside my packages.json file!

Also i am using the same approach in approuter, see https://blogs.sap.com/2019/10/24/test-your-cap-nodejs-applications-with-authentication-locally/ and special section running locally.

We have to define this destination for our local environment. create a default-env.json file in the approuter folder.

{
"destinations": [
{
"name": "srv_api",
"url": "http://localhost:4004",
"forwardAuthToken": true,
"strictSSL": false
}
]
}

The reccomendations here are https://gregorwolf.github.io/SAP-NPM-API-collection/apis/approuter/#configurations Configurations from the environment - these configurations are either read from the application router's environment (when deployed on Cloud Foundry or XS Advanced OnPremise Runtime) or from the default-env.json file (when running locally). Refer to the documentation of the @sap/xsenv package for more details.

Or the @sap/xsenv see section local usage https://gregorwolf.github.io/SAP-NPM-API-collection/apis/xsenv/#local-usage

You can provide default configurations on two levels: For bound services via getServices() and default-services.json For any environment variable via loadEnv() and default-env.json

But it depends on you, if you will add this compability to sap cap cds.connect.to.

Finally i choosed default-env.json, because it is used most common among libraries (one file fits all ;-)

Best Regards Holger