Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

zapier convert: Add support for basic auth mapping #200

Merged
merged 8 commits into from
Dec 18, 2017
Merged

zapier convert: Add support for basic auth mapping #200

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cdd046b
zapier convert: Add support for basic auth mapping
eliangcs Dec 8, 2017
e48ae43
zapier convert: Fix auth tests
eliangcs Dec 11, 2017
e6bd39a
zapier convert: Handle no auth correctly
eliangcs Dec 11, 2017
fba8369
zapier convert: Generated test code should use auth_fields
eliangcs Dec 11, 2017
e1fdd11
zapier convert: Add test app to test basic auth with auth mapping
eliangcs Dec 11, 2017
0a2b0f9
zapier convert: Fix no auth test
eliangcs Dec 11, 2017
88ed299
Merge branch 'master' into convert-basic-auth-mapping
eliangcs Dec 13, 2017
7ac3941
zapier convert: Fix base64 encoding for basic auth
eliangcs Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion scaffold/convert/header.template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
<% if (before && !session && !oauth) { %>const maybeIncludeAuth = (request, z, bundle) => {
<% if (customBasic) { %>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { replaceVars } = require('./utils');
<% } %>
<% if (before && !session && !oauth && !customBasic) { %>const maybeIncludeAuth = (request, z, bundle) => {
<%
Object.keys(mapping).forEach((mapperKey) => {
fields.forEach((field) => {
Expand All @@ -14,6 +17,18 @@
%>
return request;
};
<% } else if (customBasic) { %>
const maybeIncludeAuth = (request, z, bundle) => {
const mapping = {
username: '<%= mapping.username %>',
password: '<%= mapping.password %>'
};
const username = replaceVars(mapping.username, bundle);
const password = replaceVars(mapping.password, bundle);
const encoded = `${username}:${password}`.toString('base64');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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='

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcooksey nice catch! Fixed in 7ac3941.

request.headers.Authorization = `Baisc ${encoded}`;
return request;
};
<% }

if (before && session) { %>const maybeIncludeAuth = (request, z, bundle) => {
Expand Down
4 changes: 1 addition & 3 deletions scaffold/convert/index.template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
const App = {
version: require('./package.json').version,
platformVersion: require('zapier-platform-core').version,
<% if (hasAuth) { %>
<% if (needsAuth) { %>
authentication,
<% } else { %>
authentication: {},
<% } %>
beforeRequest: [
<%= BEFORE_REQUESTS %>
Expand Down
1 change: 1 addition & 0 deletions scripts/test-convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const appsToConvert = [
{id: 82250, name: 'search-oauth'},
{id: 82251, name: 'basic-api-header'},
{id: 82460, name: 'custom-fields'},
{id: 80444, name: 'custom-baisc'},
// TODO: Add more apps that use different scripting methods, as we start to support them
];

Expand Down
36 changes: 13 additions & 23 deletions src/tests/utils/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:');
Copy link
Member Author

@eliangcs eliangcs Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

convert.hasAuth(wbDef).should.be.false();
});
});
Expand All @@ -131,7 +131,7 @@ describe('convert render functions', () => {
return convert.renderAuth(wbDef)
.then(string => {
const auth = loadAuthModuleFromString(string);
auth.type.should.eql('basic');
auth.type.should.eql('custom');
auth.fields.should.eql([
{
key: 'username',
Expand All @@ -157,7 +157,7 @@ describe('convert render functions', () => {
return convert.renderAuth(wbDef)
.then(string => {
const auth = loadAuthModuleFromString(string);
auth.type.should.eql('basic');
auth.type.should.eql('custom');
auth.fields.should.eql([
{
key: 'username',
Expand Down Expand Up @@ -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) => {
Copy link
Member Author

@eliangcs eliangcs Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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.


request.headers['Authorization'] = bundle.authData['api_key'];

return request;
};

`);
};`);
});
});

Expand Down Expand Up @@ -238,14 +236,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) => {

request.params['api_key'] = bundle.authData['api_key'];

return request;
};

`);
};`);
});
});

Expand Down Expand Up @@ -281,7 +277,7 @@ 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) => {

request.headers['X-Token'] = bundle.authData.sessionKey;

Expand Down Expand Up @@ -320,9 +316,7 @@ const getSessionKey = (z, bundle) => {
sessionKey: firstKeyValue
};
});
};

`);
};`);
});
});

Expand Down Expand Up @@ -371,14 +365,12 @@ const getSessionKey = (z, bundle) => {

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

request.headers.Authorization = \`Bearer \${bundle.authData.access_token}\`;

return request;
};

`);
};`);
});
});

Expand Down Expand Up @@ -493,14 +485,12 @@ const getSessionKey = (z, bundle) => {

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

request.headers.Authorization = \`Bearer \${bundle.authData.access_token}\`;

return request;
};

`);
};`);
});
});

Expand Down
4 changes: 2 additions & 2 deletions src/tests/utils/definitions/basic-scripting.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@
"auth_data": {},
"auth_label": "{{username}}",
"auth_mapping": {
"Password": "{{password}}",
"Username": "{{username}}"
"password": "{{password}}",
"username": "{{username}}"
},
"auth_type": "Basic Auth",
"auth_urls": {},
Expand Down
4 changes: 2 additions & 2 deletions src/tests/utils/definitions/basic.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@
"auth_data": {},
"auth_label": "{{username}}",
"auth_mapping": {
"Password": "{{password}}",
"Username": "{{username}}"
"password": "{{password}}",
"username": "{{username}}"
},
"auth_type": "Basic Auth",
"auth_urls": {},
Expand Down
45 changes: 31 additions & 14 deletions src/utils/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the logic determining if an app has auth.

};

const renderField = (definition, key, indent = 0) => {
Expand Down Expand Up @@ -199,6 +199,10 @@ const renderAuthTemplate = (authType, definition) => {
const testTriggerKey = getTestTriggerKey(definition);
const { hasGetConnectionLabelScripting } = getAuthMetaData(definition);

if (authType === 'basic' && !_.isEmpty(definition.general.auth_mapping)) {
authType = 'custom';
}

const templateContext = {
TEST_TRIGGER_MODULE: `./triggers/${snakeCase(testTriggerKey)}`,
TYPE: authType,
Expand Down Expand Up @@ -446,11 +450,17 @@ const getMetaData = (definition) => {
});
});

const hasBefore = (type === 'api-header' || type === 'api-query' || type === 'session' || type === 'oauth2' || type === 'oauth2-refresh');
const hasAfter = (type === 'session');
const needsAuth = hasAuth(definition);
const isCustomBasic = (needsAuth && type === 'basic' && !_.isEmpty(definition.general.auth_mapping));
const hasBefore = needsAuth && (
type === 'api-header' || type === 'api-query' || type === 'session' ||
type === 'oauth2' || type === 'oauth2-refresh' || isCustomBasic
);
const hasAfter = (needsAuth && type === 'session');
const fieldsOnQuery = (authPlacement === 'params' || type === 'api-query');
const isSession = (type === 'session');
const isOAuth = (type === 'oauth2' || type === 'oauth2-refresh');
const isSession = (needsAuth && type === 'session');
const isOAuth = needsAuth && (type === 'oauth2' || type === 'oauth2-refresh');

const needsLegacyScriptingRunner = isSession || hasAnyScriptingMethods;

return {
Expand All @@ -460,6 +470,7 @@ const getMetaData = (definition) => {
fieldsOnQuery,
isSession,
isOAuth,
isCustomBasic,
needsLegacyScriptingRunner,
};
};
Expand All @@ -472,6 +483,7 @@ const getHeader = (definition) => {
hasAfter,
isSession,
isOAuth,
isCustomBasic,
fieldsOnQuery,
} = getMetaData(definition);

Expand All @@ -481,6 +493,7 @@ const getHeader = (definition) => {
after: hasAfter,
session: isSession,
oauth: isOAuth,
customBasic: isCustomBasic,
fields: Object.keys(definition.auth_fields),
mapping: _.get(definition, ['general', 'auth_mapping'], {}),
query: fieldsOnQuery,
Expand Down Expand Up @@ -585,15 +598,20 @@ const writeStep = (type, definition, key, legacyApp, newAppDir) => {
};

// render the authData used in the trigger/search/create test code
const renderAuthData = (authType) => {
const renderAuthData = (definition) => {
const authType = getAuthType(definition);
let result;
switch (authType) {
case 'basic':
case 'basic': {
let lines = _.map(definition.auth_fields, (field, key) => {
const upperKey = key.toUpperCase();
return ` ${key}: process.env.${upperKey}`;
});
result = `{
username: process.env.USERNAME,
password: process.env.PASSWORD
${lines.join(',\n')}
}`;
break;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case 'oauth2':
result = `{
access_token: process.env.ACCESS_TOKEN
Expand Down Expand Up @@ -626,9 +644,8 @@ const renderAuthData = (authType) => {
};

const renderStepTest = (type, definition, key, legacyApp) => {
const authType = getAuthType(legacyApp);
const label = definition.label || _.capitalize(key);
const authData = renderAuthData(authType);
const authData = renderAuthData(legacyApp);
const templateContext = {
KEY: key,
LABEL: label,
Expand Down Expand Up @@ -661,15 +678,15 @@ const writeUtils = (newAppDir) => {
};

const renderIndex = (legacyApp) => {
const _hasAuth = hasAuth(legacyApp);
const needsAuth = hasAuth(legacyApp);
const templateContext = {
HEADER: '',
TRIGGERS: '',
SEARCHES: '',
CREATES: '',
BEFORE_REQUESTS: getBeforeRequests(legacyApp),
AFTER_RESPONSES: getAfterResponses(legacyApp),
hasAuth: _hasAuth
needsAuth,
};

return getHeader(legacyApp)
Expand All @@ -678,7 +695,7 @@ const renderIndex = (legacyApp) => {

const importLines = [];

if (_hasAuth) {
if (needsAuth) {
importLines.push("const authentication = require('./authentication');");
}

Expand Down