Add checkToken back to addAppServicePath and ensure metrics doesn't require a token #435
Conversation
src/bridge.ts
Outdated
| @@ -967,6 +967,7 @@ export class Bridge { | |||
| public addAppServicePath(opts: { | |||
| method: "GET"|"PUT"|"POST"|"DELETE", | |||
| path: string, | |||
| checkToken?: boolean, | |||
There was a problem hiding this comment.
| checkToken?: boolean, | |
| checkToken: boolean = true, |
Given the checkToken === false, this defaults to true.
Let's make that explicit in the signature.
I'm a fan of defaulting to false. Have you considered ignoreToken as a variable name?
There was a problem hiding this comment.
Hm, maybe we should just have authenticate to be properly explicit. checkToken was used as the legacy variable name, but since that was removed in 5.0.0 and we have typings, I don't think we need to follow it.
There was a problem hiding this comment.
Oh I should have mentioned that I couldn't do a explicit signature value because it's an opts object type. I have however been explicit in the code below.
On the naming, on the whole I think that having a positive variable name like authenticate is probably a better look than ignoreToken because negative variable names are usually bad. I do concede that having default-to-true variables are a bit spooky, I hope people read the docstring :)
| handler: (req: ExRequest, respose: ExResponse, next: NextFunction) => void, | ||
| }): void { | ||
| if (!this.appservice) { | ||
| throw Error('Cannot call addAppServicePath before calling .run()'); | ||
| } | ||
| const app: Application = this.appservice.expressApp; | ||
|
|
||
| const authenticate = opts.authenticate ?? true; |
There was a problem hiding this comment.
| const authenticate = opts.authenticate ?? true; | |
| authenticate = opts.authenticate ?? true; |
Is re-declaring a parameter on the top level of a function valid in JavaScript?
I'm getting SyntaxError: Identifier 'hi' has already been declared in a local test.
Is the linter dead?
There was a problem hiding this comment.
Should be fine? It's an opts object, not a parameter. (Which is why I can't do defaults)
This was broken in a previous release where we suddenly enabled authentication forcefully across all our endpoints, which broke bridges that used it for unauthenticated metrics access.