Make user data private

[scholvat]

This adds an authentication token to every API request, in preparation for the backend to change to always require a token to get data.

[picklechips]

I don't think we should be doing this in the ApiClient since then it sends the token along with every request, which might not be necessary. It might not matter now but it doesn't feel right to me, since if a request doesn't need it we probably shouldn't be sending it. It could slow down things too since we're waiting or the token on every request, so for requests where we might already have the token saved locally or it doesn't require a token this adds unnecessary overhead.

I would leave the ApiClient alone, and just pass it in in the headers parameter on each individual request.

@aopal @neilbrub what do you guys think about this

[scholvat]

Incorporating it into the apiClient makes the code a lot cleaner. I could change it to be cached in apiClient, and only have to grab the token from storage when starting the app. This would make it faster overall, though I'm not sure if that would have any security issues.

[picklechips]

I personally don't think having to pass one extra field into the call where we need it is necessarily unclean, especially since it allows us to be more explicit with where/when we use the token.

Caching it in the ApiClient might be a good solution for performance reasons, but to me it still just feels wrong to send it on a request when the request doesn't require it - so I think I would still prefer to just include it as a parameter on the specific call.
But by all means maybe that's just me being picky/unreasonable and its not really an issue - so I'd be interested in hearing what others have to say.

cc @neilbrub @aopal

[neilbrub]

Hmm yeah. I'm not super polarised on this, I think I would lean towards adding auth header on individual requests though. It's more symmetric with how we implement it server-side (requiring verifyToken on specific endpoints), and I'm personally ok with being explicit even if it means a few more lines of code. Making a util method or something we can call to get a stored token might help with cleanliness and let us optimise performance centrally.

[scholvat]

What about having the functions look like get(endpoint, headers, authorized, body){} where authorized is a boolean which controls if the token should be used or not? It would still cache the token to improve performance, eliminate some lines of code from the rest of the app, and still explicitly say if the request needs the credentials or not.

[neilbrub]

Sorry @scholvat I should have replied to this earlier - definitely like this architecture, the api method signatures thing can wait if we want to merge this in soon!

[scholvat]

I've updated it with more selective authroization such that the authorization token isn't always sent, and the token is cached. Also merged in the don info stuff

[neilbrub]

Something about having to pass a boolean for a specific task like auth as a standalone argument makes me hurt a little inside, especially for instances where we need to pass an empty headers object, ie api.post('/some/endpoint', {}, true, body_data).

I definitely do like the architecture of each request specifying whether it should be authenticated and how ApiClient handles tokens centrally (nice work Stephen!), just wonder if we might want to consider switching up the signature for our Api methods to have an options parameter to hold data like headers and booleans. i.e:

static async post(endpoint, body, options={}) {
  if (options.authenticate) //...
}

invoked as api.post('/some/endpoint', body_data, { authenticate: true }), with a headers property in the third argument if needed (which looks a little messy but is rarely needed). This catch-all options object would give us a better set-up for other spiciness we might want to add to requests in future, without having to extend the arguments list and dodge the ones we don't need to satisfy the method signature.

HOWEVER, as it is right now it's not that bad (only 4 args) and likely won't get that much bigger, plus having a headers argument makes a fair amount of sense for an Api method. What do you think @scholvat and @picklechips?
We'd have to go through most api methods and invocations to change this so I'd be happy to merge it as-is for now and make a small PR for it after.

[scholvat]

I like your suggestion of using an option parameter, I didn't like how messy the signature was getting and that is a good solution. I think it's better to make a clean solution now, and that way we'll hopefully never have to go through and update every api call again.
(Sidenote, I like your use of "spicy" to describe code features)

[neilbrub]

Sounds good, t'is probably better to get 'er all done now. And thanks 😉

[scholvat]

Updated with the new API from @neilbrub 's suggestions

[neilbrub]

LGTM, sick work!

[picklechips]

I like this a lot, much cleaner than what we had before! Bit of a nit pick but before merging can we replace the usage of var with const or let? It's just nicer, more modern syntax.

Really good stuff tho here

[picklechips]

I'd prefer const/let over var but otherwise good stuff

[picklechips]

This just looks super awkward, what would you think about

let headers = {...options.headers}
if(options.authorized) 
    headers.Authorization = `Bearer ${await this.getAuthToken()}`;

Or even moving the logic into a method so we can do something like

const headers = formatHeaders(options)

or something since it's used in multiple places.

[scholvat]

@picklechips I've refactored it so its a bit cleaner, and less code repetition. Lmk what you think

[picklechips]

much nicer! 👍 Merge away