974 koa hapi multi authentication #995
Conversation
|
@technicallyfeasible I changed the approach with handling errors in the controller after authentication. Based on the tests, It seems like its still working to me. If you're still relying on this behavior, I'd appreciate your eyes here. |
Please elaborate. The polyfill would never have to be provided by tsoa('s build), but that's what I think you're saying? |
fantapop
force-pushed
the
974-koa-hapi-multi-authentication
branch
from
bf331ae
to
29fae38
Compare
I was assuming we wouldn't want to put the onus of including the polyfill on a user of tsoa so we'd either want to include it in the runtime as a dependency and load it in the routes or inline an implementation in the routes file itself. I wasn't really sure which to do but after you mentioned this I did some research and found that the polyfill is only a few lines of code so I added it directly to the routes template. After this, various bugs and misunderstandings of what the code was currently doing, led me to do a bit of a refactor of the authentication code. I added some new tests and kept all the rest of course. I think Its a bit clearer now and I was able to bring the complicated part of the implementation back into sync across each server routes file. Let me know what you think. |
thanks for checking, will give this a go and get back to you |
When would any transpiler + tsconfig that someone could currently be using that takes the routeTemplate and transforms it to JS not take care of that? |
In our service that uses TSOA, if i try to use
|
Yes, if we use Promise.any without fallback. I am not really familiar with the Hapi template, maybe @icopp can check that? |
| request['user'] = await promiseAny(secMethodOrPromises); | ||
| next(); | ||
| } | ||
| catch(err) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
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:
- bearer token authentication
- api key
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.
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.
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.
still need some code outside of the framework to determine which one is the correct one to show the user
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.
The current templates would return the error saying "missing X-Api-Key Header" since it was last to fail.
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.
TS-ignore them or cast to explicit any
There was a problem hiding this comment.
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.
Can you elaborate?
import * as promiseAny from 'promise.any'
should be fine.
There was a problem hiding this comment.
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.
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
import { implementation } from ''promise.any;
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.
fantapop
force-pushed
the
974-koa-hapi-multi-authentication
branch
from
70c7e07
to
4ae2d7a
Compare
fantapop
force-pushed
the
974-koa-hapi-multi-authentication
branch
from
4ae2d7a
to
f0ef4c0
Compare
changed APIkey to ApiKey
fantapop
force-pushed
the
974-koa-hapi-multi-authentication
branch
from
f0ef4c0
to
0a5e35b
Compare
As part of this refactored how calls to the authentication method for each server type are handled. Express multi auth was not previously broken but I was able to share the same solution across all three servers and isolate the server specific handling to a single block of code after waiting for the appropriate promises to be resolved. I additionally added tests for the following: - security AND / OR tests for hapi/koa - a slow failure test to show that promiseAny returns with first success - checks for which error is resolved to the user in the case that there is more than one Closes lukeautry#974
fantapop
force-pushed
the
974-koa-hapi-multi-authentication
branch
from
ba13a5b
to
3b0ab3b
Compare
| } | ||
| catch(err) { | ||
| // Show most recent error as response | ||
| const error = failedAttempts.pop(); |
There was a problem hiding this comment.
Instead of manual tracking, err.errors.pop()
There was a problem hiding this comment.
Here's the combined diff
diff --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.
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.
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 TsoaAuthenticationError that extends AggregateError for v4, I think this has to be the long-term plan.
However, I still can't reproduce the tests failing with import * as. Any hints there?
Anything else LGTM as is.
All Submissions:
Potential Problems With The Approach
Test plan
I've ported the AND / OR Security method tests from the express tests to Hapi / Koa. Everything seems to be working from what I can tell. I've also tested locally with my Koa server to verify the intitial issue I saw is fixed.