Upgrade to Express 5 (along with some other minor dependency updates) #347
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson! See below for a few thoughts and suggestions.
api/src/services/Authentication.ts
Outdated
| } else if (authStrategy === "saml") { | ||
| const samlStrategy = this.getSamlStrategy(); | ||
| passport.use(samlStrategy); | ||
| passport.use(samlStrategy as unknown as passport.Strategy); |
There was a problem hiding this comment.
| passport.use(samlStrategy as unknown as passport.Strategy); | |
| passport.use(samlStrategy); |
Do we actually need this change? samlStrategy should always be a subclass of passport.Strategy, so I don't think there's a need to recast it. When I remove this, tests and lint still pass for me, and the build still builds.
There was a problem hiding this comment.
Argument of type 'import("/opt/vudl/api/node_modules/@node-saml/passport-saml/lib/strategy").Strategy' is not assignable to parameter of type 'import("/opt/vudl/api/node_modules/@types/passport/index").Strategy'.
Types of property 'authenticate' are incompatible.
Type '(req: Request<ParamsDictionary, any, any, ParsedQs, Record<string, any>>, options: AuthenticateOptions) => void' is not assignable to type '(this: StrategyCreated<Strategy, Strategy & StrategyCreatedStatic>, req: Request<ParamsDictionary, any, any, ParsedQs, Record<string, any>>, options?: any) => any'.
Types of parameters 'req' and 'req' are incompatible.
Property 'param' is missing in type 'import("/opt/vudl/api/node_modules/@types/express/index").Request<import("/opt/vudl/api/node_modules/@types/express-serve-static-core/index").ParamsDictionary, any, any, qs.ParsedQs, Record<string, any>>' but required in type 'import("/opt/vudl/api/node_modules/@node-saml/passport-saml/node_modules/@types/express/index").Request<import("/opt/vudl/api/node_modules/@node-saml/passport-saml/node_modules/@types/express-serve-static-core/index").ParamsDictionary, any, any, qs.ParsedQs, Record<...>>'.ts(2345)
api/src/services/Authentication.ts
Outdated
| }.bind(this), | ||
| // TODO: implement a logout function here: | ||
| () => null, | ||
| done(null, user ? (user as unknown as Record<string, unknown>) : undefined); |
There was a problem hiding this comment.
I think I prefer using null here instead of undefined since we're explicitly saying "there is no valid user" rather than "we haven't defined a user yet." But maybe I'm splitting hairs. If nothing else, it's shorter. ;-)
| done(null, user ? (user as unknown as Record<string, unknown>) : undefined); | |
| done(null, user ? (user as unknown as Record<string, unknown>) : null); |
demiankatz
changed the title
|
Superseded by #350. |
2 participants
Updating the @types/express dependency to the current version required making changes to authentication.ts
getSamlStrategy had mixed verify-signature forms, so TypeScript picked an overload that caused the library to pass different arguments at runtime — the done parameter ended up being a non-function, producing the "done is not callable" error. Making both callbacks the same form, giving explicit types, and removing the invalid placeholder ensured that overload resolution, runtime argument order, and TypeScript types all lined up.