add bearer token on signout

[yuenmichelle1]

Related: zooniverse/panoptes#4134
https://github.com/zooniverse/how-to-zooniverse/issues/319

Edit: Add missing Bearer Authorization Header on Signout

[eatyourgreens]

LGTM. Thanks for this!

[camallen]

Would it be better to ensure the credentialed superagent singleton has the access token setup all the time as well vs injecting it on this action? Longer term that means less changes to the client if we need this behaviour on other actions.

Similar to how we do that here?

panoptes-javascript-client/lib/auth.js

Lines 51 to 52 in 87f1d17

	this._bearerToken = response.access_token;
	apiClient.headers.Authorization = 'Bearer ' + this._bearerToken;

That api client is the same under the hood pretty much, e.g.

panoptes-javascript-client/lib/api-client.js

Lines 4 to 8 in 87f1d17

	var apiClient = new JSONAPIClient(config.host + '/api', {
	'Content-Type': 'application/json',
	'Accept': 'application/vnd.api+json; version=1',
	}, {
	params: config.params,

That said if we don't need it then this change looks fine to me but i'd like a review from someone else to confirm it's acceptable longer term.

[eatyourgreens]

superagent.auth() supports bearer tokens now, so that’s an option.
https://ladjs.github.io/superagent/#authentication

The agents themselves aren’t exposed publicly by json-api-client so the headers would still have to be constructed manually before calling makeCredentialHTTPRequest.

Does the signOut method in oauth.js need to be updated too? This is used by https://classroom.zooniverse.org.

panoptes-javascript-client/lib/oauth.js

Lines 129 to 132 in 87f1d17

	var deleteHeaders = {
	...config.jsonHeaders,
	['X-CSRF-Token']: token
	};

[yuenmichelle1]

If we wanted to set auth on credentialed superagent then we could possibly do it here:
https://github.com/zooniverse/panoptes-javascript-client/blob/master/lib/json-api-client/make-http-request.js#L26-L32. I think? I havent checked.

One thing I'm unsure of is when grabbing a new token:
https://github.com/zooniverse/panoptes-javascript-client/blob/master/lib/auth.js#L34-L38 when a token is already set on superagent.auth request, will it set the old token or the new one.

[eatyourgreens]

One thing I'm unsure of is when grabbing a new token:
https://github.com/zooniverse/panoptes-javascript-client/blob/master/lib/auth.js#L34-L38 when a token is already set on superagent.auth request, will it set the old token or the new one.

Ages since I've looked at this code, so I'm not sure, but I think it refreshes the bearer token if the existing token is within five minutes of expiring. If you've cleared the stored token, then it should always fetch a new one from Panoptes.

[eatyourgreens]

That said if we don't need it then this change looks fine to me but i'd like a review from someone else to confirm it's acceptable longer term.

I already reviewed this, last week. Still looks good to me, so I'm happy for it to be merged as-is. oauth.signOut() might need the same changes to the request headers, so that this works for https://classroom.zooniverse.org and other domains that use the oauth client.

[yuenmichelle1]

I already reviewed this, last week. Still looks good to me, so I'm happy for it to be merged as-is. oauth.signOut() might need the same changes to the request headers, so that this works for https://classroom.zooniverse.org/ and other domains that use the oauth client.

Apologies @eatyourgreens , Mondays at Adler are tricky since meetings are stacked together. I wanted to test around with setting the header somewhere higher on chain. Will ping you again for another review if needed.

Otherwise, I'll go with original fix (and update oauth client with a similar fix) and get this merged. Thanks!

[eatyourgreens]

No worries, just wanted to make sure I'm not holding this up.

[yuenmichelle1]

Hey @eatyourgreens , do you mind re-reviewing? I ended up reverting the change on auth.js and did the set of token on deleteHeaders.
Mainly, I wanted to double check on oauth.js (not sure where that is used, ), if I'm grabbing the token correctly.

[eatyourgreens]

Mainly, I wanted to double check on oauth.js (not sure where that is used, ), if I'm grabbing the token correctly.

I'm not 100% sure how oauth.js stores the token either, but the changes look reasonable to me. https://classroom.zooniverse.org uses that client for Panoptes auth. It's also used by any project that's not hosted on a *.zooniverse.org domain eg. Scribes of the Cairo Genizah.

[eatyourgreens]

Similar comment here. This code is absolutely fine but if the token details are missing, then you aren't logged in anyway (I think.)