-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
Add support for authMethod: POST #173
Conversation
1f84c25
to
e2e8e5e
Compare
Looks good |
}; | ||
Logger.logAction(Logger.LOG_MICRO, 'Auth.requestToken().tokenRequestCallback', 'Sending; ' + authOptions.authUrl + '; Params: ' + JSON.stringify(authParams)); | ||
if(authOptions.authMethod && authOptions.authMethod.toLowerCase() === 'post') { | ||
Http.postUri(rest, authOptions.authUrl, authHeaders || {}, authParams, {}, authUrlRequestCallback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone specifies authParams
then these end up in the body and not as params - is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that too, but give all token Params are sent in the body,
this may in fact be ok. I will check what I implemented in the ruby lib
On Thu, 3 Dec 2015 at 16:18, Paddy Byers notifications@github.com wrote:
In common/lib/client/auth.js
#173 (comment):@@ -260,7 +259,13 @@ var Auth = (function() {
}
}
cb(null, body);
});
};
Logger.logAction(Logger.LOG_MICRO, 'Auth.requestToken().tokenRequestCallback', 'Sending; ' + authOptions.authUrl + '; Params: ' + JSON.stringify(authParams));
if(authOptions.authMethod && authOptions.authMethod.toLowerCase() === 'post') {
Http.postUri(rest, authOptions.authUrl, authHeaders || {}, authParams, {}, authUrlRequestCallback);
If someone specifies authParams then these end up in the body and not as
params - is that right?—
Reply to this email directly or view it on GitHub
https://github.com/ably/ably-js/pull/173/files#r46572684.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this behaviour is correct, see http://docs.ably.io/client-lib-development-guide/features/#RSA8c1b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec says to put them in the body
(RSA8c1b) When the authMethod is POST, the TokenParams and authParams are merged and sent form-encoded in the body of the POST request, and the authHeaders are sent as HTTP headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check what I implemented in the ruby lib
I think it's more a question of what's desirable. I can imagine that a URL is given, with params, and the endpoint isn't expecting those params to come in the body instead. Keeping them as params IMO is more likely to be what the client is expecting and can deal with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My argument is that the client may require params and/or headers to support its general processing/validation/routing of the URL
Ok, but for params they can specify this as part of the authUrl
, and for any headers, they can provide this using authHeaders
if you wanted to add query params to the URL you could simply change the URL
That's not what's implemented.
Why not? new Ably.Realtime({ authUrl: 'x?param1=x¶m2=y' })
on re-reading it, spec also says it should be "form-encoded
@SimonWoolf this is simple key value string pairs, so you don't need to worry about Javascript objects. In typed languages this is not possible, and if someone is stupid enough to add objects to authParams then they can waste their time trying to figure out why it's not encoded as they expected.
We decide on form-encoded
because it's the most widely supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW. I propose if we are to change the spec, then we should do that.
However, in the mean time this PR is in accordance with the spec so should be merged.
@paddybyers you OK with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you OK with that?
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless you want me to change it to form-encode rather than json-encode first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was only referring to the way params were being sent in the form body. Yes, the spec requires form encoded so please do that first and merge
@mattheworiordan does f3c6562 look right for form-encoding? |
f3c6562
to
abceb14
Compare
@SimonWoolf oddly this fails in Firefox, see internal CI Not sure if it's related, but so do |
browser xhrRequest implementation was adding its own `rnd` param to the url, so was ending up as `?type=json?rnd=xxxxx`
@mattheworiordan ah, sorry, I only tried with nodeunit. turns out was failing in firefox because the browser xhr request implementation adds its own (maybe that was what Paddy meant by "That's not what's implemented" when you suggested adding query params to the authUrl? not sure) 3a15613 is a quick&dirty workaround -- probably doesn't make sense to decide a final fix (eg parsing the authUrl to extract any params and feeding them back in in the right place?) till you and paddy have decided how authUrl should treat authParams. By the way -- I'm guessing the |
The |
Out of interest, why is it needed for requests to Ably - can't we just set |
I think the issue is that various browsers don't handle that correctly. |
http://stackoverflow.com/a/2068407 claims that |
It sounds like you've talked yourself into doing a PR :) |
Add support for authMethod: POST
@SimonWoolf I have merged this and not raised a separate issue for the cache busting technique. I believe jQuery still uses the querystring cache busting technique, so I am not sure we should necessarily drop it as they probably test far more widely than we do. Up to you if you want to raise a separate issue for this |
Fixes #172
Depends on https://github.com/ably/echoserver/pull/1