Use access-token in header #478
Conversation
krombel
force-pushed
the
access_token_header
branch
from
80fd8d4
to
c38572e
Compare
krombel
force-pushed
the
access_token_header
branch
from
c38572e
to
59160a5
Compare
krombel
force-pushed
the
access_token_header
branch
from
6bb6f99
to
c6d2d4c
Compare
krombel
force-pushed
the
access_token_header
branch
2 times, most recently
from
dd82be5
to
2aececb
Compare
… by header (clearer structure)
krombel
force-pushed
the
access_token_header
branch
from
2aececb
to
dd0ff3e
Compare
src/http-api.js
Outdated
| returnPromise.abort = requestPromise.abort; | ||
|
|
||
| requestPromise.then((resp) => { | ||
| self.authorizationHeaderSupported = true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Default to query params, naturally.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
… construcor-option
| queryParams.access_token = this.opts.accessToken; | ||
| if (this.useAuthorizationHeader) { | ||
| if (isFinite(opts)) { | ||
| // opts used to be localTimeoutMs |
There was a problem hiding this comment.
Source? I'm surprised this PR is adding in backwards compat support in addition to the Authorization header thing.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As reference: It got copied from here
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It looks like krombel@9e89e71#diff-d2cbeec2e0aad0b77c59efdedd123c51L286 implemented the API change.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`.
src/http-api.js
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
|
Otherwise LGTM, thanks for fixing some lint errors along the way :) |
|
Wonderful, thank you. LGTM |
Instead of passing the
access_tokenas query-param set theAuthorizationheaderSigned-off-by: Matthias Kesler krombel@krombel.de