-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support for request verification #31
base: main
Are you sure you want to change the base?
Conversation
test/router.test.js
Dismissed
const response = await router.handle( | ||
new Request('https://localhost/', { | ||
headers: { | ||
authorization: 'Bearer yourbearerhere', |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
authorization header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-_-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly questions, but so far LGTM
| spaceId | <code>string</code> | the space for which the token will be generated | | ||
| roleKeys | <code>Array.<string></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) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qs: why isn't the companyId needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const spaceId = this.#getRequiredHeader(req, 'x-space-id'); | ||
const payload = jwt.decode(token); | ||
|
||
// first validate that the tenant is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qs: is there a way to validate the company id is a valid company id too/first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. cc/ @alexkli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* @param {Array<string>} roleIds | ||
* @param {number | undefined} expiresInMinutes | ||
*/ | ||
async #generateAccessToken( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
test/router.test.js
Dismissed
const response = await router.handle( | ||
new Request('https://localhost/', { | ||
headers: { | ||
authorization: 'Bearer yourbearerhere', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-_-
This PR will trigger a minor release when merged. |
fyi 2 of the security tests are failing:
|
Tests are fixed, thanks @jdelbick |
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@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 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 |
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? |
@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. |
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. |
Please ensure your pull request adheres to the following guidelines:
Related Issues
https://trello.com/c/2A4zpMbE/250-create-commons-library-for-creating-verifying-frontegg-backend-tokens
Notes:
[scope:test,prod]/group:companyId,shared]/[application]/[key]
Thanks for contributing!