feat: Support for request verification by klcodanr · Pull Request #31 · adobe/content-lake-commons

klcodanr · 2023-05-25T13:21:34Z

Please ensure your pull request adheres to the following guidelines:

make sure to link the related issues in this description
when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

https://trello.com/c/2A4zpMbE/250-create-commons-library-for-creating-verifying-frontegg-backend-tokens

Notes:

This includes breaking changes to the SecretsManager and the Router
SecretsManager changes: improving the structure of the secrets to better allow for identifying / managing secrets by making the pattern:
[scope:test,prod]/group:companyId,shared]/[application]/[key]
Router - changed the parameters passed to the handler to have the params be the first and an info object containing the request and context as the second. The impetus being that generally you already have the context and request available and in most case I was ignoring these params which made for clunky code
This implementation is pretty naive -- we probably should put some caching in place
The code for checking roles / permissions doesn't quite work for M2M tokens as there's a different method that we're supposed to use, but it doesn't work the way described in Frontegg's docs (but that's not essential now anyway because the scope of this isn't M2M tokens)

Thanks for contributing!

jdelbick · 2023-05-25T17:22:23Z

+    const response = await router.handle(
+      new Request('https://localhost/', {
+        headers: {
+          authorization: 'Bearer yourbearerhere',


+
+    it('rejects non-bearer authorization header', async () => {
+      const request = createRequest('test-space', token);
+      request.headers.set('authorization', 'basic YWRtaW46YWRtaW4=');


jdelbick

Mostly questions, but so far LGTM

jdelbick · 2023-05-25T17:09:05Z

+| spaceId | <code>string</code> | the space for which the token will be generated |
+| roleKeys | <code>Array.&lt;string&gt;</code> | the role keys for which the token should be generated |
+| generator | <code>string</code> | provides attribution for the key in the description |
+| [expiresInMinutes] | <code>number</code> | the number of minutes before the token expires (or 14 days) |


qs: why isn't the companyId needed?

It's not used for validating the token, but it does make sense that we need to validate that as well as the space id.

jdelbick · 2023-05-25T17:18:12Z

+    const spaceId = this.#getRequiredHeader(req, 'x-space-id');
+    const payload = jwt.decode(token);
+
+    // first validate that the tenant is correct


qs: is there a way to validate the company id is a valid company id too/first?

That's a good question. cc/ @alexkli

for the record, I am working on getting the company id (parent tenant id) into the JWT. this is possible using the jwt generation frontegg prehook. adding this new hook over in adobe/content-lake-frontegg-service.

jdelbick · 2023-05-25T17:20:52Z

+   * @param {Array<string>} roleIds
+   * @param {number | undefined} expiresInMinutes
+   */
+  async #generateAccessToken(


in the process through the UI -> backend, the request will already have this access token from frontegg right?

so token generation is only needed in situations where the endpoint is called from outside frontegg login screen (ie extractors or manual requests)

Right now, the only real case right now is to support the IT, however, this could be used for backend calls (for example Dropbox Extractor calling Ingestor)

jdelbick · 2023-05-25T17:22:23Z

+    const response = await router.handle(
+      new Request('https://localhost/', {
+        headers: {
+          authorization: 'Bearer yourbearerhere',


github-actions · 2023-05-26T13:31:15Z

This PR will trigger a minor release when merged.

+  });
+
+  it('will not authorize invalid token', async () => {
+    await assert.rejects(() => security.authorize(createRequest('not a valid token')));


jdelbick · 2023-05-27T00:50:06Z

fyi 2 of the security tests are failing:

 2 failing

  1) Security Unit Tests
       authorization
         rejects malformed bearer token:
     AssertionError [ERR_ASSERTION]: undefined == 401
      at assertFailsWithStatus (file:///home/circleci/project/test/security.test.js:42:10)
      at async Context.<anonymous> (file:///home/circleci/project/test/security.test.js:75:7)

  2) Security Unit Tests
       authorization
         rejects tokens from other issuers:
     AssertionError [ERR_ASSERTION]: undefined == 401
      at assertFailsWithStatus (file:///home/circleci/project/test/security.test.js:42:10)
      at async Context.<anonymous> (file:///home/circleci/project/test/security.test.js:86:7)

klcodanr · 2023-06-01T17:00:11Z

Tests are fixed, thanks @jdelbick

codecov · 2023-06-01T17:17:51Z

Codecov Report

Merging #31 (22a4a02) into main (f04f985) will decrease coverage by 0.59%.
The diff coverage is 98.39%.

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
- Coverage   97.14%   96.56%   -0.59%     
==========================================
  Files          11       12       +1     
  Lines        1263     1688     +425     
==========================================
+ Hits         1227     1630     +403     
- Misses         36       58      +22

Impacted Files	Coverage Δ
src/schema-validator.js	`80.23% <90.90%> (-19.77%)`	⬇️
src/security.js	`98.52% <98.52%> (ø)`
src/cloud-search-index-storage.js	`100.00% <100.00%> (ø)`
src/context.js	`100.00% <100.00%> (ø)`
src/index.js	`100.00% <100.00%> (ø)`
src/router.js	`100.00% <100.00%> (ø)`
src/secret.js	`94.26% <100.00%> (+0.40%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alexkli · 2023-06-02T01:15:55Z

@klcodanr If you don't mind, I would approach it a bit differently. Instead of adding it to our custom Router mechanism, I would like to be more Franklin/helix compatible and plug this in as a wrapper function, like for example helix-shared-body-data.

This wrapper would respond with a 401 or 403 if authN or (basic tenant) authZ fails. If successful, it would set the necessary tenant and user information on the context object, say as context.user and context.space with the various ids and the parsed token (including roles & permissions), so it is easily available to any downstream code. Fine grained authZ can happen in the main function later by looking at the context object - as that might be different depending on what API/resource is being accessed and thus is probably best right in the code that has that information.

This would make it easy to plug this into our function, without also having to depend on the Router and everything that comes with it. And we don't introduce any new dependencies on code as they only need to look at context.* - we did a similar thing in Nui (nui/auth - you can ignore that this is its own serverless function, Nui has its own params payload that is passed through various functions in its execution flow).

klcodanr · 2023-06-02T02:50:47Z

That makes sense as long as the function has the same authorization/authentication requirements for any invocation. The Extractor doesn't as neither the webhook or callback endpoints require any form of auth but I think the Extractor is an anomaly and could be handled by just calling the functions underlying the auth wrapper inside the handler functions.

@jdelbick would you agree with that or are there other cases where having more fine-grained control at the router level would be helpful?

alexkli · 2023-06-05T11:54:40Z

@klcodanr I am not sure I see the implication - if there is a different authorization requirement, then it's different logic either way - a different wrapper function OR Router "plugin". The wrapper is not forced onto all our functions, you would add it to the index.js file of each lambda that needs it.

Or are you saying we already have lambdas (eg. in the extractor) which handle multiple sub apis/routes (/one, /two, /three) where each might have different authN & authZ requirements? If yes we could make this configurable. Or maybe the (new) library exports a function just for the auth logic that could be used and directly called from anywhere (e.g. inside an api/route like /one) – and separately it exports a wrapper that happens to call this very function, basically to make it easy and convenient to use for (most?) lambdas that need the same authN for all their requests.

klcodanr · 2023-06-05T14:06:23Z

Yes, I'm referring to the second case and I agree with that approach. Make it easier for the 90% and expose a function (or functions) for checking more involved use cases.

Dan Klco added 4 commits May 24, 2023 13:31

feat: adding support for security validation

393eb94

Adding an IT and some cleanup

8a525f7

Updating the router to support security

b8dc563

Fixing issue where secret IT was deleting other test's secrets

2170893

klcodanr requested review from jdelbick and mattvryan May 25, 2023 13:21

github-advanced-security AI found potential problems May 25, 2023

View reviewed changes

jdelbick reviewed May 25, 2023

View reviewed changes

Fixing typo

3334ec8

Dan Klco added 4 commits May 26, 2023 09:34

Adding samples to the docs

d49c2d4

Updated API docs

54d85d4

Moving constant out to top of file

609aed5

Adding negative IT's and fixing a subitle promise issue

f3312cb

github-advanced-security AI found potential problems May 26, 2023

View reviewed changes

Adding instructions for setting up the .env file

a158b80

Dan Klco added 2 commits June 1, 2023 12:41

Merge branch 'main' into request-verification

f1b0901

Fixing broken test due to async fixed

22a4a02

Conversation

klcodanr commented May 25, 2023

Related Issues

Uh oh!

Check failure

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check failure

jdelbick left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

klcodanr May 25, 2023 • 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!

Uh oh!

Uh oh!

github-actions Bot commented May 26, 2023

Uh oh!

Check failure

jdelbick commented May 27, 2023

Uh oh!

klcodanr commented Jun 1, 2023

Uh oh!

codecov Bot commented Jun 1, 2023

Codecov Report

Uh oh!

alexkli commented Jun 2, 2023

Uh oh!

klcodanr commented Jun 2, 2023

Uh oh!

alexkli commented Jun 5, 2023

Uh oh!

klcodanr commented Jun 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jdelbick left a comment •

edited

Loading

klcodanr May 25, 2023 •

edited

Loading