Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

commcare: standardise auth #554

Closed
josephjclark opened this issue May 10, 2024 · 1 comment
Closed

commcare: standardise auth #554

josephjclark opened this issue May 10, 2024 · 1 comment
Assignees

Comments

@josephjclark
Copy link
Collaborator

josephjclark commented May 10, 2024

commcare at the moment uses two different auth schemes: basic and api key.

This is fine, but a big problem is that different functions supports different schemes.

I presume that any commcare API can use either auth, but this needs validating.

We should ensure that the new request helper is smart enough to load the two different credential types from config, and set up the header appropriately.

Design

Here's what I suggest

When calling request, we should pass state.config as the first argument:

request(state.configuration, { path: 'case' })

That way, you don't need to pass the whole state object.

The request signature should like be:

request(auth: Object, options: Object): Promise<>

We should not need to pass authType into the request at all.

Inside request, we set set the Authorization differently depending on the shape of auth:

  • If there's an apikey,we do Authorization: ApiKey ${username}:${apiKey},
  • Otherwise if there's a password, we do: Authorisation: ${Buffer.from(${username}:${password}).toString('base64')}
  • Otherwise we should probably throw an error like "no authorisation credentials provided"
  • Note that we don't do any of this if an Authorization header has already been set

We probably want a helper function like:

function configureAuth(auth, headers = {}) {
   // ...
  return headers
}

I'd love to see some unit tests against that function!

@hunterachieng
Copy link
Contributor

@aleksa-krolls This has been merged into this branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants