Fixes Handling of ClientID for Password Grant #1679
Conversation
|
Fixes #1678 |
There was a problem hiding this comment.
I think you're right actually about client_id belonging in a query param actually and this was wrong all along 🙀. This is a breaking change though and we'd have to add a flag for opting in to the new behavior together with a deprecation if we find that flag to be false.
Regarding client_secret: there is no way that I'm aware of for effectively using that in a client side JS app where you'd necessarily have to publish your client_secret and thus make it useless.
| @default null | ||
| @public | ||
| */ | ||
| clientSecret: null, |
There was a problem hiding this comment.
This was intentionally left out - as soon as you publish your client secret as part of your publicly accessible Ember app it's no longer a secret.
There was a problem hiding this comment.
You are right, I got a little over zealous, I'll remove it.
|
@marcoow if you can provide guidance on how to feature flag something (I'm pretty new to the ember world) I'll gladly fix this. |
|
Sure, don't worry - happy to help. So what we'll have to do is:
The idea behind this is that people who update to the next major version get the deprecation and can opt-in to the new behavior and validate that it works with their OAuth 2.0 server. Once the next major version is released, they just remove the flag. At the same time, we don't have to release this as a breaking change (yet) though. |
|
@marcoow I'm not sure how to add deprecation notices but the rest should be in properly now. |
|
Here's an example for a deprecation: https://github.com/simplabs/ember-simple-auth/blob/76e0eacf94f1d1d4d1194b3ef93ceae0bc5e000e/addon/authorizers/base.js#L28 |
|
|
||
| if (data['grant_type'] === 'password' && this.get('refreshAccessTokens')) { | ||
| data['offline_token'] = true; | ||
| } |
There was a problem hiding this comment.
I can open a second pull request if you like. It's actually required if you want access tokens to be refreshed by an Oauth2 server that's been implemented to spec. Generally you have to pass the offline_token as well as have the server allow for tokens to be refreshed.
There was a problem hiding this comment.
Hm, I can't find any reference to "offline_token" in https://tools.ietf.org/html/rfc6749… Anyway, I'd put this in a separate PR for simplicity.
There was a problem hiding this comment.
I'll have to dig a bit more, but it looks it might not be a spec thing, but instead an opinionated option that many oauth2 services implement. The most common being offline_token in the body, but Auth0 implements it as a scope.
I think it's important to look at adding because many require it.
|
I'll look at how to add tests for this. |
|
Hm, I didn't read through all of that tbh but Cmd+F "offl" comes back with 0 matches…
… Am 24.09.2018 um 17:28 schrieb Erik Kristensen ***@***.***>:
@ekristen commented on this pull request.
In addon/authenticators/oauth2-password-grant.js <#1679 (comment)>:
> @@ -336,6 +346,17 @@ export default BaseAuthenticator.extend({
makeRequest(url, data, headers = {}) {
headers['Content-Type'] = 'application/x-www-form-urlencoded';
+ if (this.get('sendClientIdAsQueryParam')) {
+ const clientId = this.get('clientId');
+ if (!isEmpty(clientId)) {
+ data['client_id'] = this.get('clientId');
+ }
+ }
+
+ if (data['grant_type'] === 'password' && this.get('refreshAccessTokens')) {
+ data['offline_token'] = true;
+ }
https://tools.ietf.org/html/rfc6749#page-47 <https://tools.ietf.org/html/rfc6749#page-47>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#1679 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF5pj4Y-o8g7FCrD6W8h8LesgnrMJ3ks5uePofgaJpZM4W0Pyc>.
|
|
@ekristen: any update on the tests? Would be great to get this in. |
|
Test to check for |
|
@marcoow forgot to mention you previously ^ |
|
@marcoow following up |
| @@ -31,6 +32,18 @@ const keys = Object.keys || emberKeys; // Ember.keys deprecated in 1.13 | |||
| @public | |||
| */ | |||
| export default BaseAuthenticator.extend({ | |||
| init() { | |||
| this._super(...arguments); | |||
| deprecate(`Ember Simple Auth: Client ID as Authorization Header is deprecated in favour of Client ID as Query String Parameter.`, | |||
There was a problem hiding this comment.
This should not actually be in the init as using the authenticator as such is totally fine - just leaving sendClientIdAsQueryParam as false is deprecated.
There was a problem hiding this comment.
Done. Moved the authenticate, triggers only if set to false.
|
Hopefully it's ok now? Tests pass for me locally. |
…r, not as a header
client_idis passed on Password Grant for OAuth2.client_secretand passing it as well for Password Grant if set.References on Password Grants:
Fixes #1678