Add default behavior for client_credentials grant type. #141

erickoledadevrel · 2018-08-31T14:31:10Z

I added support for arbitrary grant types primarily to support the client_credentials grant type, but I didn't special-case it at the time. In practice developers expect that setting the client ID and secret will just work when using that grant type, but the values were being ignored.

Given that mostly all providers use these values in a predictable way I decided to added a default behavior that does what the developer expects, set an Authorazation: Basic ... header. Developers not wishing to use this default behavior can avoid it by not setting the client ID and secret or by manually adding an Authorization header of their own.

grant

Looks OK. In some places, the logic could be simplified.

grant · 2018-09-10T16:19:08Z

src/Service.js

- * this will be "client_credentials", in which case make sure to also specify an
- * Authorization header if required by your OAuth provider.
+ * this will be "client_credentials", and a client ID and secret are set an
+ * "Authorization: Baseic ..." header will be added using thos values.


typo: thos

grant · 2018-09-10T16:20:55Z

src/Service.js

  };
  payload = extend_(payload, this.params_);
+
+  // For the client_credentials grant type, add a basic authorization header


I suggest making this comment more readable:

// For the client_credentials grant type, add a basic authorization header: // - If the client ID and client secret are set // - No authorization header has been set yet

grant · 2018-09-10T16:24:24Z

src/Utilities.js

+ * @return {Object} the value under that key, or undefined otherwise
+ */
+function getValueCaseInsensitive_(obj, key) {
+  if (obj == null || typeof obj !== 'object' ||


=== instead of == always

Simply boolean logic.

if typeof obj !== 'object', then obj === null will always be true.

grant · 2018-09-10T16:25:50Z

src/Utilities.js

+function getValueCaseInsensitive_(obj, key) {
+  if (obj == null || typeof obj !== 'object' ||
+      key == null || !key.toString) {
+    return undefined;


Undefined is returned by default in JavaScript.
Why don't you just return;?

Sample:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/return#Description

grant · 2018-09-10T16:26:34Z

src/Utilities.js

+  if (key in obj) {
+    return obj[key];
+  }
+  return Object.keys(obj).reduce(function(result, k) {


Can you add a comment to what this semi-complicated block is doing?

grant · 2018-09-10T16:30:48Z

src/Utilities.js

+      key == null || !key.toString) {
+    return undefined;
+  }
+  if (key in obj) {


This is a shortcut for some cases.
It's not immediately obvious why we can return here, can you add a comment?

grant · 2018-09-10T16:31:48Z

src/Utilities.js

+    if (k.toLowerCase() === key.toLowerCase()) {
+      return obj[k];
+    }
+  }, undefined);


It would be great if we could remove this , undefined) since it requires more advanced js (reduce) knowledge.

grant · 2018-09-10T16:34:07Z

src/Utilities.js

+
+/* exported getValueCaseInsensitive_ */
+/**
+ * Gets the value stored in the object under the given key, in a


getValueCaseInsensitive_ seems like an obscure method.

Why don't we make a util method that converts an object to a lowercased-keyed object and then get that value?

erickoledadevrel

Changes made, thanks for the review!

erickoledadevrel · 2018-09-10T17:10:11Z

src/Service.js

- * this will be "client_credentials", in which case make sure to also specify an
- * Authorization header if required by your OAuth provider.
+ * this will be "client_credentials", and a client ID and secret are set an
+ * "Authorization: Baseic ..." header will be added using thos values.


erickoledadevrel · 2018-09-10T17:11:02Z

src/Service.js

  };
  payload = extend_(payload, this.params_);
+
+  // For the client_credentials grant type, add a basic authorization header


erickoledadevrel · 2018-09-11T15:20:08Z

src/Utilities.js

+
+/* exported getValueCaseInsensitive_ */
+/**
+ * Gets the value stored in the object under the given key, in a


erickoledadevrel · 2018-09-11T15:20:19Z

src/Utilities.js

+ * @return {Object} the value under that key, or undefined otherwise
+ */
+function getValueCaseInsensitive_(obj, key) {
+  if (obj == null || typeof obj !== 'object' ||


erickoledadevrel · 2018-09-11T15:20:29Z

src/Utilities.js

+function getValueCaseInsensitive_(obj, key) {
+  if (obj == null || typeof obj !== 'object' ||
+      key == null || !key.toString) {
+    return undefined;


erickoledadevrel · 2018-09-11T15:20:43Z

src/Utilities.js

+      key == null || !key.toString) {
+    return undefined;
+  }
+  if (key in obj) {


erickoledadevrel · 2018-09-11T15:21:48Z

src/Utilities.js

+  if (key in obj) {
+    return obj[key];
+  }
+  return Object.keys(obj).reduce(function(result, k) {


erickoledadevrel · 2018-09-11T15:21:57Z

src/Utilities.js

+    if (k.toLowerCase() === key.toLowerCase()) {
+      return obj[k];
+    }
+  }, undefined);


grant

Looks OK after applying the fixes.

grant · 2018-09-11T15:32:52Z

src/Utilities.js

 }

-/* exported getValueCaseInsensitive_ */
+/* exported witLowerCaseKeys_ */


Rename witLowerCaseKeys_ to
toLowerCaseKeys_ or copyWithLowerCaseKeys_.

If you read this function aloud, it is not clear what the function does. By using "to lowercase keys", it's a bit clearer that like JS's toLowerCase, this function creates a copy of the parameter with lowercase keys.

Done, selected toLowerCaseKeys_

grant · 2018-09-11T15:51:08Z

test/test.js

+    };
+    var lowerCaseData = witLowerCaseKeys_(data);
+
+    it('should contain lower-case keys', function() {


Note, a stronger test would compare the exact input/output of values for the function rather than the presence and lack of presence of certain keys.

i.e. Combine these 2 tests. 'a' and 'A' could be one test like:

assert.deepEqual(lowerCaseData, { 'a': true, 'b': true, 'cc': true, 'd2': true, 'e!@#': true, });

This will assert everything else not in the object isUndefined too. You could also then inline lowerCaseData.

Good idea, done!

Add default behavior for client_credentials grant type.

e5a7db6

erickoledadevrel requested a review from grant August 31, 2018 14:31

Update the Domo samples to use the default client_credentials behavior.

daa1258

grant reviewed Sep 10, 2018

View reviewed changes

Switch to witLowerCaseKeys_ method, and other small fixes.

313fb23

erickoledadevrel commented Sep 11, 2018

View reviewed changes

grant approved these changes Sep 11, 2018

View reviewed changes

Change to toLowerCaseKeys_(), combine tests.

d8a5f45

erickoledadevrel merged commit bcf34f8 into master Sep 11, 2018

erickoledadevrel deleted the client_credentials branch September 11, 2018 16:26

Add default behavior for client_credentials grant type. #141

Add default behavior for client_credentials grant type. #141

Uh oh!

Conversation

erickoledadevrel commented Aug 31, 2018

Uh oh!

grant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erickoledadevrel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants