zapier convert: Generate different auth code based on scripting #185

eliangcs · 2017-11-15T13:47:09Z

Summary:

Move authentication code from index.js to its own separate file authentication.js
Generate different auth code based on scripting, including:
- OAuth: generate a getAccessToken() if pre_oauthv2_token or post_oauthv2_token method exists
- OAuth: generate a refreshAccessToken() if pre_oauthv2_refresh method exists
- Session: generate a getSessionKey() that invokes get_session_info method
- Generate a getConnectionLabel() if get_connection_label method exists

See more detail in the following comments.

eliangcs · 2017-11-16T07:46:31Z

src/tests/utils/convert.js

-const AuthTest = { operation: { perform: 'FAKE_PERFORM_FUNCTION' } };
-const getSessionKey = 'FAKE_GET_SESSION_KEY_FUNCTION';
-(${string});
-`);


Since now I'm using loadAuthModuleFromString to eval the generated auth code, we don't need AuthTest and getSessionKey in s2js.

eliangcs · 2017-11-16T07:47:41Z

src/tests/utils/convert.js

 const convert = require('../../utils/convert');
 const definitions = {
  basic: require('./definitions/basic.json'),
+  basicScripting: require('./definitions/basic-scripting.json'),


basic-scripting.json is same as basic.json except that it has a get_connection_info method in scripting.

eliangcs · 2017-11-16T07:48:25Z

src/tests/utils/convert.js

  apiHeader: require('./definitions/api-header.json'),
  apiQuery: require('./definitions/api-query.json'),
  session: require('./definitions/session.json'),
  oauth2: require('./definitions/oauth2.json'),
+  oauth2Scripting: require('./definitions/oauth2-scripting.json'),


oauth2-scripting.json is same as oauth2.json except that it defines pre_oauthv2_token and post_oauthv2_token method in scripting.

eliangcs · 2017-11-16T07:49:27Z

src/tests/utils/convert.js

  oauth2Refresh: require('./definitions/oauth2-refresh.json'),
+  oauth2RefreshScripting: require('./definitions/oauth2-refresh-scripting.json'),


oauth2-refresh-scripting.json is same as oauth2-refresh.json except that it defines pre_oauthv2_token, post_oauthv2_token and pre_oauthv2_refresh method in scripting.

eliangcs · 2017-11-16T07:54:31Z

src/tests/utils/convert.js

      const wbDef = definitions.basic;

-      convert.renderAuth(wbDef)
+      return convert.renderAuth(wbDef)


To make the code cleaner, instead of using done() callback, I changed the style of all the auth-related tests by simply returning a Promise.

eliangcs · 2017-11-16T07:56:43Z

src/tests/utils/convert.js

+            }
+          ]);
+          auth.connectionLabel.should.eql('{{username}}');
+          auth.test().should.eql('./triggers/test_auth');


This test trigger is mocked so that it returns the module path. See loadAuthModuleFromString().

eliangcs · 2017-11-16T08:01:23Z

src/utils/convert.js

@@ -495,15 +559,13 @@ const renderIndex = (legacyApp) => {
    AFTER_RESPONSES: getAfterResponses(legacyApp),
  };

-  return renderAuth(legacyApp)


Authentication has been moved to its own separate file authentication.js, so we no longer need to call renderAuth() when rendering index.js.

eliangcs · 2017-11-16T08:05:43Z

package.json

@@ -67,6 +68,7 @@
    "mocha": "3.4.2",
    "ngrok": "2.2.10",
    "node-watch": "0.5.4",
+    "require-from-string": "2.0.1",


This package is used to load the rendered auth code, i.e., authentication.js. Since authentication.js is a CommonJS module (with module.exports = ... ; at the end), we can't just use s2js() or eval() to evaluate the code. require-from-string allows us to do require() but from a string instead of a file.

eliangcs · 2017-11-16T08:16:09Z

src/utils/convert.js

-
-          if (cliType === 'trigger' && _.get(legacyApp, ['general', 'test_trigger_key']) === name) {
-            importLines.push(`const AuthTest = ${varName};`);
-          }


Test trigger is now imported in authentication.js, so we can remove it from index.js.

BrunoBernardino

This is amazing @eliangcs ! Awesome tests, refactoring for re-usability, and things are looking neat!

I've got a few suggestions and requests, but it's nothing really major.

BrunoBernardino · 2017-11-17T09:50:03Z

scaffold/convert/basic-auth.template.js

+<%= FIELDS %>
+  ],
+<% if (hasGetConnectionLabelScripting) { %>
+  connectionLabel: getConnectionLabel,


For consistency, I'd expect this to not have a trailing comma, since the else doesn't as well.

Fixed in 73e2c37.

BrunoBernardino · 2017-11-17T09:51:00Z

scaffold/convert/basic-auth.template.js

+const testTrigger = require('<%= TEST_TRIGGER_MODULE %>');
+<% if (hasGetConnectionLabelScripting) { %>
+const getConnectionLabel = (z, bundle) => {
+  const scripting = require('../scripting');


I believe this isn't running from a folder, so scripting.js should be at the same level, not one level above?

Nice catch! Fixed in ecb1a36.

BrunoBernardino · 2017-11-17T09:52:50Z

scaffold/convert/oauth2.template.js

+const testTrigger = require('<%= TEST_TRIGGER_MODULE %>');
+<% if (hasPreOAuthTokenScripting && hasPostOAuthTokenScripting) { %>
+const getAccessToken = (z, bundle) => {
+  const scripting = require('../scripting');


Same comment as above with the relative location of scripting.js.

BrunoBernardino · 2017-11-17T10:11:56Z

src/utils/convert.js

@@ -86,7 +87,7 @@ const getAuthType = (definition) => {
  return authTypeMap[definition.general.auth_type];
 };

-const renderField = (definition, key) => {
+const renderField = (definition, key, indent = 0) => {


BrunoBernardino · 2017-11-17T10:13:00Z

src/utils/convert.js

+};
+
+const renderFields = (fields, indent = 0) => {
+  const result = [];


I'd prefer to make this plural results just to more clearly indicate it's a list of things, not a single thing/object.

Fixed in 990f1c9.

BrunoBernardino · 2017-11-17T10:26:38Z

scaffold/convert/oauth2.template.js

+
+   if (hasGetConnectionLabelScripting) { %>
+const getConnectionLabel = (z, bundle) => {
+  const scripting = require('../scripting');


Same comment as above with the relative location of scripting.js.

BrunoBernardino · 2017-11-17T10:26:42Z

scaffold/convert/session.template.js

+const testTrigger = require('<%= TEST_TRIGGER_MODULE %>');
+
+const getSessionKey = (z, bundle) => {
+  const scripting = require('../scripting');


Same comment as above with the relative location of scripting.js.

BrunoBernardino · 2017-11-17T10:26:51Z

scaffold/convert/session.template.js

+};
+<% if (hasGetConnectionLabelScripting) { %>
+const getConnectionLabel = (z, bundle) => {
+  const scripting = require('../scripting');


Same comment as above with the relative location of scripting.js.

BrunoBernardino · 2017-11-17T10:27:41Z

scaffold/convert/oauth2.template.js

  },
+<% if (hasGetConnectionLabelScripting) { %>
+  connectionLabel: getConnectionLabel,


Same comment as above with the trailing comma removal for consistency.

BrunoBernardino · 2017-11-17T10:27:47Z

scaffold/convert/session.template.js

  ],
  sessionConfig: {
    perform: getSessionKey
  },
+<% if (hasGetConnectionLabelScripting) { %>
+  connectionLabel: getConnectionLabel,


Same comment as above with the trailing comma removal for consistency.

BrunoBernardino · 2017-11-17T15:10:38Z

src/tests/utils/definitions/no-auth.json

+    "auth_data": {},
+    "auth_label": "",
+    "auth_mapping": {},
+    "auth_type": "Unknown Auth",


I think we should try to render a custom auth for Unknown Auth. No Auth would be No Auth Needed.

I made an app https://zapier.com/developer/builder/app/79939/development and specified "No Auth" for the authentication. The dumped result (https://zapier.com/api/developer/v1/apps/79939/dump) is "auth_type": "Unknown Auth". Am I missing something here?

Well, in that case let it be, then!

BrunoBernardino · 2017-11-17T15:12:21Z

src/utils/convert.js

@@ -87,6 +87,10 @@ const getAuthType = (definition) => {
  return authTypeMap[definition.general.auth_type];
 };

+const hasAuth = (definition) => {


Could this be moved to the getAuthMetaData as a returned property instead of its own function?

They are separate functions because in some situations we only need to quickly check if the app needs authentication or not. I don't want to do all the unnecessary things in getAuthMetaData for those situations. Makes sense?

BrunoBernardino

Just found one place missing the replacement from ../scripting to ./scripting and then I'll pull this down and test it around.

BrunoBernardino · 2017-11-27T14:44:46Z

scaffold/convert/oauth2.template.js

+};
+<% } else if (hasPreOAuthTokenScripting && !hasPostOAuthTokenScripting) { %>
+const getAccessToken = (z, bundle) => {
+  const scripting = require('../scripting');


I believe this one still needs to be changed?

eliangcs · 2017-11-28T03:21:50Z

@BrunoBernardino Good catch! Fixed in fecf3c0.

BrunoBernardino · 2017-11-28T08:02:06Z

Alright! I added a new app to the convert test (let me know if you want any help or pointers on doing that for new convert work that gets added), and all looks great:

Individually converting the app and running npm install and zapier test:

Running test suite.


  Creates - Recipe
    ✓ should create an object (1442ms)

  Searches - Recipe
    ✓ should get an object (391ms)

  Triggers - Recipe
    ✓ should get an array (307ms)


  3 passing (2s)

And running npm run test-convert:

simple-basic-auth converted successfully
simple-oauth converted successfully
trigger-session-auth converted successfully
apps converted successfully

zapier convert: Generate different auth code based on scripting

7b50adc

eliangcs added the ZAPIER STAFF label Nov 15, 2017

eliangcs changed the title ~~zapier convert: Generate different auth code based on scripting~~ [WIP] zapier convert: Generate different auth code based on scripting Nov 15, 2017

eliangcs added 2 commits November 16, 2017 11:57

zapier convert: Fix tests for auth codegen

fc4dc25

zapier convert: Add tests for auth w/scripting

33ac7ce

eliangcs commented Nov 16, 2017

View reviewed changes

eliangcs requested a review from BrunoBernardino November 16, 2017 08:14

eliangcs commented Nov 16, 2017

View reviewed changes

eliangcs changed the title ~~[WIP] zapier convert: Generate different auth code based on scripting~~ zapier convert: Generate different auth code based on scripting Nov 16, 2017

BrunoBernardino suggested changes Nov 17, 2017

View reviewed changes

eliangcs added 6 commits November 17, 2017 21:49

zapier convert: Rename basic-auth.template.js to simple-auth.template.js

0cb65c9

zapier convert: Fix scripting import path in authentication.js

ecb1a36

zapier convert: Remove trailing commas for consistency

73e2c37

zapier convert: Use plural naming to indicate list

990f1c9

zapier convert: Fix typo

9212231

zapier convert: Don't generate authentication.js if no auth

f837e3a

BrunoBernardino reviewed Nov 17, 2017

View reviewed changes

BrunoBernardino suggested changes Nov 27, 2017

View reviewed changes

zapier convert: Fix scripting import path in oauth2 template

fecf3c0

BrunoBernardino approved these changes Nov 28, 2017

View reviewed changes

Adding simple OAuth convert test app (trigger + create + search)

7b4f7e6

eliangcs merged commit 303cb06 into master Nov 28, 2017

eliangcs deleted the convert-auth-scripting branch November 28, 2017 09:42

BrunoBernardino mentioned this pull request Dec 7, 2017

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

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zapier convert: Generate different auth code based on scripting #185

zapier convert: Generate different auth code based on scripting #185

eliangcs commented Nov 15, 2017 •

edited

Loading

eliangcs Nov 16, 2017

eliangcs Nov 16, 2017 •

edited

Loading

eliangcs Nov 16, 2017 •

edited

Loading

eliangcs Nov 16, 2017

eliangcs Nov 16, 2017 •

edited

Loading

eliangcs Nov 16, 2017

eliangcs Nov 16, 2017

eliangcs Nov 16, 2017 •

edited

Loading

eliangcs Nov 16, 2017

BrunoBernardino left a comment

BrunoBernardino Nov 17, 2017

eliangcs Nov 17, 2017

BrunoBernardino Nov 17, 2017

eliangcs Nov 17, 2017

BrunoBernardino Nov 17, 2017

BrunoBernardino Nov 17, 2017

BrunoBernardino Nov 17, 2017

eliangcs Nov 17, 2017

BrunoBernardino Nov 17, 2017

BrunoBernardino Nov 17, 2017

BrunoBernardino Nov 17, 2017

BrunoBernardino Nov 17, 2017

BrunoBernardino Nov 17, 2017

BrunoBernardino Nov 17, 2017 •

edited

Loading

eliangcs Nov 27, 2017

BrunoBernardino Nov 27, 2017

BrunoBernardino Nov 17, 2017 •

edited

Loading

eliangcs Nov 27, 2017

BrunoBernardino Nov 27, 2017

BrunoBernardino left a comment •

edited

Loading

BrunoBernardino Nov 27, 2017

eliangcs commented Nov 28, 2017

BrunoBernardino commented Nov 28, 2017 •

edited

Loading

		oauth2Refresh: require('./definitions/oauth2-refresh.json'),
		oauth2RefreshScripting: require('./definitions/oauth2-refresh-scripting.json'),

zapier convert: Generate different auth code based on scripting #185

zapier convert: Generate different auth code based on scripting #185

Conversation

eliangcs commented Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

eliangcs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

eliangcs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliangcs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliangcs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoBernardino left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoBernardino Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoBernardino Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoBernardino left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliangcs commented Nov 28, 2017

BrunoBernardino commented Nov 28, 2017 • edited Loading

eliangcs commented Nov 15, 2017 •

edited

Loading

eliangcs Nov 16, 2017 •

edited

Loading

eliangcs Nov 16, 2017 •

edited

Loading

eliangcs Nov 16, 2017 •

edited

Loading

eliangcs Nov 16, 2017 •

edited

Loading

BrunoBernardino Nov 17, 2017 •

edited

Loading

BrunoBernardino Nov 17, 2017 •

edited

Loading

BrunoBernardino left a comment •

edited

Loading

BrunoBernardino commented Nov 28, 2017 •

edited

Loading