Sc 68589/new integration bearer authentication and #13

cgrayson · 2024-03-22T18:55:18Z

Description of the change

Adds new token-authenticated JSON integration. See my comments on sc-68589 from 5/7 & 5/15 for details on the approach taken here.

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Configuration change
Technical Debt
Documentation

Related tickets

https://app.shortcut.com/active-prospect/story/68589/new-integration-bearer-authentication-and-authorization-method

Checklists

Development and Testing

Lint rules pass locally.
The code changed/added as part of this pull request has been covered with tests, or this PR does not alter production code.
All tests related to the changed code pass in development, or tests are not applicable.

Code Review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached.
At least two engineers have been added as "Reviewers" on the pull request.
Changes have been reviewed by at least two other engineers who did not write the code.
This branch has been rebased off master to be current.

Tracking

Issue from Shortcut/Jira has a link to this pull request.
This PR has a link to the issue in Shortcut.

QA

This branch has been deployed to staging and tested.

shortcut-integration · 2024-03-22T18:55:21Z

This pull request has been linked to Shortcut Story #68589: New Integration: Bearer authentication and authorization method.

cgrayson · 2024-03-22T18:56:31Z

lib/ui/public/index.html

@@ -1,7 +1,6 @@
 <!DOCTYPE html>
 <head>
  <title>Leadconduit Integration</title>
-  <link href="/lc-client.css" rel="stylesheet">


(Not a big deal but this should never have been checked in this way.)

cgrayson · 2024-03-22T18:58:53Z

test/json-spec.js

@@ -1,267 +1,284 @@
-// Generated by CoffeeScript 1.10.0


The only real change here was to add PATCH as a valid verb (L233) to fix a test failure from upstream changes; everything else is just reformatting from an old CoffeeScript conversion that never got cleaned up.

I very much appreciate this comment, I would have been so lost 🙏🏻

ericyd

Looks great! All my comments are just nits and don't need to be addressed. Thanks for the detailed notes in the ticket and the thorough tests!

ericyd · 2024-03-22T20:40:33Z

index.js

@@ -1,5 +1,6 @@
 module.exports = {
  outbound: {
+    token_authenticated: require('./lib/authjson'),


I'll be honest, I don't know the point of the /docs directory, but should we add an entry there too?

These markdown files are the source of the descriptions in-app and on the "catalog" pages. Absolutely need one here, thanks!

ericyd · 2024-03-22T20:42:28Z

test/json-spec.js

@@ -1,267 +1,284 @@
-// Generated by CoffeeScript 1.10.0


I very much appreciate this comment, I would have been so lost 🙏🏻

ericyd · 2024-03-22T21:00:12Z

lib/ui/public/app/auth/Auth.vue

-      <button v-on:click="$store.dispatch('cancel')">Cancel</button>
-      <button v-on:click="next" class="primary">{{ (credential.username && credential.password) ? 'Continue' : 'Skip' }}</button>
-    </footer>
+    <Navigation :onNext="next" :disableNext="(credential.type === 'token' && !credential.token) || (credential.type === 'user' && (!credential.username || !credential.password))" />


a computed property for the :disableNext prop might be a little easier to read, but certainly not a blocker

Good call. And I went ahead & made that property function a little more verbose; feedback welcome on that.

I like it! The more verbose version is a bit easier to reason about I think 👍🏻

ericyd · 2024-03-22T21:08:12Z

lib/authHelpers.js

+const substituteHeaderTokens = (headers, credential) => {
+  const tokenAttributes = getTokenAttributes(credential);
+
+  if (headers && tokenAttributes) {


nit, but tokenAttributes will always be truthy so it isn't doing anything in this conditional. You could use tokenAttributes.length, but ultimately it won't matter much so I'll leave it to you?

Good call; though the result is the same without, I added .length because I felt checking both here is a tiny bit more descriptive.

ericyd · 2024-03-22T21:13:44Z

lib/authHelpers.js

+    const normalizePart = (part) => capitalize(part);
+    const normalField = key.split('-').map(normalizePart).join('-');


nit, but normalizePart is identical to capitalize, you could just map(capitalize) and have the same behavior I believe

ericyd · 2024-03-22T21:15:32Z

test/authHelpers-spec.js

very nice specs! 👏🏻

ericyd · 2024-03-22T21:42:11Z

test/authjson-spec.js

+        const pending = nock.pendingMocks();
+        assert.equal(pending.length, 1);
+        assert.isTrue(pending.includes(`POST ${baseAuthUrl}:443/login`));


TIL about pendingMocks(), that's pretty handy!

ericyd · 2024-03-22T21:47:40Z

lib/authjson.js

+          // update vars.credential with the new token, using the user-configured (or default) attribute
+          const tokenPath = vars.authentication_token_path || 'accessToken';
+          const tokenName = tokenPath.split('.').at(-1); // use the name after the final '.', if there are any
+          vars.credential[tokenName] = get(tokenResponse, tokenPath);


I know this is a stupid question but I've been meaning to ask and I keep forgetting.

Are credentials saved back to the DB after integrations run? I've been wondering how the token gets persisted between integration invocations

Not a stupid question at all; saving updated credentials is one of the many things send does. See https://github.com/activeprospect/leadconduit-api/blob/59c0a7a3dd966e99bf8dd9c1d20a08283c42bf6e/lib/handler/util/send.js#L291-L316 (& ff.)

cgrayson · 2024-03-25T16:23:07Z

(fyi - still some QA feedback to address before re-review)

ericyd

Looks great!

cgrayson added 5 commits March 8, 2024 16:46

WIP: basic working structure & start of tests

cd0228f

reworked UI for token credential

c2c9179

add tests

623651b

hacked some CSS to tighten things up

d935c3d

reformat old test file to standard JS (and add PATCH to fix test)

1969413

cgrayson requested a review from ericyd March 22, 2024 18:55

cgrayson commented Mar 22, 2024

View reviewed changes

revert version #

96a70c1

cgrayson commented Mar 22, 2024

View reviewed changes

ericyd previously approved these changes Mar 22, 2024

View reviewed changes

address PR feedback

ddb3d4f

cgrayson dismissed ericyd’s stale review via ddb3d4f March 25, 2024 16:18

cgrayson requested a review from ericyd March 26, 2024 18:37

ericyd approved these changes Mar 26, 2024

View reviewed changes

cgrayson merged commit 67c5a42 into master Mar 26, 2024
3 checks passed

cgrayson deleted the sc-68589/new-integration-bearer-authentication-and branch March 26, 2024 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sc 68589/new integration bearer authentication and #13

Sc 68589/new integration bearer authentication and #13

cgrayson commented Mar 22, 2024

shortcut-integration bot commented Mar 22, 2024

cgrayson Mar 22, 2024

cgrayson Mar 22, 2024 •

edited

Loading

ericyd Mar 22, 2024

ericyd left a comment

ericyd Mar 22, 2024

cgrayson Mar 25, 2024

ericyd Mar 22, 2024

ericyd Mar 22, 2024

cgrayson Mar 25, 2024

ericyd Mar 26, 2024

ericyd Mar 22, 2024

cgrayson Mar 25, 2024

ericyd Mar 22, 2024

ericyd Mar 22, 2024

ericyd Mar 22, 2024

ericyd Mar 22, 2024

cgrayson Mar 25, 2024

cgrayson commented Mar 25, 2024

ericyd left a comment

		const normalizePart = (part) => capitalize(part);
		const normalField = key.split('-').map(normalizePart).join('-');

Sc 68589/new integration bearer authentication and #13

Sc 68589/new integration bearer authentication and #13

Conversation

cgrayson commented Mar 22, 2024

Description of the change

Type of change

Related tickets

Checklists

Development and Testing

Code Review

Tracking

QA

shortcut-integration bot commented Mar 22, 2024

Choose a reason for hiding this comment

cgrayson Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericyd 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

cgrayson commented Mar 25, 2024

ericyd left a comment

Choose a reason for hiding this comment

cgrayson Mar 22, 2024 •

edited

Loading