Restricting alexaSkill functions to specific Alexa skills #4701

kobim · 2018-01-31T15:44:10Z

What did you implement:

Closes #4700 - using the same concept as alexaSmartHome event which forces you to specify skill ID.
This implementation is backwards compatible and alerts the users to migrate to the new syntax.

Also, added the option to provide multiple alexaSkill events per a function, allowing it to be invoked by multiple skills.

How did you implement it:

Added the options to provide either a string or an object to the event, while the values impact the Permission Policy which is sent to AWS.
By default, the alexaSkill is enabled.

How can we verify it:

Example configs (also available in the updated documents):

one function, one skill:

functions:
  mySkill:
    handler: mySkill.handler
    events:
      - alexaSkill: amzn1.ask.skill.xx-xx-xx-xx-xx

one function, two skills (of which one is disabled):

functions:
  mySkill:
    handler: mySkill.handler
    events:
      - alexaSkill:
          appId: amzn1.ask.skill.xx-xx-xx-xx
      - alexaSkill:
          appId: amzn1.ask.skill.yy-yy-yy-yy
          enabled: false

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification config / commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

- Adding support for multiple `alexaSkill` events on a single function (allows multiple Alexa Skills on a single lambda)

… (hopefully) (https://travis-ci.org/serverless/serverless/jobs/335361582 / https://travis-ci.org/serverless/serverless/jobs/335706593 / https://travis-ci.org/serverless/serverless/jobs/335682396)

…ered.

HyperBrain

Thanks for the PR :). Please have a look at my comments.

HyperBrain · 2018-02-01T14:59:04Z

lib/plugins/aws/deploy/lib/extendedValidate.test.js

@@ -126,7 +126,7 @@ describe('extendedValidate', () => {
      return awsDeploy.extendedValidate().then(() => {
        expect(fileExistsSyncStub.calledTwice).to.equal(true);
        expect(readFileSyncStub.calledOnce).to.equal(true);
-        expect(fileExistsSyncStub).to.have.been.calledWithExactly('artifact.zip');
+        expect(fileExistsSyncStub.calledWithExactly('artifact.zip'));


You removed the check of the expectation here. This change will not evaluate the expectation result anymore.
The previous code was ok imo.

calledWithExactly is a sinon function, not chai's. I think I'm missing here a .to.equal(true).

(side note - the 3 unrelated test files I changed were causing occasional failures in the test process. I added some links in the commit.)

we use sinon-chai which adds these extensions ;-) So the old one is correct and provides better error messages than just "expected true to equal false"

Ah, it was missing in this particular file (also explains why I received TypeError: expect(...).to.have.been.calledWithExactly is not a function when testing). I'll take care of it.

HyperBrain · 2018-02-01T15:00:48Z

lib/plugins/aws/deploy/lib/uploadArtifacts.test.js

-      sinon.stub(fs, 'statSync').returns({ size: 1024 });
-      sinon.stub(awsDeploy, 'uploadZipFile').resolves();
+      const statSyncStub = sinon.stub(fs, 'statSync').returns({ size: 1024 });
+      const uploadZipFileStub = sinon.stub(awsDeploy, 'uploadZipFile').resolves();


If you create the stubs inside the test, you have to make sure that they are restored if the test exists. Even if it enters a rejection path. Also use the sandbox to create the stubs. That makes sure that they are removed properly as soon as the sanbox is cleaned up.

So having this, you must add a .finally() clause to the test that restores and removes the stubs again. Otherwise following tests are affected.

In general, to prevent such stub leaks, it's better to setup the test stubs in before() or beforeEach() and clean them in the resp. after methods.

I can rewrite these tests to use the sandbox, but I don't think it related to this PR.
The changes I made were only to make sure these tests won't randomly fail (1) (2)

Agree. It's out of scope. So the stabilization of the unit tests should include this

HyperBrain · 2018-02-01T15:01:33Z

lib/plugins/aws/lib/naming.js

-  getLambdaAlexaSkillPermissionLogicalId(functionName) {
-    return `${this.getNormalizedFunctionName(functionName)}LambdaPermissionAlexaSkill`;
+  getLambdaAlexaSkillPermissionLogicalId(functionName, alexaSkillIndex) {
+    return `${this.getNormalizedFunctionName(functionName)}LambdaPermissionAlexaSkill${


You could default the skill index to 0 here.

not sure i'm following you here.. do you want the logical numbering to start from 0 (instead of the current 1 index), or do you want this function to have a default parameter value? (the project supports node v4+, but default parameters were only enabled at node v6).

I was just thinking about if it would be allowed to omit the skillIndex here in the function so that it defaults to 0 if it is not set. On the other hand that behavior does not really make sense, because the caller is currently only inside the aws plugin.
Only if a 3rd party plugin uses this function in naming this would matter - but then the call should fail if no index is given if we do not use a default inside.

Would you be ok with a solution like

return `${this.getNormalizedFunctionName(functionName)}LambdaPermissionAlexaSkill${ alexaSkillIndex || 0}`;

?

Yeah, that sounds reasonable... should be '0' with quotes, shouldn't it?

HyperBrain · 2018-02-01T15:03:29Z

lib/plugins/aws/package/compile/events/alexaSkill/index.js

+              throw new this.serverless.classes.Error(errorMessage);
+            }
+            appId = event.alexaSkill.appId;
+            enabled = event.alexaSkill.enabled !== false;


enabled = event.alexaSkill.enabled; - having the explicit not-equal check for false just obscures this and lessens readability.

the explicit not-equal check here is because the enabled option is optional and defaults to true.
The following definition:

- alexaSkill: appId: amzn1.ask.skill.yy-yy-yy-yy

should be considered enabled.

Oh, ok. Maybe you can add a comment above the assignment, because it is not obvious without knowing exactly this context 😄 And other people might then change it without anyone really complaining.

HyperBrain · 2018-02-01T15:03:59Z

lib/plugins/aws/package/compile/events/alexaSkill/index.js

+              ' Please refer to the documentation for additional information.',
+            ].join('');
+            this.serverless.cli.log(warningMessage);
+          } else if (typeof event.alexaSkill === 'string') {


We should use lodash consistently _.isString()

HyperBrain · 2018-02-01T15:04:51Z

lib/plugins/aws/package/compile/events/alexaSkill/index.js

+            this.serverless.cli.log(warningMessage);
+          } else if (typeof event.alexaSkill === 'string') {
+            appId = event.alexaSkill;
+          } else if (typeof event.alexaSkill === 'object') {


Use _.isPlainObject() or _.isObject() here, dependeing if you'd allow inherited full objects or just plain objects.

HyperBrain · 2018-02-01T15:08:04Z

lib/plugins/aws/package/compile/events/alexaSkill/index.test.js

+      expect(awsCompileAlexaSkillEvents.serverless.service
+        .provider.compiledCloudFormationTemplate.Resources
+        .FirstLambdaPermissionAlexaSkill1.Properties.EventSourceToken
+      ).to.be.an('undefined');


).to.be.undefined;

HyperBrain · 2018-02-01T15:10:42Z

lib/plugins/aws/deploy/lib/uploadArtifacts.test.js


      return awsDeploy.uploadFunctions().then(() => {
        expect(uploadZipFileStub.calledTwice).to.be.equal(true);
        expect(uploadZipFileStub.args[0][0])
          .to.be.equal(awsDeploy.serverless.service.functions.first.package.artifact);
        expect(uploadZipFileStub.args[1][0])
          .to.be.equal(awsDeploy.serverless.service.package.artifact);
-        awsDeploy.uploadZipFile.restore();
-        fs.statSync.restore();
+        uploadZipFileStub.restore();


This is wrong in case id the test fails. You need to move the restores into a .finally() at the end of the promise chain to guarantee that they are executed, regardless which branch the promise chain takes. Otherwise the test will pollute the test environment for all tests executed afterwards.

HyperBrain · 2018-02-01T15:11:09Z

lib/plugins/aws/deploy/lib/uploadArtifacts.test.js

      sinon.spy(awsDeploy.serverless.cli, 'log');

      return awsDeploy.uploadFunctions().then(() => {
        const expected = 'Uploading service .zip file to S3 (1 KB)...';
        expect(awsDeploy.serverless.cli.log.calledWithExactly(expected)).to.be.equal(true);

-        fs.statSync.restore();
-        awsDeploy.uploadZipFile.restore();
+        statSyncStub.restore();


.finally() here too

…eb498ecdd7c)

kobim · 2018-02-02T11:06:10Z

@HyperBrain - I finally reproduced the createStack random error. The problem is that sinon-bluebird wraps BlueBird's reject method with it's own wrapper (RejectBPromise) and it applies the promise. (hence mocha yields Uncaught does not exist sometimes). Adding 1 second of a delay to the test will reproduce the error every time:

    it('should set the createLater flag and resolve if deployment bucket is provided', () => {
      awsDeploy.serverless.service.provider.deploymentBucket = 'serverless';
      sandbox.stub(awsDeploy.provider, 'request')
        .returns(BbPromise.reject({ message: 'does not exist' }));

      return new Promise(resolve => {
        setTimeout(resolve, 1000);
      }).then(() => {
        awsDeploy.createStack().then(() => {
          expect(awsDeploy.createLater).to.equal(true);
        });
      });
    });

This is the only place BbPromise.reject is used in a sinon stub (commit 9631f), and I'm not sure why it replaced the previous syntax (rejecting with an object).
What do you think?

HyperBrain · 2018-02-02T11:26:01Z

@kobim Good catch. That's indeed the error.

.returns(BbPromise.reject({ message: 'does not exist' }));
is definitely wrong here. A rejection must always reject with an error, and never with an object. That's where sinon-buebird gets confused and cannot handle the stubbed rejection anymore.

It should be .rejects(new Error('does not exist'));.

Can you try that? Then the "does not exist" error check should work properly.

…lves to an error.

HyperBrain · 2018-02-02T11:38:35Z

@horike37 @pmuens Thanks to @kobim we now got rid of the createStack unit test failures that often seemed to happen without a reason 🎉 🎉 . The fix (see the 2 comments above) is include in this PR which I will merge soon. We should check the upload tests too, because they show a quite similar behavior.

HyperBrain · 2018-02-02T11:39:57Z

@kobim Travis succeeded now, and I'm quite sure that the createStack tests are now ultimately fixed. Thanks for that. I'll finish the review now.

kobim · 2018-02-02T14:22:00Z

@HyperBrain - just found out that if you specify an event which isn't alexaSkill, an error is thrown.
I'll add a new PR with the fix and test case soon.

HyperBrain · 2018-02-02T14:35:03Z

@kobim Thanks for the note. I will review merge the new PR then as sson as it is there. Please add me explicitly as reviewer there.

* The new alexaSkill skill id restriction (#4701) was throwing an error if an event which is not `alexaSkill` was presented. * `.to.not.throw()` is a function, not a getter.

Saandji · 2018-05-11T08:17:17Z

Is it released? I tried to specify 2 skills for my lambda function and it didn't work with latest serverless 1.27.2

When I specify:

    events:
      - alexaSkill:
        appId: amzn1.ask.skill.xx
        enabled: true
      - alexaSkill: 
        appId: amzn1.ask.skill.yy
        enabled: true

The trigger gets removed completely

kobim · 2018-05-11T08:44:40Z

@Saandji it was released with 1.27.0, and I just verified it with 1.27.2:

functions:
  mySkill:
    handler: handler.alexa
    events:
      - alexaSkill: amzn1.ask.skill.xx-xx-xx-xx-xx

ended up with the following at .serverless/cloudformation-template-update-stack.json:

    "MySkillLambdaFunction": {
      "Type": "AWS::Lambda::Function",
...
        "Handler": "handler.alexa",
...
    },
    "MySkillLambdaPermissionAlexaSkill1": {
...
        "Action": "lambda:InvokeFunction",
        "Principal": "alexa-appkit.amazon.com",
        "EventSourceToken": "amzn1.ask.skill.xx-xx-xx-xx-xx" // <--- PR #4701
      }
    }

Saandji · 2018-05-11T08:47:11Z

@kobim for one skill id it works like a charm for me as well. But when I specify multiple skills, it removes Alexa triggers from my function

kobim · 2018-05-11T08:56:36Z

I think you are missing spaces in your yml (see in the docs), try:

    events:
      - alexaSkill:
          appId: amzn1.ask.skill.xx
          enabled: true
      - alexaSkill: 
          appId: amzn1.ask.skill.yy
          enabled: true

YAML is very sensitive for its spaces:

events:
   - alexaSkill:
     appId: 1

translates to

{
  "events": [
    {
      "alexaSkill": null,
      "appId": 1
    }
  ]
}

while the correct format will translate to:

{
  "events": [
    {
      "alexaSkill": {
        "appId": 1
      }
    }
  ]
}

Saandji · 2018-05-11T08:57:28Z

@kobim sorry I'm an idiot, I really did miss the space! Thank you very much! Sorry for bothering =)

kobim · 2018-05-11T08:58:24Z

All good, it happens to everyone :)

kobim added 5 commits January 31, 2018 17:36

- Adding support for restricting the executing alexa skill by id (#4700)

dbd76a5

- Adding support for multiple `alexaSkill` events on a single function (allows multiple Alexa Skills on a single lambda)

Adding a comment on the serverless.cli addition when testing.

5434f45

Updating templates to have the right alexaSkill syntax

692ac58

Cleaning up Travis-CI's occasional errors with sinon stubs going wild…

c967c8d

… (hopefully) (https://travis-ci.org/serverless/serverless/jobs/335361582 / https://travis-ci.org/serverless/serverless/jobs/335706593 / https://travis-ci.org/serverless/serverless/jobs/335682396)

one last test case to make sure the alexaSkill file will be fully cov…

4296691

…ered.

kobim changed the title ~~Supports restricting alexaSkill functions to specific Alexa skills~~ Restricting alexaSkill functions to specific Alexa skills Jan 31, 2018

HyperBrain added the stage/in-review label Feb 1, 2018

HyperBrain suggested changes Feb 1, 2018

View reviewed changes

kobim added 3 commits February 2, 2018 00:12

PR notes

e0715bb

naming - Add a default suffix for alexaSkillLogicalId if undefined

73126ad

Revert changes to createStack.test.js (#c967c8d956b3d96afbaefa7fbe3e6…

6b98e24

…eb498ecdd7c)

createStack.test - reject with an Error, not with a promise that reso…

82e6b68

…lves to an error.

HyperBrain approved these changes Feb 2, 2018

View reviewed changes

HyperBrain added this to the 1.27 milestone Feb 2, 2018

HyperBrain merged commit d7eea20 into serverless:master Feb 2, 2018

kobim mentioned this pull request Feb 2, 2018

Stop throwing errors if alexaSkill is not in the events list #4708

Merged

7 tasks

kobim deleted the feature/alexaSkillId branch February 2, 2018 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restricting alexaSkill functions to specific Alexa skills #4701

Restricting alexaSkill functions to specific Alexa skills #4701

kobim commented Jan 31, 2018 •

edited

HyperBrain left a comment

HyperBrain Feb 1, 2018

kobim Feb 1, 2018

HyperBrain Feb 1, 2018 •

edited

kobim Feb 1, 2018

HyperBrain Feb 1, 2018

HyperBrain Feb 1, 2018

kobim Feb 1, 2018

HyperBrain Feb 1, 2018

HyperBrain Feb 1, 2018

kobim Feb 1, 2018

HyperBrain Feb 1, 2018 •

edited

kobim Feb 1, 2018

HyperBrain Feb 1, 2018 •

edited

HyperBrain Feb 1, 2018

kobim Feb 1, 2018

HyperBrain Feb 1, 2018 •

edited

HyperBrain Feb 1, 2018

HyperBrain Feb 1, 2018

HyperBrain Feb 1, 2018

HyperBrain Feb 1, 2018

HyperBrain Feb 1, 2018

kobim commented Feb 2, 2018

HyperBrain commented Feb 2, 2018 •

edited

HyperBrain commented Feb 2, 2018

HyperBrain commented Feb 2, 2018

kobim commented Feb 2, 2018

HyperBrain commented Feb 2, 2018 •

edited

Saandji commented May 11, 2018 •

edited

kobim commented May 11, 2018

Saandji commented May 11, 2018

kobim commented May 11, 2018

Saandji commented May 11, 2018

kobim commented May 11, 2018 •

edited

Restricting alexaSkill functions to specific Alexa skills #4701

Restricting alexaSkill functions to specific Alexa skills #4701

Conversation

kobim commented Jan 31, 2018 • edited

What did you implement:

How did you implement it:

How can we verify it:

one function, one skill:

one function, two skills (of which one is disabled):

Todos:

HyperBrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyperBrain Feb 1, 2018 • edited

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

HyperBrain Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyperBrain Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyperBrain Feb 1, 2018 • edited

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

kobim commented Feb 2, 2018

HyperBrain commented Feb 2, 2018 • edited

HyperBrain commented Feb 2, 2018

HyperBrain commented Feb 2, 2018

kobim commented Feb 2, 2018

HyperBrain commented Feb 2, 2018 • edited

Saandji commented May 11, 2018 • edited

kobim commented May 11, 2018

Saandji commented May 11, 2018

kobim commented May 11, 2018

Saandji commented May 11, 2018

kobim commented May 11, 2018 • edited

kobim commented Jan 31, 2018 •

edited

HyperBrain Feb 1, 2018 •

edited

HyperBrain Feb 1, 2018 •

edited

HyperBrain Feb 1, 2018 •

edited

HyperBrain Feb 1, 2018 •

edited

HyperBrain commented Feb 2, 2018 •

edited

HyperBrain commented Feb 2, 2018 •

edited

Saandji commented May 11, 2018 •

edited

kobim commented May 11, 2018 •

edited