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 9 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
75 changes: 70 additions & 5 deletions src/http-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,24 +385,89 @@ module.exports.MatrixHttpApi.prototype = {
if (!queryParams) {
queryParams = {};
}
if (!queryParams.access_token) {
queryParams.access_token = this.opts.accessToken;
if (this.authorizationHeaderSupported === undefined ||
this.authorizationHeaderSupported) {
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) {
if (queryParams.access_token) {
opts.headers.Authorization = "Bearer " + queryParams.access_token;
delete queryParams.access_token;
} else {
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) {

if (this.authorizationHeaderSupported === undefined) {
const defer = q.defer();
const returnPromise = defer.promise;
returnPromise.abort = requestPromise.abort;

requestPromise.then((resp) => {
self.authorizationHeaderSupported = true;
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 like that we now always try to set an Authorization header and then re-send the request with a query parameter if it fails and then fail the request if that fails. I don't like it because:

  • Clients may incorrectly assume that headers are being sent, and therefore happily log the URL which, under some circumstances, might contain the access_token in the query but we've lulled them into a false sense of security as we don't always do that.
  • The fallback to query params is arguably unnecessary. Synapse has supported headers for nearly a year now.
  • This promise chain logic is unwieldy at best, I have little confidence that it will work under all circumstances.

I would much prefer this to be a option in the constructor to this class, which will always set the header or always send query parameters, negating the need for complex fallback logic and removing the risk of people logging URLs expecting them to not contain authentication information.

Copy link
Member

Choose a reason for hiding this comment

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

Default to query params, naturally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First: Although Header-Support is implemented for a long time it was not allowed in terms of CORS. Is got implemented recently and there was no release since then.

Beside that: I understand the "false sense of security"-point. And I have no problem removing the promise-chain-stuff.
I can change it to be an constructor-set decision.

But especially in terms of CORS: Where should such check be implemented?

Copy link
Member

Choose a reason for hiding this comment

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

Leave it up to the client to decide if they want to operate in header mode or query param mode. We'll likely keep Riot on query param mode until enough time has passed to let your Synapse PR trickle through, then flip it over to header mode.

defer.resolve(resp);
}, (err) => {
if (err.errcode == 'M_MISSING_TOKEN' ||
err.toString().indexOf("Error: CORS request rejected") != -1) {
self.authorizationHeaderSupported = false;
queryParams.access_token = opts.headers.Authorization.substr(7);
delete opts.headers.Authorization;
const secondPromise = self.request(
callback, method, path, queryParams, data, opts,
);
returnPromise.abort = secondPromise.abort;
secondPromise.then((resp) => {
defer.resolve(resp);
}, (err) => {
if (err.errcode == 'M_UNKNOWN_TOKEN') {
self.event_emitter.emit("Session.logged_out");
}
defer.reject(err);
});
} else if (err.errcode == 'M_UNKNOWN_TOKEN') {
self.event_emitter.emit("Session.logged_out");
defer.reject(err);
} else {
defer.reject(err);
}
});

return returnPromise;
}

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