fix #1155 by renaming signalRTokenHeader to access_token #1343
Conversation
analogrelay
changed the title
| @@ -68,9 +68,9 @@ export class HttpConnection implements IConnection { | |||
| } | |||
| else { | |||
| let headers; | |||
| if (this.options.accessToken) { | |||
| if (this.options.accessTokenFactory) { | |||
There was a problem hiding this comment.
Curious: is accessTokenFactory allowed to return a null/empty token? Should a non-null token check be added?
There was a problem hiding this comment.
I guess it could, and I don't see why we couldn't "allow" it (and just not provide the header).
client-ts/signalr/src/Transports.ts
Outdated
| url += (url.indexOf("?") < 0 ? "?" : "&") + `signalRTokenHeader=${token}`; | ||
| if (this.accessTokenFactory) { | ||
| let token = this.accessTokenFactory(); | ||
| url += (url.indexOf("?") < 0 ? "?" : "&") + `access_token=${token}`; |
There was a problem hiding this comment.
Same question (also, should you URLencode the token query string parameter, just in case?)
|
Thanks for fixing that 👍 |
|
Base changed to |
|
FYI I addressed @PinpointTownes 's comment about the factory returning null by changing the logic a little. The constructors that accept the factories now ensure they are always non-null by using a "default" factory that returns null all the time if the factory itself is null. Then the logic just shifts to only writing the header/qs if the factory returns a truthy value (rather than testing if the factory is present). |
|
Yeah, until we get some automation running, the first comment for any typescript changes should be have you run the browser tests |
| options = options || {}; | ||
| options.accessTokenFactory = options.accessTokenFactory || (() => null); |
There was a problem hiding this comment.
Would it be easier to just do a null check on the accessTokenFactory before creating and checking the token. It gets rid of some of the logic
|
Looks mechanical |
analogrelay
force-pushed
the
anurse/1155-accesstoken-rename
branch
from
7ad49b5 to
22dc794
Compare
|
Rebased so Travis and AppVeyor could see it. Merging when I get some green. |
I actually started this on
release/2.1because to be honest, it's pretty simple and I feel comfortable taking it in preview 1 if @muratg is OK with that. It's low-hanging fruit, and it's "public" API surface (kinda) so front-loading it would be nice. Also, I was reviewing the OAuth spec and noticed that it calls out how to pass access tokens via the query string in the spec (https://tools.ietf.org/html/rfc6750#section-2.3), including the query string name to use,access_token(as @PinpointTownes referenced in the issue #1155).Browser functional tests have been updated and run (next up on my plate is to automate those ;)). JwtSample has also been updated and verified. The C# client doesn't ever use the query string, so the only product changes are in the Jwt sample (since we haven't decided what to "build in" for this yet) and the TS client.
Anyway, this PR is on
devright now. If @muratg signs off on pulling it up to preview 1, I'll retarget torelease/2.1