CHT-datasource doesn't handle remote authentication

[dianabarsan]

Describe the bug
cht-datasource uses fetch to interact with a remote data source. However, all it receives when the datasource is initialized is the URL, but not authentication, and it further does not handle authentication at all.

cht-core/shared-libs/cht-datasource/src/remote/libs/data-context.ts

Line 71 in 35bb802

const response = await fetch(`${context.url}/${path}?${params}`);

And fetch does not support adding inlined basic authentication in the URL, for basic auth it requires an Authorization header.

There are some integration tests, but these are relying on enriching global fetch with a request-promise-native cookie jar that contains a session cookie:

cht-core/tests/utils/index.js

Line 68 in 35bb802

global.fetch = require('fetch-cookie').default(global.fetch, cookieJar);

We are removing

To Reproduce
Steps to reproduce the behavior:

1. Remove this line:

   cht-core/tests/utils/index.js

   Line 68 in 35bb802

   global.fetch = require('fetch-cookie').default(global.fetch, cookieJar);

2. Run integration tests.
3. See lots of person and place api failures.

Expected behavior
CHT-datasource should be independent of hacks we set up in e2e tests.

Environment

• Instance: local
• App: api
• Version: all

Additional context
As part of #6250 , and in conjunction with #8338 , I am removing all session setup and only relying on basic auth. CouchDb can now handle basic auth with many iterations out of the box, and maintaining sessions adds code complexity with we don't need anymore (fe. an entire PouchDb plugin which we now no longer need to maintain).

[jkuester]

I am removing all session setup and only relying on basic auth

Wait, so https://github.com/medic/pouchdb-session-authentication is going away? I thought we just switched to that? 🤔 😅

all it receives when the datasource is initialized is the URL, but not authentication, and it further does not handle authentication at all.

FTR, this was by design. We concluded it was unnecessary to do any authentication work in the initial MVP of cht-datasource since it would be able to inherent the session from the context that it was running in. Not sure I would characterize this as a "bug".

With the removal of session authentication, are we going to be in a situation where production workflows are going to be running with the user/pass included in the URL? It seems like this would not be the case in webapp/admin (which are the two places that the remote cht-datasource context is getting used). In api/sentinel, the local cht-datasource context is used (which uses Pouch instead of fetch). So, I guess I do not see where we would have an issue with the production code.

That does still leave us with the problem of how the integration tests for the remote cht-datasource context should work now. Ideally we could find a way to make this work without any "hacks", but if that is not possible I would prefer to hack the test code instead of adding unnecessary complexity to the implementation code...

[dianabarsan]

Wait, so https://github.com/medic/pouchdb-session-authentication is going away? I thought we just switched to that? 🤔 😅

Yes, this is going away. It's additional code we need to maintain and we don't need it anymore.

are we going to be in a situation where production workflows are going to be running with the user/pass included in the URL

No, we're going to pass an "Authorization" header with "Basic" authentication in api and sentinel. But some other services (like healthcheck) are still using auth in URLs I believe. We hadn't switched those to sessions yet.

how the integration tests

In my PR, I switched all datasource integration tests to hit the code through API endpoints. Up to you if you want to add other integration tests when implementing support for remote auth.

[jkuester]

I switched all datasource integration tests to hit the code through API endpoints

Have not looked at the actual code change here, but taking this statement at face value this seems like a significant regression in test coverage, leaving the remote context of cht-datasource untested for by any sort of integration tests. This also seems to directly contradict the agreement reached in a separate PR to duplicate the tests here.

The reason why I think integration test coverage is so important here is because the low-level nature of the cht-datasource code makes it pretty risky to assume it is sufficiently tested by other integration/e2e workflows. I am less worried about the local context flows because the integration tests hitting the api REST endpoints do pretty much call directly through to the local cht-datasource logic. However, the remote context code is currently only consumed by the admin app and by online-only users of webapp. To properly test the behavior of the remote context against real api REST endpoints we need these additional integration tests specific to the remote context.