-
Notifications
You must be signed in to change notification settings - Fork 475
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
974 koa hapi multi authentication #995
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ import { {{name}} } from '{{modulePath}}'; | |
{{/each}} | ||
{{#if authenticationModule}} | ||
import { expressAuthentication } from '{{authenticationModule}}'; | ||
// @ts-ignore - no great way to install types from subpackage | ||
const promiseAny = require('promise.any'); | ||
{{/if}} | ||
{{#if iocModule}} | ||
import { iocContainer } from '{{iocModule}}'; | ||
|
@@ -104,51 +106,63 @@ export function RegisterRoutes(app: express.Router) { | |
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
|
||
{{#if useSecurity}} | ||
function authenticateMiddleware(security: TsoaRoute.Security[] = []) { | ||
return function runAuthenticationMiddleware(request: any, _response: any, next: any) { | ||
let responded = 0; | ||
let success = false; | ||
|
||
const succeed = function(user: any) { | ||
if (!success) { | ||
success = true; | ||
responded++; | ||
request['user'] = user; | ||
next(); | ||
} | ||
} | ||
|
||
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
|
||
const fail = function(error: any) { | ||
responded++; | ||
if (responded == security.length && !success) { | ||
error.status = error.status || 401; | ||
next(error) | ||
} | ||
} | ||
function authenticateMiddleware(security: TsoaRoute.Security[] = []) { | ||
return async function runAuthenticationMiddleware(request: any, _response: any, next: any) { | ||
|
||
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
|
||
// keep track of failed auth attempts so we can hand back the most | ||
// recent one. This behavior was previously existing so preserving it | ||
// here | ||
const failedAttempts: any[] = []; | ||
const pushAndRethrow = (error: any) => { | ||
failedAttempts.push(error); | ||
throw error; | ||
}; | ||
|
||
const secMethodOrPromises: Promise<any>[] = []; | ||
for (const secMethod of security) { | ||
if (Object.keys(secMethod).length > 1) { | ||
let promises: Promise<any>[] = []; | ||
const secMethodAndPromises: Promise<any>[] = []; | ||
|
||
for (const name in secMethod) { | ||
promises.push(expressAuthentication(request, name, secMethod[name])); | ||
secMethodAndPromises.push( | ||
expressAuthentication(request, name, secMethod[name]) | ||
.catch(pushAndRethrow) | ||
); | ||
} | ||
|
||
Promise.all(promises) | ||
.then((users) => { succeed(users[0]); }) | ||
.catch(fail); | ||
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
|
||
secMethodOrPromises.push(Promise.all(secMethodAndPromises) | ||
.then(users => { return users[0]; })); | ||
} else { | ||
for (const name in secMethod) { | ||
expressAuthentication(request, name, secMethod[name]) | ||
.then(succeed) | ||
.catch(fail); | ||
secMethodOrPromises.push( | ||
expressAuthentication(request, name, secMethod[name]) | ||
.catch(pushAndRethrow) | ||
); | ||
} | ||
} | ||
} | ||
|
||
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
|
||
try { | ||
request['user'] = await promiseAny(secMethodOrPromises); | ||
next(); | ||
} | ||
catch(err) { | ||
// Show most recent error as response | ||
const error = failedAttempts.pop(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of manual tracking, err.errors.pop() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's the combined diffdiff --git a/packages/cli/src/routeGeneration/templates/express.hbs b/packages/cli/src/routeGeneration/templates/express.hbs
index 222b28b..1ae0354 100644
--- a/packages/cli/src/routeGeneration/templates/express.hbs
+++ b/packages/cli/src/routeGeneration/templates/express.hbs
@@ -9,7 +9,7 @@ import { {{name}} } from '{{modulePath}}';
{{#if authenticationModule}}
import { expressAuthentication } from '{{authenticationModule}}';
// @ts-ignore - no great way to install types from subpackage
-const promiseAny = require('promise.any');
+import * as promiseAny from 'promise.any';
{{/if}}
{{#if iocModule}}
import { iocContainer } from '{{iocModule}}';
@@ -114,15 +114,6 @@ export function RegisterRoutes(app: express.Router) {
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa
- // keep track of failed auth attempts so we can hand back the most
- // recent one. This behavior was previously existing so preserving it
- // here
- const failedAttempts: any[] = [];
- const pushAndRethrow = (error: any) => {
- failedAttempts.push(error);
- throw error;
- };
-
const secMethodOrPromises: Promise<any>[] = [];
for (const secMethod of security) {
if (Object.keys(secMethod).length > 1) {
@@ -131,7 +122,6 @@ export function RegisterRoutes(app: express.Router) {
for (const name in secMethod) {
secMethodAndPromises.push(
expressAuthentication(request, name, secMethod[name])
- .catch(pushAndRethrow)
);
}
@@ -143,7 +133,6 @@ export function RegisterRoutes(app: express.Router) {
for (const name in secMethod) {
secMethodOrPromises.push(
expressAuthentication(request, name, secMethod[name])
- .catch(pushAndRethrow)
);
}
}
@@ -157,7 +146,7 @@ export function RegisterRoutes(app: express.Router) {
}
catch(err) {
// Show most recent error as response
- const error = failedAttempts.pop();
+ const error = err.errors.pop();
error.status = error.status || 401;
next(error);
}
diff --git a/packages/cli/src/routeGeneration/templates/hapi.hbs b/packages/cli/src/routeGeneration/templates/hapi.hbs
index ddb3251..c1822b6 100644
--- a/packages/cli/src/routeGeneration/templates/hapi.hbs
+++ b/packages/cli/src/routeGeneration/templates/hapi.hbs
@@ -9,7 +9,7 @@ import { {{name}} } from '{{modulePath}}';
{{#if authenticationModule}}
import { hapiAuthentication } from '{{authenticationModule}}';
// @ts-ignore - no great way to install types from subpackage
-const promiseAny = require('promise.any');
+import * as promiseAny from 'promise.any';
{{/if}}
{{#if iocModule}}
import { iocContainer } from '{{iocModule}}';
@@ -145,15 +145,6 @@ export function RegisterRoutes(server: any) {
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa
- // keep track of failed auth attempts so we can hand back the most
- // recent one. This behavior was previously existing so preserving it
- // here
- const failedAttempts: any[] = [];
- const pushAndRethrow = (error: any) => {
- failedAttempts.push(error);
- throw error;
- };
-
const secMethodOrPromises: Promise<any>[] = [];
for (const secMethod of security) {
if (Object.keys(secMethod).length > 1) {
@@ -162,7 +153,6 @@ export function RegisterRoutes(server: any) {
for (const name in secMethod) {
secMethodAndPromises.push(
hapiAuthentication(request, name, secMethod[name])
- .catch(pushAndRethrow)
);
}
@@ -174,7 +164,6 @@ export function RegisterRoutes(server: any) {
for (const name in secMethod) {
secMethodOrPromises.push(
hapiAuthentication(request, name, secMethod[name])
- .catch(pushAndRethrow)
);
}
}
@@ -188,7 +177,7 @@ export function RegisterRoutes(server: any) {
}
catch(err) {
// Show most recent error as response
- const error = failedAttempts.pop();
+ const error = err.errors.pop();
if (isBoom(error)) {
throw error;
}
diff --git a/packages/cli/src/routeGeneration/templates/koa.hbs b/packages/cli/src/routeGeneration/templates/koa.hbs
index 5147456..4855634 100644
--- a/packages/cli/src/routeGeneration/templates/koa.hbs
+++ b/packages/cli/src/routeGeneration/templates/koa.hbs
@@ -13,7 +13,7 @@ import { {{name}} } from '{{modulePath}}';
{{#if authenticationModule}}
import { koaAuthentication } from '{{authenticationModule}}';
// @ts-ignore - no great way to install types from subpackage
-const promiseAny = require('promise.any');
+import * as promiseAny from 'promise.any';
{{/if}}
{{#if iocModule}}
import { iocContainer } from '{{iocModule}}';
@@ -114,15 +114,6 @@ export function RegisterRoutes(router: KoaRouter) {
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa
- // keep track of failed auth attempts so we can hand back the most
- // recent one. This behavior was previously existing so preserving it
- // here
- const failedAttempts: any[] = [];
- const pushAndRethrow = (error: any) => {
- failedAttempts.push(error);
- throw error;
- };
-
const secMethodOrPromises: Promise<any>[] = [];
for (const secMethod of security) {
if (Object.keys(secMethod).length > 1) {
@@ -131,7 +122,6 @@ export function RegisterRoutes(router: KoaRouter) {
for (const name in secMethod) {
secMethodAndPromises.push(
koaAuthentication(context.request, name, secMethod[name])
- .catch(pushAndRethrow)
);
}
@@ -141,7 +131,6 @@ export function RegisterRoutes(router: KoaRouter) {
for (const name in secMethod) {
secMethodOrPromises.push(
koaAuthentication(context.request, name, secMethod[name])
- .catch(pushAndRethrow)
);
}
}
@@ -157,7 +146,7 @@ export function RegisterRoutes(router: KoaRouter) {
}
catch(err) {
// Show most recent error as response
- const error = failedAttempts.pop();
+ const error = err.errors.pop();
context.status = error.status || 401;
context.throw(context.status, error.message, error);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we pop the error off of the aggregated error list, I believe this will always be the error from the last auth decorator. The failedAttempts array tracks the rejections in the order that they failed rather than the order they were specified. I do like the simplicity of not worrying about that but it is a breaking change and I've always thought the behavior tended to choose the "right" error to report on in most cases. I could be wrong though. Since you're getting ready to rev the major we could make that change as part of that assuming we also get something in to control the order in a timely manner. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems about right, my brain actively tries to block that out as it's just not how I would want this to (have) work(ed). I'll try to build on your PR and propagate a Anything else LGTM as is. |
||
error.status = error.status || 401; | ||
next(error); | ||
} | ||
|
||
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
} | ||
} | ||
{{/if}} | ||
|
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.
Looking at #659 here.
The Promise.any would throw an AggregateError here, which would be great for determining what the respective issues were.
However the current Polyfill doesn't do that. Wdyt about bringing in a spec compliant shim? I'm looking at es-shims here for example.
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 would be a breaking change, but one that makes sense and allows us to eventually remove witout headaches.
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.
I'm all for bringing in a spec compliant shim. It would utilize shared code better by using a single import and it would be easy to pull it out later once Promise.any becomes commonplace.
I'm not convinced that we'd want to return the aggregate error here however. If we returned the AggregateError we'd still need some code outside of the framework to determine which one is the correct one to show the user. It feels like integrating that into the authorization code is the way to go since it already exists and its a singular spot to contain crucial details that may not be passed along in the AggregateError. Here's one example.
say we're using two kinds of auth:
A user with a bearer token which is a JWT attempts to authenticate but the token is expired
the code checks bearer token auth and throws an Error saying "Token Expired"
the code then checks the api key auth and see's that there is no X-Api-Key Header at all so throws an error saying "Missing X-Api-Key Header".
The current templates would return the error saying "missing X-Api-Key Header" since it was last to fail. Inside the authorization code we've just checked for the X-Api-Key header, seen that its missing and we have a pretty good idea that this type of authentication is not the intended one. If we sent both errors inside of an AggregateError we'd still need to do a follow up check on the request object to see that X-Api-Key header is missing. This bit of code would probably need to do a lot of little checks which would end up looking like the authorization code path already does.
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.
I did push a fix up to show what the changes look like to use the polyfill instead. I ended up using the imported method directly instead of executing any.shim() because typescript still didn't know about
Promise.any
after the shim ran so it would still involve extra changes to suppress this.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.
The imported method is perfectly fine.
For your example I'd argue it should be up to the developer to decide which error to show.
We already pass down the error, so this would not change. However, we could let the developer decide which Error to show if multiple errors occurred (in your example, we might end up passing down the Error for 2) when the token is expired.
Which is obviously what I'd want to prevent. Expired Token is the correct issue to face, wouldn't you agree?
Instead of one error you can have an AggregateError you can use to derive what the issues were.
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.
TS-ignore them or cast to explicit any
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.
okay, ts-ignore works well for the lack of types. I ended up having to use the require for this shim because i couldn't get the tests working without it. Not sure why.
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.
Can you elaborate?
should be fine.
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.
I retried again and it seems to be working fine. I think my environment was slightly messed up. I'll push that change back up.
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.
actually it doesn't work. This version builds and passes the tests but when I test it with my project I get this error:
err: TypeError: promiseAny is not a function
I looked at the promise.any code and its doing something fancy to make the import work as a function an object. I'm wondering if this is causing it to break using this syntax. I'm unclear why its not breaking for the tests. Maybe I've got my project configured to handle the imports differently?
here is the source:
https://github.com/es-shims/Promise.any/blob/master/index.js
I do recall now testing this and trying to get it working by importing the implementation directly for example
but no dice.
I also noted previously that require syntax is already being used in some of the templates which is why I went ahead and opted for that, besides it being the only one I could get working.