feat: add a support for onUnauthenticatedRequest property #7780

kamaz · 2020-05-26T21:56:35Z

Closes: #7773

@medikoo would be good to get your view on deprecation.

codecov-commenter · 2020-05-26T22:03:28Z

Codecov Report

Merging #7780 into master will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7780      +/-   ##
==========================================
+ Coverage   88.13%   88.43%   +0.30%     
==========================================
  Files         245      244       -1     
  Lines        9262     9054     -208     
==========================================
- Hits         8163     8007     -156     
+ Misses       1099     1047      -52

Impacted Files	Coverage Δ
...ins/aws/package/compile/events/alb/lib/validate.js	`97.93% <100.00%> (+1.27%)`	⬆️
lib/utils/getServerlessDir.js	`0.00% <0.00%> (-100.00%)`	⬇️
lib/plugins/aws/info/getApiKeyValues.js	`82.75% <0.00%> (-9.55%)`	⬇️
lib/plugins/slstats/slstats.js	`90.90% <0.00%> (-9.10%)`	⬇️
...ackage/compile/events/websockets/lib/deployment.js	`95.65% <0.00%> (-4.35%)`	⬇️
lib/plugins/aws/deploy/lib/createStack.js	`97.29% <0.00%> (-2.71%)`	⬇️
lib/classes/Utils.js	`92.95% <0.00%> (-2.32%)`	⬇️
lib/plugins/aws/info/getResourceCount.js	`88.88% <0.00%> (-1.12%)`	⬇️
lib/plugins/config/config.js	`88.88% <0.00%> (-0.91%)`	⬇️
lib/plugins/create/create.js	`90.80% <0.00%> (-0.31%)`	⬇️
... and 58 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 af3fbf0...7d22c4e. Read the comment docs.

medikoo

@kamaz great thanks for that! It looks really good. See my remarks, they're mostly about conventions we strive to obey currently in a project

medikoo · 2020-05-28T06:50:45Z

docs/deprecations.md

@@ -7,7 +7,12 @@ layout: Doc
 # Serverless Framework Deprecations

 <a name="BIN_SERVERLESS"><div>&nbsp;</div></a>
+<a name="AWS_ALB_EVENT_ALLOW_UNAUTHENTICATED"><div>&nbsp;</div></a>


This should be added over the given deprecation header

medikoo · 2020-05-28T06:51:43Z

docs/deprecations.md


 ## `bin/serverless`

 Please use `bin/serverless.js` instead. `bin/serverless` will be removed with v2.0.0
+
+## `AWS ALB Event allowUnauthenticated`


Let's remove ticks, make it as:

## AWS ALB Event `allowUnauthenticated`

medikoo · 2020-05-28T06:52:09Z

docs/deprecations.md

@@ -7,7 +7,12 @@ layout: Doc
 # Serverless Framework Deprecations

 <a name="BIN_SERVERLESS"><div>&nbsp;</div></a>
+<a name="AWS_ALB_EVENT_ALLOW_UNAUTHENTICATED"><div>&nbsp;</div></a>


Let's make deprecation code slightly shorter as: AWS_ALB_ALLOW_UNAUTHENTICATED

medikoo · 2020-05-28T06:52:55Z

docs/providers/aws/events/alb.md

@@ -77,7 +77,7 @@ provider:
        userPoolArn: 'arn:aws:cognito-idp:us-east-1:123412341234:userpool/us-east-1_123412341', # required
        userPoolClientId: '1h57kf5cpq17m0eml12EXAMPLE', # required
        userPoolDomain: 'your-test-domain' # required
-        allowUnauthenticated: true # If set to true this allows the request to be forwarded to the target when user is not authenticated. Omit this parameter to make a HTTP 401 Unauthorized error be returned instead
+        onUnauthenticatedRequest: 'allow' # If set to allow this allows the request to be forwarded to the target when user is not authenticated. Omit or set to 'deny' to make a HTTP 401 Unauthorized error be returned instead (default). Alternatively configure to 'authenticate' to redirect request to IdP authorization endpoint.


Let's list default option (so 'deny')

medikoo · 2020-05-28T06:54:21Z

lib/plugins/aws/package/compile/events/alb/lib/validate.js

-  validateAlbAuth(auth) {
-    auth.onUnauthenticatedRequest = auth.allowUnauthenticated ? 'allow' : 'deny';
+  validateAlbAuth(auth, authName) {
+    const hasAllowUnauthenticated = _.has(auth, ['allowUnauthenticated']);


Let's not use lodash for constructs which can easily be written in plain JS (see #7747)

medikoo · 2020-05-28T06:56:25Z

lib/plugins/aws/package/compile/events/alb/lib/validate.test.js

@@ -455,6 +455,20 @@ describe('#validate()', () => {
      });
    });

+    it('returns valid authorizer when valid object provided with disabled allowUnauthenticated', () => {


Let's construct tests using runServerless as documented here: https://github.com/serverless/serverless/blob/master/tests/README.md#unit-tests
If something is still unclear please do not hesitate to ask.

As it'll be first tests like that in this file, you can create another describe at bottom of test file to host them

Thanks for sharing a new way of testing. It looks good as it tests full configuration, not just a single method outside the context.

I'm not sure if the validate.test.js is the right place to add tests when using runServerless as my understanding it imitates serverless framework.

Would it be better to move these tests to index.test.js and have describe dedicated to onUnauthenticatedRequest and allowUnauthenticated?

@kamaz yes that's a very good point. Indeed in that case index.test.js is better (and it's where we usually put all tests for newly introduced plugins)

medikoo · 2020-05-28T06:57:49Z

lib/plugins/aws/package/compile/events/alb/lib/validate.js

+      hasOnUnauthenticatedRequest &&
+      !_.includes(supportedValues, auth.onUnauthenticatedRequest)
+    ) {
+      throw new this.serverless.classes.Error(


Remove this validation (this will be in scope of schema config validation which is handled here: #6562)

Here just assume that passed value is valid

kamaz · 2020-05-28T19:35:11Z

@kamaz great thanks for that! It looks really good. See my remarks, they're mostly about conventions we strive to obey currently in a project

@medikoo
Thanks all make sense made the changes except the tests where I need a little bit clarifications.

When it comes to lodash I didn't want to break the convention that was already set up in the file but it is good to know for future that you are moving more towards native js.

medikoo · 2020-06-08T11:00:24Z

@kamaz is it read for review? If yes, please re-request review. Thank you!

kamaz · 2020-06-08T12:05:20Z

hi @medikoo, sorry for the delay but last week was a little bit crazy for me. I should have something ready by tomorrow.

kamaz · 2020-06-08T20:37:02Z

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

@@ -79,4 +81,55 @@ describe('AwsCompileAlbEvents', () => {
      return awsCompileAlbEvents.hooks['package:compileEvents']();
    });
  });
+
+  describe('onUnauthenticatedRequest', () => {


@medikoo I thought before I start writing a set of tests to migrate them to use runServerless I will check what would like to change in the example test that tests the change to onUnauthenticatedRequest.

Do you expect to have an assertion to https://github.com/serverless/serverless/pull/7780/files#diff-f210397c5bba20a4235eaae4251556f9R131 ?

Or

Should the generated cloudformation be asserted as that is the final result?

I would probably opt-out for the latter one.

Ideally if we confirm on generated template. So something as:

serverless/lib/plugins/aws/package/compile/events/httpApi/index.test.js

Line 66 in 332524d

expect(resource.Properties.StageName).to.equal('$default');

which will validate that we have "deny" set at given property.

Still if for some reason you'll find troublesome getting right path to resource to be validated, let me know

medikoo · 2020-06-09T10:10:19Z

docs/deprecations.md

+
+## AWS ALB `allowUnauthenticated`
+
+Please use `onUnauthenticatedRequest` instead `allowUnauthenticated` will be removed with v2.0.0


Let's put . after "instead"

medikoo · 2020-06-09T10:15:04Z

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

@@ -79,4 +81,55 @@ describe('AwsCompileAlbEvents', () => {
      return awsCompileAlbEvents.hooks['package:compileEvents']();
    });
  });
+
+  describe('onUnauthenticatedRequest', () => {


Ideally if we confirm on generated template. So something as:

serverless/lib/plugins/aws/package/compile/events/httpApi/index.test.js

Line 66 in 332524d

expect(resource.Properties.StageName).to.equal('$default');

which will validate that we have "deny" set at given property.

Still if for some reason you'll find troublesome getting right path to resource to be validated, let me know

rwestergren · 2020-06-11T17:54:43Z

Looking forward to this change. At the moment, I'm having to override the generated AlbListenerRule, with resource extensions, for example:

  extensions:
    BillerAlbListenerRule1:
      Properties:
        Actions:
          - Type: authenticate-oidc
            Order: 1
            AuthenticateOidcConfig:
              OnUnauthenticatedRequest: authenticate
              AuthorizationEndpoint: https://login.microsoftonline.com/***REMOVED***/oauth2/v2.0/authorize
              ClientId: ***REMOVED***
              Issuer: https://login.microsoftonline.com/***REMOVED***/v2.0
              TokenEndpoint: https://login.microsoftonline.com/***REMOVED***/oauth2/v2.0/token
              UserInfoEndpoint: https://graph.microsoft.com/oidc/userinfo
              ClientSecret: ***REMOVED***
          - Type: forward
            TargetGroupArn: !Ref BillerAlbTargetGroupLoadBalancerListener
            Order: 2

…ents

kamaz · 2020-06-13T19:04:01Z

@medikoo thanks for sharing the initial thoughts about the testing. I hope all the comments should be done now.

medikoo

@kamaz looks great, thank you! I've just proposed some minor style improvements

medikoo · 2020-06-16T07:18:24Z

lib/plugins/aws/package/compile/events/alb/lib/validate.js

@@ -217,7 +218,21 @@ module.exports = {
  },

  validateAlbAuth(auth) {
-    auth.onUnauthenticatedRequest = auth.allowUnauthenticated ? 'allow' : 'deny';
+    const hasAllowUnauthenticated = auth.allowUnauthenticated !== undefined;


Let's use more natural != null

I'm happy to change that but not sure if != null makes it clearer. In this particular case, we only care about a case were the property is not defined not if the value is null 0 or any other.

we only care about a case were the property is not defined

If that would be the case, then proper check would be 'allowUnauthenticated' in auth.

Still in JS it's not common to do such checks. Usually best practice is just to check whether a property holds some value, where lack of value is represented by three possible scenarios:

property not defined at all

undefined assigned explicitly

null assigned explicitly.

Having that, the most straightforward way to confirm property holds a value, is to follow with obj.prop != null (that excludes just three above cases and nothing more)

medikoo · 2020-06-16T07:19:23Z

lib/plugins/aws/package/compile/events/alb/lib/validate.js

@@ -217,7 +218,21 @@ module.exports = {
  },

  validateAlbAuth(auth) {
-    auth.onUnauthenticatedRequest = auth.allowUnauthenticated ? 'allow' : 'deny';
+    const hasAllowUnauthenticated = auth.allowUnauthenticated !== undefined;
+    const hasOnUnauthenticatedRequest = auth.onUnauthenticatedRequest !== undefined;


This is a string property, so technically natural check would be Boolean(auth.onUnauthenticatedRequest)

happy to change see comment https://github.com/serverless/serverless/pull/7780/files#r441055404

medikoo · 2020-06-16T07:22:10Z

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

+          );
+
+          expect(rule.Properties.Actions[1].Type).to.equal('forward');
+          expect(rule.Properties.Actions[1].Order).to.equal(2);


It'll be nice if this test matches one that confirms for allow, so just checks for deny.

I wouldn't check for every CF property, as that technically means reimplementing CF configuration in unit tests, and it's not what I think tests should be about (Testing that given CF configuration results with desired outcome is in scope of integration tests)

medikoo · 2020-06-16T07:24:11Z

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

+        override
+      );
+
+    it('maps undefined allowUnauthenticated to "deny"', () =>


Title should rather be:

should be "deny" by default

medikoo · 2020-06-16T07:25:02Z

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

+          );
+        }));
+
+    it('maps allowUnauthenticated set to false to onUnauthenticatedRequest "allow"', () =>


`allowUnauthenticated` set to false should be ineffective

medikoo · 2020-06-16T07:26:30Z

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

+          );
+        }));
+
+    it('supports setting value to "deny"', () =>


I think either we can skip that test ("deny" is default) or name it as:

`allowUnauthenticated` should remain "deny" when set to "deny"

medikoo · 2020-06-16T07:28:41Z

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

+          expect(rule.Properties.Actions[1].Order).to.equal(2);
+        }));
+
+    it('maps allowUnauthenticated set to true to onUnauthenticatedRequest "allow"', () =>


As we're in context of suite titled onUnauthenticatedRequest. I would name test as:

maps `allowUnauthenticated` set to true to "allow"

medikoo · 2020-06-16T07:30:02Z

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

+          );
+        }));
+
+    it('takes precedence over allowUnauthenticated set "deny"', () =>


Let's remove that test. I think that testing that onUnauthenticatedRequest takes precedence over allowUnauthenticated shouldn't require more than one test (and anyway such tests will be removed with removal of support for allowUnauthenticated)

medikoo · 2020-06-16T07:30:26Z

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

+          );
+        }));
+
+    it('takes precedence over allowUnauthenticated set "allow"', () =>


Just: takes precedence over allowUnauthenticated

medikoo · 2020-06-22T14:48:36Z

@kamaz is this ready for re-review? If it is, can you re-request review (top right box) from me? Thank you!

medikoo

@kamaz great thanks for update! It looks very good, just one minor suggestion towards documentation, so it aligns our convention (although I know old version didn't follow that)

medikoo · 2020-06-23T06:51:26Z

docs/providers/aws/guide/serverless.yml.md

@@ -88,7 +88,7 @@ provider:
        userPoolArn: 'arn:aws:cognito-idp:us-east-1:123412341234:userpool/us-east-1_123412341', # required
        userPoolClientId: '1h57kf5cpq17m0eml12EXAMPLE', # required
        userPoolDomain: 'your-test-domain' # required
-        allowUnauthenticated: true # If set to true this allows the request to be forwarded to the target when user is not authenticated. Omit this parameter to make a HTTP 401 Unauthorized error be returned instead
+        onUnauthenticatedRequest: 'allow' # If set to 'allow' this allows the request to be forwarded to the target when user is not authenticated. When omitted it defaults 'deny' which makes a HTTP 401 Unauthorized error be returned. Alternatively configure to 'authenticate' to redirect request to IdP authorization endpoint.


By convention for optional parameters we put defaults as values. So let's use 'deny' as value.

medikoo

Thank you @kamaz !

feat: add a support for onUnauthenticatedRequest property

1fd4093

medikoo added cat/aws-event-alb deprecation Deprecation proposal (breaking with next major) enhancement labels May 28, 2020

medikoo suggested changes May 28, 2020

View reviewed changes

medikoo assigned kamaz May 28, 2020

fix: review comments

1d24397

tests: sample tests to verify approach

312d5b1

kamaz commented Jun 8, 2020

View reviewed changes

medikoo reviewed Jun 9, 2020

View reviewed changes

tests: migrate unit tests to run serverless implementation for alb ev…

40ad572

…ents

kamaz requested a review from medikoo June 13, 2020 19:04

medikoo suggested changes Jun 16, 2020

View reviewed changes

kamaz added 2 commits June 16, 2020 19:31

fix: review comments

1830c50

fix: comment for undefined check

f726dca

kamaz requested a review from medikoo June 22, 2020 16:00

medikoo suggested changes Jun 23, 2020

View reviewed changes

docs: update to readme to use default value for onUnauthenticatedRequest

7d22c4e

kamaz requested a review from medikoo June 25, 2020 20:39

medikoo approved these changes Jun 26, 2020

View reviewed changes

medikoo merged commit b976677 into serverless:master Jun 26, 2020

kamaz deleted the feat/alb-on-unauthenticated-request branch June 26, 2020 18:40


		## AWS ALB `allowUnauthenticated`

		Please use `onUnauthenticatedRequest` instead `allowUnauthenticated` will be removed with v2.0.0

feat: add a support for onUnauthenticatedRequest property #7780

feat: add a support for onUnauthenticatedRequest property #7780

Conversation

kamaz commented May 26, 2020

codecov-commenter commented May 26, 2020 • edited

Codecov Report

medikoo 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

kamaz commented May 28, 2020 • edited

medikoo commented Jun 8, 2020

kamaz commented Jun 8, 2020

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

rwestergren commented Jun 11, 2020

kamaz commented Jun 13, 2020

medikoo 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

medikoo commented Jun 22, 2020

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 26, 2020 •

edited

kamaz commented May 28, 2020 •

edited