zapier convert: Add support for basic auth mapping #200

eliangcs · 2017-12-08T13:45:59Z

This PR handles the case where a WB app with basic authentication has an authentication mapping. In CLI, we don't have authentication mappings. Instead, we just let users freely define their authentication in a function. So when converting a WB app like this, we need to use custom auth type and mimic the mapping of the auth fields in the authentication function.

Example app to try out: 80444

Most of these tests failed because I added heading and trailing spaces to the trigger template. This commit fixes them by removing the heading and trailing spaces with .trim() method before making assertion in the test code. This is temporary because I think we can use a tool like Prettier to format the generated code automitcally, so we don't have to manually format it in the templates, which is such a pain.

If no auth, there should be no `authetication` section in the app definition nor `authencation.js`.

We were assuming basic auth is always username and password when generating test code. The right thing to do is to use `auth_fields`. This commit only addresses the issue for basic auth. We should really do this for all the auth types.

eliangcs · 2017-12-11T09:04:34Z

scaffold/convert/header.template.js

@@ -1,4 +1,7 @@
-<% if (before && !session && !oauth) { %>const maybeIncludeAuth = (request, z, bundle) => {
+<% if (customBasic) { %>


I use the term "custom basic" to indicate a basic authentication that does not use username and password as user-facing authentication fields.

eliangcs · 2017-12-11T09:07:51Z

src/tests/utils/convert.js

@@ -120,7 +120,7 @@ describe('convert render functions', () => {
      const wbDef = definitions.noAuth;
      return convert.renderIndex(wbDef)
        .then(string => {
-          string.should.containEql('authentication: {}');
+          string.should.not.containEql('authentication:');


Originally, we generated an empty {} when a WB app doesn't have auth. The right way is not to include an authentication section at all if the WB app doesn't have auth. Refer to AppSchema.

eliangcs · 2017-12-11T09:11:25Z

src/tests/utils/convert.js

@@ -202,14 +202,12 @@ describe('convert render functions', () => {

      return convert.getHeader(wbDef)
        .then(string => {
-          string.should.eql(`const maybeIncludeAuth = (request, z, bundle) => {
+          string.trim().should.eql(`const maybeIncludeAuth = (request, z, bundle) => {


.trim() is to remove leading and trailing spaces generated by the template. I leave the generated code unformatted because formatting the code in the templates is hard and makes the template unreadable. I think I'll make another PR that uses Prettier to format the generated code.

eliangcs · 2017-12-11T09:16:12Z

src/utils/convert.js

@@ -95,7 +95,7 @@ const getAuthType = (definition) => {
 };

 const hasAuth = (definition) => {
-  return getAuthType(definition) !== 'custom' || !_.isEmpty(definition.auth_fields);
+  return getAuthType(definition) !== 'custom' && !_.isEmpty(definition.auth_fields);


Fixes the logic determining if an app has auth.

eliangcs · 2017-12-11T09:17:52Z

src/utils/convert.js

      }`;
    break;
+  }


In the generated test code, we were assuming basic auth is always username and password, which is not true. We should really take advantage of auth_fields instead.

bcooksey · 2017-12-12T23:51:43Z

Will try to test this out a bit tomorrow. Off the cuff, does this handle a case where say an app only has an api_key for auth fields and is doing something like Authorization: Basic <api_key>:?

eliangcs · 2017-12-13T02:38:46Z

@bcooksey exactly that case!

bcooksey

Found a small bug. I'm not quite sure how to cover for that in tests, as you'd need to actually execute the generated code. If you have a quick win, go for it, otherwise I think the test coverage here is good enough.

bcooksey · 2017-12-13T16:01:33Z

scaffold/convert/header.template.js

+  };
+  const username = replaceVars(mapping.username, bundle);
+  const password = replaceVars(mapping.password, bundle);
+  const encoded = `${username}:${password}`.toString('base64');


toString() on a string won't encode in JS like you'd hope. Need to wrapper in a Buffer I believe.

> 'asdf:'.toString('base64') 'asdf:' > new Buffer('asdf:').toString('base64') 'YXNkZjo='

@bcooksey nice catch! Fixed in 7ac3941.

zapier convert: Add support for basic auth mapping

cdd046b

BrunoBernardino mentioned this pull request Dec 8, 2017

Have zapier convert take full advantage of zapier-platform-legacy-scripting-runner #180

Closed

11 tasks

eliangcs changed the title ~~zapier convert: Add support for basic auth mapping~~ (WIP) zapier convert: Add support for basic auth mapping Dec 8, 2017

eliangcs changed the title ~~(WIP) zapier convert: Add support for basic auth mapping~~ zapier convert: Add support for basic auth mapping Dec 11, 2017

eliangcs added 4 commits December 11, 2017 16:46

zapier convert: Handle no auth correctly

e6bd39a

If no auth, there should be no `authetication` section in the app definition nor `authencation.js`.

zapier convert: Add test app to test basic auth with auth mapping

e1fdd11

zapier convert: Fix no auth test

0a2b0f9

eliangcs added the ZAPIER STAFF label Dec 11, 2017

eliangcs commented Dec 11, 2017

View reviewed changes

eliangcs requested review from BrunoBernardino, bcooksey and xavdid and removed request for BrunoBernardino December 11, 2017 09:18

Merge branch 'master' into convert-basic-auth-mapping

88ed299

bcooksey suggested changes Dec 13, 2017

View reviewed changes

zapier convert: Fix base64 encoding for basic auth

7ac3941

bcooksey approved these changes Dec 15, 2017

View reviewed changes

eliangcs merged commit c5bb2ea into master Dec 18, 2017

eliangcs deleted the convert-basic-auth-mapping branch December 18, 2017 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zapier convert: Add support for basic auth mapping #200

zapier convert: Add support for basic auth mapping #200

eliangcs commented Dec 8, 2017 •

edited

Loading

eliangcs Dec 11, 2017

eliangcs Dec 11, 2017 •

edited

Loading

eliangcs Dec 11, 2017 •

edited

Loading

eliangcs Dec 11, 2017

eliangcs Dec 11, 2017

bcooksey commented Dec 12, 2017

eliangcs commented Dec 13, 2017

bcooksey left a comment

bcooksey Dec 13, 2017

eliangcs Dec 14, 2017

		@@ -1,4 +1,7 @@
		<% if (before && !session && !oauth) { %>const maybeIncludeAuth = (request, z, bundle) => {
		<% if (customBasic) { %>

zapier convert: Add support for basic auth mapping #200

zapier convert: Add support for basic auth mapping #200

Conversation

eliangcs commented Dec 8, 2017 • edited Loading

eliangcs Dec 11, 2017

Choose a reason for hiding this comment

eliangcs Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

eliangcs Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

eliangcs Dec 11, 2017

Choose a reason for hiding this comment

eliangcs Dec 11, 2017

Choose a reason for hiding this comment

bcooksey commented Dec 12, 2017

eliangcs commented Dec 13, 2017

bcooksey left a comment

Choose a reason for hiding this comment

bcooksey Dec 13, 2017

Choose a reason for hiding this comment

eliangcs Dec 14, 2017

Choose a reason for hiding this comment

eliangcs commented Dec 8, 2017 •

edited

Loading

eliangcs Dec 11, 2017 •

edited

Loading

eliangcs Dec 11, 2017 •

edited

Loading