http: dies on platform if no credential

[josephjclark]

The http adaptor does this:

export function addBasicAuth(configuration = {}, headers) {
  const { username, password } = configuration;
  if (username && password) {
    Object.assign(headers, makeBasicAuthHeader(username, password));
  }
}

(probably other adaptors too)

This blows up on v1 if there's no credential, because v1 passesnull as the credential. And null is a value, it won't trigger the default.

While we're still supporting v1, we need to be careful about defaulting the configuration object across our adaptors.

we should really fix this in http because latest will be forever broken on v1, which isn't so good

[mtuchi]

@josephjclark Does this still need fixing ?, I am seeing a much better error on [email protected] [current latest version] when running a simple job

Eg:
get('mama')

[aleksa-krolls]

@mtuchi was something done in a recent release to address this?

Bc in v6.4.3 or v.6.4.2 it was causing the workflow to crash if no baseUrl was defined or if credential was not added at all to the job (no configuration values present).

[mtuchi]

From our pairing session with @aleksa-krolls, Tested on v2 a simple job without credentials using http adaptor
v6.2.0 to 6.4.4 it works as expected.

get('/jokes/random')
get('https://api.openconceptlab.org/mappings/')

[josephjclark]

We know it works on v2, the problem is only seen on v1. It doesn't matter that we get a nice error - the problem is it shouldn't error at all.

So yes, this still needs fixing unfortunately

[mtuchi]

@josephjclark But isn't INVALID_URL the expected error because you don't have any baseUrl if your making this request get('/jokes/random') ?

Regarding v2 i was pairing with @aleksa-krolls yesterday, She mentioned she faced something similar on v2 but we couldn't reproduce it during the call. The only error we were getting is INVALID_URL when you make a get request to get('/jokes/random')

[josephjclark]

Hmm. It's a badly written issue and I only barely remember it. But I think the problem is that get('https://api.openconceptlab.org/mappings/') fails on v1 if there's no credential. It's the thing about the credential being null. You could probably reproduce that in a unit test. Or using the CLI, maybe set state.configuration to null. That'll probably cause a similar exception.

get('mama') is indeed an INVALID_URL if there's no baseURL in the credential, but I don't think that's what we're talking about. We should handle that case better by saying "You passed a relative URL but didn't set baseURL" or something.

[aleksa-krolls]

Ah I agree with Joe @mtuchi ... didn't think about this use case, but some jobs do not require authentication and no credential... so that should be allowed.

[mtuchi]

EOD Update #772
I have improved the error logs in http adaptor when baseUrl is not set and the relative url is provided. And also if baseUrl is not set and the url is not provided

[josephjclark]

Thanks @mtuchi, that's cool, but that PR doesn't fix the actual issue here.

You need to change:

export function addBasicAuth(configuration = {}, headers) {
  const { username, password } = configuration;
  //  ...
}

To this (or some equivalent:

export function addBasicAuth(configuration, headers) {
  const { username, password } = configuration ?? {};
  //  ...
}

To understand what's happening, try this test in node or your browser devtools:

function fn(x = {}) {
  return x
}

console.log(fn(1)) // returns 1
console.log(fn(undefined)) // returns {}
console.log(fn(null)) // returns null! 🤯

[mtuchi]

@josephjclark i have updated the addAuth in Utils.js to use const { username, password, access_token} = configuration ?? {}; But something to note, I have manage to reproduce the bug in v6.1.0

But in v6.4.4 it's working as expected without credential