websocket event authorizer now supports name and arn fields together by rzaldana · Pull Request #8335 · serverless/serverless

rzaldana · 2020-10-02T14:03:54Z

Closes: #8328

codecov-commenter · 2020-10-02T14:06:13Z

Codecov Report

Merging #8335 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #8335   +/-   ##
=======================================
  Coverage   88.01%   88.02%           
=======================================
  Files         249      249           
  Lines        9306     9308    +2     
=======================================
+ Hits         8191     8193    +2     
  Misses       1115     1115

Impacted Files	Coverage Δ
...ins/aws/package/compile/events/websockets/index.js	`100.00% <ø> (ø)`
.../package/compile/events/websockets/lib/validate.js	`97.36% <100.00%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc34140...423269d. Read the comment docs.

…(schema also changed)

medikoo

@zaldanaraul thank you! That looks very good. I have just remark concerning tests. We now stay away from old way of writing tests (as they're very problematic for eventual refactors), see my comment

medikoo · 2020-10-05T12:40:44Z

  });

+  it('should add authorizer config when authorizer is specified as an object with both arn and name', () => {
+    awsCompileWebsocketsEvents.serverless.service.functions = {


Let's configure this test with runServerless util. See: https://github.com/serverless/serverless/tree/master/test#unit-tests

hi @medikoo I just did this. Please take a look to see if its appropriate

medikoo

Thank you @zaldanaraul ! That looks great. I've spotted just one issue in configured test

medikoo · 2020-10-09T10:49:37Z

+    const nameField = 'authName';
+    const arnName = 'auth';
+
+    runServerless({


Let's use async/await (note also that here promise as being not returned is orphaned)

@medikoo just changed to use async/await. This explains some issues I was having earlier. Thanks for pointing out!

@zaldanaraul as I look they return the promise (note no brackets wrapping arrow function body).

There's no async/await as they were written before we could use it before breaking things. While it be nice to have everything migrated to async/await, it's not easily doable, do we do it on demand

you're right. Just noticed the missing brackets. My bad!

codecov-io · 2020-10-09T14:04:14Z

Codecov Report

Merging #8335 into master will increase coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #8335      +/-   ##
==========================================
+ Coverage   88.01%   88.04%   +0.02%     
==========================================
  Files         249      249              
  Lines        9306     9287      -19     
==========================================
- Hits         8191     8177      -14     
+ Misses       1115     1110       -5

Impacted Files	Coverage Δ
...ins/aws/package/compile/events/websockets/index.js	`100.00% <ø> (ø)`
.../package/compile/events/websockets/lib/validate.js	`97.56% <83.33%> (+0.33%)`	⬆️
...ckage/compile/events/websockets/lib/permissions.js	`100.00% <100.00%> (+5.88%)`	⬆️
...ws/package/compile/events/cloudWatchEvent/index.js	`97.67% <0.00%> (-2.33%)`	⬇️
...ns/aws/package/compile/events/eventBridge/index.js	`98.30% <0.00%> (-1.70%)`	⬇️
.../package/compile/events/apiGateway/lib/validate.js	`96.23% <0.00%> (-0.88%)`	⬇️
lib/plugins/aws/deployFunction/index.js	`89.56% <0.00%> (-0.71%)`	⬇️
lib/classes/Variables.js	`99.72% <0.00%> (-0.01%)`	⬇️
lib/plugins/aws/lib/monitorStack.js	`100.00% <0.00%> (ø)`
lib/plugins/aws/package/compile/events/s3/index.js	`100.00% <0.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc34140...027d24f. Read the comment docs.

medikoo

Great thanks for update!

Sorry for late notice, but I've just realized it needs a bit of additional work, to be a complete fix for #8328. Please see my comments

medikoo · 2020-10-09T14:55:16Z

@@ -72,7 +72,7 @@ class AwsCompileWebsockets {
                    arn: { $ref: '#/definitions/awsArnString' },


To fully fix #8328 we need to change here awsArnString to awsArn

medikoo · 2020-10-09T14:55:53Z

+                  route: '$connect',
+                  authorizer: {
+                    name: nameField,
+                    arn: `arn:aws:${arnName}`,


Let's use object form of ARN for this test

medikoo · 2020-10-09T14:59:23Z

-                websocketObj.authorizer.name = getAuthorizerNameFromArn(
-                  event.websocket.authorizer.arn
-                );
+                if (event.websocket.authorizer.name) {


To not be breaking, we actually need to consturct this resolution as:

if (_.isObject(event.websocket.authorizer.arn)) { if (!event.websocket.authorizer.name) throw new ServerlessError(....) websocketObj.authorizer.name = event.websocket.authorizer.name; } else { websocketObj.authorizer.name = getAuthorizerNameFromArn(...) }

rzaldana · 2020-10-11T16:11:18Z

Hi @medikoo , Just implemented suggested changes. Also had to change a line in permissions.js . See below:

Old:

serverless/lib/plugins/aws/package/compile/events/websockets/lib/permissions.js

Line 46 in 661caad

if (event.authorizer.permission.includes(':')) {

New:

serverless/lib/plugins/aws/package/compile/events/websockets/lib/permissions.js

Line 46 in 9d03ff6

    
           if (_.isObject(event.authorizer.permission) || event.authorizer.permission.includes(':')) {

Please let me know if it needs further changes!

medikoo

@zaldanaraul that looks great! I have just one last very minor remark and we're good to go

medikoo · 2020-10-12T11:41:04Z

+                if (_.isObject(event.websocket.authorizer.arn)) {
+                  if (!event.websocket.authorizer.name) {
+                    throw new ServerlessError(
+                      'Websocket Authorizer: Non-string arn needs to be accompanied with "name"'


Let's put arn in quotes

medikoo

Thank you @zaldanaraul !

websocket event authorizer now supports name and arn fields together …

423269d

…(schema also changed)

rzaldana force-pushed the master branch from 01325c5 to 423269d Compare October 2, 2020 20:14

medikoo suggested changes Oct 5, 2020

View reviewed changes

medikoo added cat/aws-event-websocket enhancement labels Oct 5, 2020

medikoo assigned rzaldana Oct 5, 2020

refactored test to use runServerless util

b350c15

rzaldana requested a review from medikoo October 9, 2020 00:29

medikoo suggested changes Oct 9, 2020

View reviewed changes

changed test to use async/await

4704a4e

rzaldana requested a review from medikoo October 9, 2020 14:52

medikoo suggested changes Oct 9, 2020

View reviewed changes

rzaldana marked this pull request as draft October 11, 2020 14:25

add support for arn object

274f89c

rzaldana force-pushed the master branch from 6b79687 to 274f89c Compare October 11, 2020 15:55

rzaldana marked this pull request as ready for review October 11, 2020 16:00

medikoo reviewed Oct 12, 2020

View reviewed changes

made error message more specific

027d24f

rzaldana force-pushed the master branch from 9d03ff6 to 027d24f Compare October 12, 2020 14:11

rzaldana requested a review from medikoo October 12, 2020 14:16

medikoo approved these changes Oct 12, 2020

View reviewed changes

medikoo merged commit 9303d8e into serverless:master Oct 12, 2020

		@@ -72,7 +72,7 @@ class AwsCompileWebsockets {
		arn: { $ref: '#/definitions/awsArnString' },

Uh oh!

Conversation

rzaldana commented Oct 2, 2020

Uh oh!

codecov-commenter commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzaldana Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzaldana commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Oct 2, 2020 •

edited

Loading

rzaldana Oct 9, 2020 •

edited

Loading

codecov-io commented Oct 9, 2020 •

edited

Loading

rzaldana commented Oct 11, 2020 •

edited

Loading