every endpoint function asks for an api_token

[cainlevy]

All of the endpoint functions exported by libmalan require a MalanConfig object for the first argument. This means every endpoint asks for an api_token key, even when the intent of the endpoint is to generate an API token. When combined with the README.md's examples, it's easy for devs to fall back on a global config object with the potential to leak tokens. I've seen it happen twice so far.

A naive fix might be to make MalanConfig.api_token optional. This would make the typing less useful on endpoints that do require an API token. It would be nice if each endpoint was precise about what it needed.

My instinct would be to solve this by storing the host value ahead of time, whether by class or closure or global. This would leave the api_token to be requested by the endpoint functions that need it.

[FreedomBen]

@cainlevy Yeah I agree this is definitely something that can be improved. When I originally wrote this lib it was just being used from a React SPA, which had a single user logged in, so sticking the MalanConfig in redux and pulling it out to call the API was nice and convenient. It (along with other redux state) could also be serialized/deserialized trivially into session storage. It also kept the library functional and stateless where functions operated only on their arguments and don't rely on side effects or previously established state.

However I agree, that pattern is definitely sub-optimal (and encouraging of bad behaviors) for calling from a backend service that supports multiple users. So, something should be done! Ideally it would be great if we could accommodate both approaches without a bunch of code duplication and/or higher complexity from too many ways of doing things. :thinking:

The closure approach appeals to me, but I'm too much of a typescript n00b to envision how that would work with typing. Having a class that basically wraps the api token and a couple other convenience things, which passes the malan config in seems like it would be a fairly simple approach. The biggest downside is that for each function there'd be one in the class and one in the module. I hacked out a quick pseudo-codish version to get a feel, and idk :shrug:

class Malan {
  hostname: string;
  api_token: string;

  constructor(hostname: string, api_token?: string) {
    this.hostname = hostname;
    this.api_token = api_token;
  }

  getConfig() {
    return {
      hostname: this.hostname,
      api_token: this.api_token
    }
  }

  /* login function updates the api token for this instance */
  login(username: string, password: string, expires_at: datetime): void {
    const resp = libmalan.login(username, password, expires_at)
    if (resp.success) {
      this.api_token = resp.data.api_token;
    } else {
      throw MalanError("failed login or something like that");
    }
  }

  /* The rest of these methods are just wrappers around the module functions */

  getUser(id: string): User {
    return libmalan.getUser(this.getConfig(), id);
  }

  listUsers(): [User] {
    return libmalan.listUsers(this.getConfig());
  }

  // something similar to the above for each function in users/sessions
}

Do you have any ideas?

[FreedomBen]

Had a thought, maybe I'm over thinking the problem. If already have an api_token for the user it would be stored somewhere like in a db column right? There's a common pattern I've used before in other apps, what about a functions like this somewhere:

function malanConfig(api_token: string = '') {
  return {
    hostname: process.env.MALAN_HOSTNAME,
    api_token: api_token
  }
}

// To call malan
malan.createUser(malanConfig(), user_params) // no api token needed
malan.login(malanConfig(), "username", "password") // no api token needed
malan.whoami(malanConfig(user.api_token)) 
malan.whoamiFull(malanConfig(user.api_token))  // basically same as getUser() but uses the user ID of the token owner
malan.getUser(malanConfig(user.api_token), "someusername_or_id") 

It would be nice if each endpoint was precise about what it needed.

Yeah I think that's a great idea. After reading what you wrote again, do you think that would fix the original problem of devs saving API tokens into a global thing that can leak and have race conditions? (I think it's a good fix either way though)

[cainlevy]

When I originally wrote this lib it was just being used from a React SPA, which had a single user logged in, so sticking the MalanConfig in redux and pulling it out to call the API was nice and convenient.

Ooo, this is helpful context.

If already have an api_token for the user it would be stored somewhere like in a db column right?

The client stores a token in local memory, then sends it to the API on every request via the Authorization header. The API parses the token out and exchanges it for a malan ID, which we use to find the current user. At that point we don't have any further use for the token and leave it for GC.

In our setup, it's just a session token. We don't currently make any other requests to Malan on the user's behalf.

do you think that would fix the original problem of devs saving API tokens into a global thing that can leak and have race conditions

I think there are two contributing factors. I'm revising these a bit based on what you've clarified:

1. the pattern in README.md, which is probably from the single-user browser client days, suggests a mutable global
2. the functions promote passing an api token all the time, which is most easily satisfied with a global

The config factory pattern you wrote would mitigate both of those factors somewhat, I think, but I'm wondering if there's a way to build the best practices into the syntax. Ideally it would be hard to get wrong.

I think the first step might be moving the api_token out of configuration. Configuration is usually a static thing, especially in a server client where developers are used to extracting configuration into ENV vars. Every other client we use has provisioned credentials, so it's easy to bring the wrong expectations when supplying MalanConfig.

The second step could just as easily be a naming thing. Consider:

• malan.getUser(config, id)
• malan.getUserByToken(config, token)

Having a class that basically wraps the api token and a couple other convenience things, which passes the malan config in seems like it would be a fairly simple approach. The biggest downside is that for each function there'd be one in the class and one in the module.

Not the worst outcome! It's a bit of tedium for the library maintainer.

How do you feel about partial application? It could result in slightly less boilerplate:

function createUser(config) {
  return (id) => {...}
}

function MalanClient(config) {
  return {
    createUser: createUser(config),
  }
}