Add cookie auth and custom header feature #814

Brent1LT · 2021-09-29T00:56:27Z

Checklist

General Contributing

Have you read the Code of Conduct and signed the CLA?

Is This a Code Change?

Non-code related change (markdown/git settings etc)
SDK Code Change
Example/Test Code Change

Validation

Does npm test pass?
Does npm run build pass?
Does npm run lint pass?

codecov · 2021-09-29T00:57:57Z

Codecov Report

Merging #814 (5733e27) into main (b851e70) will increase coverage by 0.31%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #814      +/-   ##
==========================================
+ Coverage   65.20%   65.51%   +0.31%     
==========================================
  Files           7        7              
  Lines         773      780       +7     
==========================================
+ Hits          504      511       +7     
  Misses        269      269

Flag	Coverage Δ
integration	`53.58% <42.85%> (-0.10%)`	⬇️
unit	`63.97% <100.00%> (+0.32%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/constants.js	`100.00% <100.00%> (ø)`
src/dropbox.js	`96.10% <100.00%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b851e70...5733e27. Read the comment docs.

Brent1LT · 2021-09-29T00:58:00Z

test/unit/dropbox.js

+      chai.assert.equal(COOKIE, dbx.rpcRequest.getCall(0).args[2]);
+    });
+
+    it('errors if cookie auth is supplied an OAuth token', () => {


Not sure if this is actually the behavior we want for cookie auth but can remove if not.

Brent1LT · 2021-09-29T01:00:45Z

src/dropbox.js

@@ -206,5 +215,10 @@ export default class Dropbox {
    if (this.pathRoot) {
      options.headers['Dropbox-API-Path-Root'] = this.pathRoot;
    }
+    if (this.customHeaders) {
+      for (const header in this.customHeaders) {


linter is failing from this line but I think this is actually the behavior we more or less want, can alter the code to satisfy linter but I feel like it just adds extra code for no reason. Can also ignore lint this line if we want

Valid linter, we don't want to do this

rogebrd · 2021-09-29T17:05:21Z

src/dropbox.js

@@ -50,6 +51,8 @@ const b64 = typeof btoa === 'undefined'
 * should only be used for testing as scaffolding to avoid making network requests.
 * @arg {String} [options.domainDelimiter] - A custom delimiter to use when separating domain from
 * subdomain. This should only be used for testing as scaffolding.
+ * @arg {Object} [options.customHeaders] - An object designed to set custom headers to use


This should have some indication of what format this object should be in

This also needs to be reflected in the TS types file or TS clients won't be able to use this

rogebrd · 2021-09-29T17:05:50Z

src/dropbox.js

@@ -125,6 +129,11 @@ export default class Dropbox {
            break;
          case NO_AUTH:
            break;
+          case COOKIE:
+            if (this.auth.accessToken) {


This is fine, just break. We don't need to explicitely check this

rogebrd · 2021-09-29T17:06:33Z

src/dropbox.js

@@ -206,5 +215,10 @@ export default class Dropbox {
    if (this.pathRoot) {
      options.headers['Dropbox-API-Path-Root'] = this.pathRoot;
    }
+    if (this.customHeaders) {
+      for (const header in this.customHeaders) {


Valid linter, we don't want to do this

rogebrd · 2021-09-29T17:08:35Z

src/dropbox.js

@@ -50,6 +51,8 @@ const b64 = typeof btoa === 'undefined'
 * should only be used for testing as scaffolding to avoid making network requests.
 * @arg {String} [options.domainDelimiter] - A custom delimiter to use when separating domain from
 * subdomain. This should only be used for testing as scaffolding.
+ * @arg {Object} [options.customHeaders] - An object designed to set custom headers to use


This also needs to be reflected in the TS types file or TS clients won't be able to use this

rogebrd · 2021-09-30T16:27:42Z

src/dropbox.js

+    if (this.customHeaders) {
+      const headerKeys = Object.keys(this.customHeaders);
+      headerKeys.forEach((header) => {
+        if (this.customHeaders[header]) {


Why do we need to check if it exists? Don't we know it exists if we just got the keys for the headers?

Because I thought the linter wanted an 'if' statement but I just realized if I do it this way then the linter doesn't care.

rogebrd · 2021-09-30T16:29:23Z

generator/typescript/index.d.tstemplate

@@ -177,6 +181,8 @@ export interface DropboxOptions {
  domain?: string;
  // A custom delimiter to use when separating domain subdomain. This should only be used for testing as scaffolding.
  domainDelimiter?: string;
+  // An object (in the form property: string) designed to set custom headers to use during a request.
+  customHeaders?: customHeaders;


We can just use object, we don't need a custom type for this

rogebrd · 2021-09-30T16:30:44Z

generator/typescript/index.d.tstemplate

@@ -177,6 +181,8 @@ export interface DropboxOptions {
  domain?: string;
  // A custom delimiter to use when separating domain subdomain. This should only be used for testing as scaffolding.
  domainDelimiter?: string;
+  // An object (in the form property: string) designed to set custom headers to use during a request.


I think this can be more concise by saying something like an object of headers to their values

rogebrd · 2021-09-30T16:31:49Z

generator/typescript/index.d.tstemplate

@@ -177,6 +181,8 @@ export interface DropboxOptions {
  domain?: string;
  // A custom delimiter to use when separating domain subdomain. This should only be used for testing as scaffolding.
  domainDelimiter?: string;
+  // An object (in the form property: string) designed to set custom headers to use during a request.
+  customHeaders?: customHeaders;
 }

 export class DropboxResponseError<T> {


This file is the template, you need to re-generate the sdk to get this into the generated code. Make sure your submodules are at the same place as main is (just pull from main in each repo)

Brent1LT commented Sep 29, 2021

View reviewed changes

rogebrd reviewed Sep 29, 2021

View reviewed changes

Brent1LT force-pushed the cookie_auth branch from 48e9b05 to 5fad8c2 Compare September 29, 2021 23:40

rogebrd reviewed Sep 30, 2021

View reviewed changes

Brent1LT force-pushed the cookie_auth branch from 5fad8c2 to 77b9bf8 Compare September 30, 2021 17:26

rogebrd approved these changes Sep 30, 2021

View reviewed changes

Add cookie auth and custom header feature

5733e27

Brent1LT force-pushed the cookie_auth branch from 77b9bf8 to 5733e27 Compare October 1, 2021 17:32

Brent1LT merged commit 7e2772b into main Oct 1, 2021

Brent1LT deleted the cookie_auth branch October 1, 2021 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cookie auth and custom header feature #814

Add cookie auth and custom header feature #814

Brent1LT commented Sep 29, 2021

codecov bot commented Sep 29, 2021 •

edited

Loading

Brent1LT Sep 29, 2021

Brent1LT Sep 29, 2021

rogebrd Sep 29, 2021

rogebrd Sep 29, 2021

rogebrd Sep 29, 2021

rogebrd Sep 29, 2021

rogebrd Sep 29, 2021

rogebrd Sep 29, 2021

rogebrd Sep 30, 2021

Brent1LT Sep 30, 2021

rogebrd Sep 30, 2021

rogebrd Sep 30, 2021

rogebrd Sep 30, 2021

Add cookie auth and custom header feature #814

Add cookie auth and custom header feature #814

Conversation

Brent1LT commented Sep 29, 2021

Checklist

codecov bot commented Sep 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2021 •

edited

Loading