Skip to content

Commit

Permalink
Move token sending to authorization header instead of body parameter (#…
Browse files Browse the repository at this point in the history 
…1337)

* Move token sending to authorization header instead of body parameter. Fixes #1132.
* Enable another level of overriding Authorization header via the apiCall() method options parameter.
  • Loading branch information
@filmaj
filmaj authored Sep 17, 2021
1 parent 66066ee commit 957c957
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 32 deletions.
71 changes: 42 additions & 29 deletions packages/web-api/src/WebClient.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@ describe('WebClient', function () {
assert.instanceOf(client, WebClient);
assert.equal(client.token, token);
assert.equal(client.slackApiUrl, 'https://slack.com/api/')
assert.equal(client.axios.defaults.headers.Authorization, `Bearer ${token}`);
});

it('should build a client without a token', function () {
const client = new WebClient();
assert.instanceOf(client, WebClient);
assert.notExists(client.axios.defaults.headers.Authorization);
});
it('should not modify global defaults in axios', function () {
// https://github.com/slackapi/node-slack-sdk/issues/1037
Expand Down Expand Up @@ -127,14 +129,14 @@ describe('WebClient', function () {
setLevel: sinon.spy(),
setName: sinon.spy(),
};
const client = new WebClient(undefined, {
timeout: timeoutOverride,

const client = new WebClient(undefined, {
timeout: timeoutOverride,
retryConfig: { retries: 0 },
logLevel: LogLevel.WARN,
logger
});
logLevel: LogLevel.WARN,
logger
});

client.apiCall('users.list')
.then(_ => {
done(new Error("expected timeout to throw error"));
Expand All @@ -151,7 +153,7 @@ describe('WebClient', function () {
} catch (err) {
done(err);
}
});
});
});
});

Expand Down Expand Up @@ -363,7 +365,7 @@ describe('WebClient', function () {
describe('when an API call fails', function () {
it('should return a Promise which rejects on error', function (done) {
const client = new WebClient(undefined, { retryConfig: { retries: 0 } });

this.scope = nock('https://slack.com')
.post(/api/)
.reply(500);
Expand Down Expand Up @@ -425,7 +427,7 @@ describe('WebClient', function () {
it('should properly serialize simple API arguments', function () {
const scope = nock('https://slack.com')
// NOTE: this could create false negatives if the serialization order changes (it shouldn't matter)
.post(/api/, 'token=xoxb-faketoken&team_id=T12345678&foo=stringval&bar=42&baz=false')
.post(/api/, 'team_id=T12345678&foo=stringval&bar=42&baz=false')
.reply(200, { ok: true });
return this.client.apiCall('method', { foo: 'stringval', bar: 42, baz: false, team_id: 'T12345678' })
.then(() => {
Expand All @@ -436,7 +438,7 @@ describe('WebClient', function () {
it('should properly serialize complex API arguments', function () {
const scope = nock('https://slack.com')
// NOTE: this could create false negatives if the serialization order changes (it shouldn't matter)
.post(/api/, 'token=xoxb-faketoken&arraything=%5B%7B%22foo%22%3A%22stringval%22%2C%22bar%22%3A42%2C%22baz%22%3Afalse%2C%22zup%22%3A%5B%22one%22%2C%22two%22%2C%22three%22%5D%7D%5D&objectthing=%7B%22foo%22%3A7%2C%22hum%22%3Afalse%7D')
.post(/api/, 'arraything=%5B%7B%22foo%22%3A%22stringval%22%2C%22bar%22%3A42%2C%22baz%22%3Afalse%2C%22zup%22%3A%5B%22one%22%2C%22two%22%2C%22three%22%5D%7D%5D&objectthing=%7B%22foo%22%3A7%2C%22hum%22%3Afalse%7D')
.reply(200, { ok: true });
return this.client.apiCall('method', {
// TODO: include things like quotes and emojis
Expand All @@ -458,7 +460,7 @@ describe('WebClient', function () {

it('should remove undefined or null values from simple API arguments', function () {
const scope = nock('https://slack.com')
.post(/api/, 'token=xoxb-faketoken&something=else')
.post(/api/, 'something=else')
.reply(200, { ok: true });
return this.client.apiCall('method', {
something_undefined: undefined,
Expand Down Expand Up @@ -521,9 +523,6 @@ describe('WebClient', function () {
const file = parts.files[0];
// the filename is picked up from the the ReadableStream since it originates from fs
assert.include(file, { fieldname: 'someBinaryField', filename: 'train.jpg' });

assert.lengthOf(parts.fields, 1);
assert.deepInclude(parts.fields, { fieldname: 'token', value: token });
});
});

Expand Down Expand Up @@ -553,8 +552,7 @@ describe('WebClient', function () {
someUndefinedField: undefined,
})
.then((parts) => {
// the only field is the one related to the token
assert.lengthOf(parts.fields, 1);
assert.lengthOf(parts.fields, 0);
})
});
});
Expand Down Expand Up @@ -670,6 +668,21 @@ describe('WebClient', function () {
scope.done();
});
});
it('should override Authorization header if passed as an option to apiCall()', function () {
const client = new WebClient(token, { headers: { 'X-XYZ': 'value' } });
const scope = nock('https://slack.com', {
reqheaders: {
'X-XYZ': 'value',
'Authorization': 'Bearer xoxp-superfake'
}
})
.post(/api/)
.reply(200, { ok: true });
const r = client.apiCall('method', { token: 'xoxp-superfake' });
return r.then((result) => {
scope.done();
});
});
});

describe('named method aliases (facets)', function () {
Expand All @@ -680,10 +693,10 @@ describe('WebClient', function () {
// This test doesn't exhaustively check all the method aliases, it just tries a couple.
// This should be enough since all methods are mounted in the exact same way.
const scope = nock('https://slack.com')
.post('/api/chat.postMessage', 'token=xoxb-faketoken&foo=stringval')
.post('/api/chat.postMessage', 'foo=stringval')
.reply(200, { ok: true })
// Trying this method because its mounted one layer "deeper"
.post('/api/team.profile.get', 'token=xoxb-faketoken')
.post('/api/team.profile.get')
.reply(200, { ok: true });
return Promise.all([this.client.chat.postMessage({ foo: 'stringval' }), this.client.team.profile.get()])
.then(() => {
Expand Down Expand Up @@ -1160,39 +1173,39 @@ describe('WebClient', function () {
verify(
client.admin.inviteRequests.approve({ team_id: 'T123', invite_request_id: 'I123' }),
'admin.inviteRequests.approve',
'token=xoxb-faketoken&team_id=T123&invite_request_id=I123',
'team_id=T123&invite_request_id=I123',
done,
);
});
it('can call admin.inviteRequests.deny', function (done) {
verify(
client.admin.inviteRequests.deny({ team_id: 'T123', invite_request_id: 'I123' }),
'admin.inviteRequests.deny',
'token=xoxb-faketoken&team_id=T123&invite_request_id=I123',
'team_id=T123&invite_request_id=I123',
done
);
});
it('can call admin.inviteRequests.list', function (done) {
verify(
client.admin.inviteRequests.list({ team_id: 'T123', limit: 10, cursor: 'position' }),
'admin.inviteRequests.list',
'token=xoxb-faketoken&team_id=T123&limit=10&cursor=position',
'team_id=T123&limit=10&cursor=position',
done
);
});
it('can call admin.inviteRequests.approved.list', function (done) {
verify(
client.admin.inviteRequests.approved.list({ team_id: 'T123', limit: 10, cursor: 'position' }),
'admin.inviteRequests.approved.list',
'token=xoxb-faketoken&team_id=T123&limit=10&cursor=position',
'team_id=T123&limit=10&cursor=position',
done
);
});
it('can call admin.inviteRequests.denied.list', function (done) {
verify(
client.admin.inviteRequests.denied.list({ team_id: 'T123', limit: 10, cursor: 'position' }),
'admin.inviteRequests.denied.list',
'token=xoxb-faketoken&team_id=T123&limit=10&cursor=position',
'team_id=T123&limit=10&cursor=position',
done
);
});
Expand Down Expand Up @@ -1224,7 +1237,7 @@ describe('WebClient', function () {
verify(
client.admin.usergroups.addChannels({ team_id: 'T123', usergroup_id: 'S123', channel_ids: 'C123,C234' }),
'admin.usergroups.addChannels',
'token=xoxb-faketoken&team_id=T123&usergroup_id=S123&channel_ids=C123%2CC234',
'team_id=T123&usergroup_id=S123&channel_ids=C123%2CC234',
done,
);
});
Expand All @@ -1233,23 +1246,23 @@ describe('WebClient', function () {
verify(
client.admin.usergroups.addChannels({ team_id: 'T123', usergroup_id: 'S123', channel_ids: ['C123','C234'] }),
'admin.usergroups.addChannels',
'token=xoxb-faketoken&team_id=T123&usergroup_id=S123&channel_ids=%5B%22C123%22%2C%22C234%22%5D', // URL encoded "['C123','C234']"
'team_id=T123&usergroup_id=S123&channel_ids=%5B%22C123%22%2C%22C234%22%5D', // URL encoded "['C123','C234']"
done,
);
});
it('can call admin.usergroups.listChannels', function (done) {
verify(
client.admin.usergroups.listChannels({ team_id: 'T123', include_num_members: true, usergroup_id: 'S123' }),
'admin.usergroups.listChannels',
'token=xoxb-faketoken&team_id=T123&include_num_members=true&usergroup_id=S123',
'team_id=T123&include_num_members=true&usergroup_id=S123',
done
);
});
it('can call admin.usergroups.removeChannels with a string "channel_ids"', function (done) {
verify(
client.admin.usergroups.removeChannels({ usergroup_id: 'S123', channel_ids: 'C123,C234' }),
'admin.usergroups.removeChannels',
'token=xoxb-faketoken&usergroup_id=S123&channel_ids=C123%2CC234',
'usergroup_id=S123&channel_ids=C123%2CC234',
done,
);
});
Expand All @@ -1258,7 +1271,7 @@ describe('WebClient', function () {
verify(
client.admin.usergroups.removeChannels({ usergroup_id: 'S123', channel_ids: ['C123','C234'] }),
'admin.usergroups.removeChannels',
'token=xoxb-faketoken&usergroup_id=S123&channel_ids=%5B%22C123%22%2C%22C234%22%5D', // URL encoded "['C123','C234']"
'usergroup_id=S123&channel_ids=%5B%22C123%22%2C%22C234%22%5D', // URL encoded "['C123','C234']"
done,
);
});
Expand Down
11 changes: 8 additions & 3 deletions packages/web-api/src/WebClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ export class WebClient extends Methods {
this.logger = getLogger(WebClient.loggerName, logLevel ?? LogLevel.INFO, logger);
}

// eslint-disable-next-line no-param-reassign
if (this.token && !headers.Authorization) headers.Authorization = `Bearer ${this.token}`;

this.axios = axios.create({
timeout,
baseURL: slackApiUrl,
Expand Down Expand Up @@ -210,7 +213,7 @@ export class WebClient extends Methods {
* @param method - the Web API method to call {@link https://api.slack.com/methods}
* @param options - options
*/
public async apiCall(method: string, options?: WebAPICallOptions): Promise<WebAPICallResult> {
public async apiCall(method: string, options: WebAPICallOptions = {}): Promise<WebAPICallResult> {
this.logger.debug(`apiCall('${method}') start`);

warnDeprecations(method, this.logger);
Expand All @@ -221,11 +224,13 @@ export class WebClient extends Methods {
throw new TypeError(`Expected an options argument but instead received a ${typeof options}`);
}

const headers: Record<string, string> = {};
if (options.token) headers.Authorization = `Bearer ${options.token}`;

const response = await this.makeRequest(method, {
token: this.token,
team_id: this.teamId,
...options,
});
}, headers);
const result = this.buildResult(response);

// log warnings in response metadata
Expand Down

0 comments on commit 957c957

Please sign in to comment.