make RequestWithUser.samlLogoutRequest optional

[clhuang]

Description

whenever I try to do passport.use('strategyName', new SamlStrategy(...)) I get the following error:

Argument of type 'import("/Users/calvinhuang/scale/scaleapi3/server/node_modules/passport-saml/lib/passport-saml/strategy").Strategy' is not assignable to parameter of type 'import("/Users/calvinhuang/scale/scaleapi3/server/node_modules/@types/passport/index").Strategy'
  Types of property 'authenticate' are incompatible.
    Type '(req: RequestWithUser, 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 'samlLogoutRequest' is missing in type 'Request<ParamsDictionary, any, any, ParsedQs, Record<string, any>>' but required in type 'RequestWithUser'.

It doesn't make sense to me that samlLogoutRequest needs to be present on every authentication request, and removing this would allow it to typecheck properly.

I'm using @types/passport 1.0.7.

Checklist:

• Issue Addressed: [x]
• Link to SAML spec: [ ]
• Tests included? [ ]
• Documentation updated? [ ]

[srd90]

Commenting just this part of PR description:

It doesn't make sense to me that samlLogoutRequest needs to be present on every authentication request,

passport-saml has leaky/vulnerable SAML SLO implementation.

It is used by propagating stuff from callback interfaces (authn and slo callback) to authenticatefunction, which is counter-intuitive but thats just the way it has been implemented back in the days. For further infromation see e.g. #221 (comment) and #419.

There was some sort of "half-way" fix for it (see node-saml/node-saml#10 and #619 ) which forces users of passport-saml library to take some/explicit responsibility of SLO scenarios and/or force users of passpor-saml library think of SLO instead of assuming that passport-saml would just work out of the box (which it doesn't...it just gave false sense of security and if someone used passport-saml's SAML SLO implementation and did not bother to test it he/she could have made users of his/her service vulnerable to local session hijack).

[cjbarth]

I would further like to note that passport-saml includes its own types, so you don't need to install other types.

[clhuang]

Yeah, this is in regards to the passport-saml included types.

Is there something I'm missing in order to have the following code typecheck properly?

passport.use(new SamlStrategy(...))

[clhuang]

this only seems to happen when there is a declaration for the express.User type somewhere in the project, e.g. custom.d.ts:

namespace Express {
  interface User {
    ...
  }
}

[clhuang]

so in the rest of the app, we use req.user differently than how passport-saml does it, this appears to be what is causing the type issues. The samlLogoutRequest thing appears to be a red herring.

[jbinto]

@clhuang Any chance you can share more about how you resolved this? We are just starting to see this error after upgrading from 3.2.1 to 4.0.1, and we also have modified express's express.User type.

edit: Was able to get it to typecheck, not pretty but look at #549 (comment)

[jbinto]

Linking #549 to this issue.