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.

Sign up for GitHub

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

Use access-token in header #478

Merged
merged 11 commits into from
Jul 6, 2017
Merged

Use access-token in header #478

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e0a5ede
implement usage of Authorization-Header instead of query-param for ac…
krombel Jun 16, 2017
9b24e66
Merge branch 'develop' into access_token_header
krombel Jun 23, 2017
539abff
Merge remote-tracking branch 'upstream/develop' into access_token_header
krombel Jun 23, 2017
d36b872
Allow Authorization-Header in tests
krombel Jun 23, 2017
5da6423
Added failover if server does not recognize the auth header
krombel Jun 27, 2017
59160a5
Implement failover when server does not allow setting the Authorized-…
krombel Jul 1, 2017
c6d2d4c
readd failover if server does not handle access-token via header
krombel Jul 1, 2017
07868f7
Merge remote-tracking branch 'upstream/develop' into access_token_header
krombel Jul 4, 2017
dd0ff3e
intercept first authedRequest to determine if accessToken can be send…
krombel Jul 4, 2017
6e7f5fe
remove fallback to query-params and set Authorization-Header based on…
krombel Jul 5, 2017
dc66bbc
pass useAuthorizationHeader from constructor; add docs
krombel Jul 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"build": "babel -s -d lib src && rimraf dist && mkdir dist && browserify -d browser-index.js | exorcist dist/browser-matrix.js.map > dist/browser-matrix.js && uglifyjs -c -m -o dist/browser-matrix.min.js --source-map dist/browser-matrix.min.js.map --in-source-map dist/browser-matrix.js.map dist/browser-matrix.js",
"dist": "npm run build",
"watch": "watchify -d browser-index.js -o 'exorcist dist/browser-matrix.js.map > dist/browser-matrix.js' -v",
"lint": "eslint --max-warnings 112 src spec",
"lint": "eslint --max-warnings 110 src spec",
"prepublish": "npm run build && git rev-parse HEAD > git-revision.txt"
},
"repository": {
Expand Down
5 changes: 4 additions & 1 deletion spec/integ/matrix-client-methods.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ describe("MatrixClient", function() {
).check(function(req) {
expect(req.data).toEqual(buf);
expect(req.queryParams.filename).toEqual("hi.txt");
expect(req.queryParams.access_token).toEqual(accessToken);
if (!(req.queryParams.access_token == accessToken ||
req.headers["Authorization"] == "Bearer " + accessToken)) {
expect(true).toBe(false);
}
expect(req.headers["Content-Type"]).toEqual("text/plain");
expect(req.opts.json).toBeFalsy();
expect(req.opts.timeout).toBe(undefined);
Expand Down
37 changes: 32 additions & 5 deletions src/http-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,18 @@ module.exports.PREFIX_MEDIA_R0 = "/_matrix/media/r0";
* requests.
* @param {Number=} opts.localTimeoutMs The default maximum amount of time to wait
* before timing out the request. If not specified, there is no timeout.
* @param {bool=} opts.useAuthorizationHeader True to use Authorization header instead of
* query param to send the access token to the server. Defaults to false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS SDK users do not create a MatrixHttpApi, so they will be unable to set this option without gut-wrenching client._http. This is set in the rather poorly defined "base APIs" constructor which is called in the client constructor.

Please bubble this down from MatrixClient, copying this documentation to the opts of MatrixClient.

*/
module.exports.MatrixHttpApi = function MatrixHttpApi(event_emitter, opts) {
utils.checkObjectHasKeys(opts, ["baseUrl", "request", "prefix"]);
opts.onlyData = opts.onlyData || false;
this.event_emitter = event_emitter;
this.opts = opts;
this.useAuthorizationHeader = false;
if (opts.useAuthorizationHeader) {
this.useAuthorizationHeader = Boolean(opts.useAuthorizationHeader);
}
this.uploads = [];
};

Expand Down Expand Up @@ -385,24 +391,45 @@ module.exports.MatrixHttpApi.prototype = {
if (!queryParams) {
queryParams = {};
}
if (!queryParams.access_token) {
queryParams.access_token = this.opts.accessToken;
if (this.useAuthorizationHeader) {
if (isFinite(opts)) {
// opts used to be localTimeoutMs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source? I'm surprised this PR is adding in backwards compat support in addition to the Authorization header thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I am modifying "opts" I have to check if the parameter is an int or not. I do not know which method actually issued it, but I got errors because it was a number but I wanted to threat is as an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As reference: It got copied from here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to check if the parameter is an int or not

Why? The docs seem pretty clear that it shouldn't be an int. Either:

  • the docs are wrong. You got errors as a result, in which case I want to know what the error is and see if we can fix the function which is calling this with an int instead of an object.
  • the docs aren't wrong, meaning this is not required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like krombel@9e89e71#diff-d2cbeec2e0aad0b77c59efdedd123c51L286 implemented the API change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like https://github.com/matrix-org/matrix-js-sdk/blob/v0.7.13/src/sync.js#L186 and https://github.com/matrix-org/matrix-js-sdk/blob/v0.7.13/src/sync.js#L578 still think the last parameter is an int, meaning that yes, we need the backwards compatible fix in here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what else is still relying on this old behaviour, so please can you update the docs from:

@param {Object=} opts additional options

to:

@param {Object|Number=} opts additional options. If a number is specified, this is treated as `opts.localTimeoutMs`.

opts = {
localTimeoutMs: opts,
};
}
if (!opts) {
opts = {};
}
if (!opts.headers) {
opts.headers = {};
}
if (!opts.headers.Authorization) {
opts.headers.Authorization = "Bearer " + this.opts.accessToken;
}
if (queryParams.access_token) {
delete queryParams.access_token;
}
} else {
if (!queryParams.access_token) {
queryParams.access_token = this.opts.accessToken;
}
}

const request_promise = this.request(
const requestPromise = this.request(
callback, method, path, queryParams, data, opts,
);

const self = this;
request_promise.catch(function(err) {
requestPromise.catch(function(err) {
if (err.errcode == 'M_UNKNOWN_TOKEN') {
self.event_emitter.emit("Session.logged_out");
}
});

// return the original promise, otherwise tests break due to it having to
// go around the event loop one more time to process the result of the request
return request_promise;
return requestPromise;
},

/**
Expand Down