Skip to content
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

1.x.x to 2.x.x migration type issue #540

Closed
vbodnia opened this issue Feb 16, 2021 · 10 comments
Closed

1.x.x to 2.x.x migration type issue #540

vbodnia opened this issue Feb 16, 2021 · 10 comments

Comments

@vbodnia
Copy link

vbodnia commented Feb 16, 2021

Env: Node 14.15.4
Typescript 4.1.3

Currently, I am using passport-saml@1.5.0 with @types/passport-saml@1.1.2. After updating to 2.0.5 I've got a type error in MultiSamlStrategy saying:

TS2345: Argument of type 'MultiSamlStrategy' is not assignable to parameter of type 'Strategy'. Types of property 'authenticate' are incompatible. Type '(req: RequestWithUser, options: AuthenticateOptions & AuthorizeOptions) => 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'.

The problem is that I'm also extending express requests with custom fields, e.g. user.

@vbodnia vbodnia added the bug label Feb 16, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Feb 16, 2021

The latest build of passport-saml includes its own types and doesn't need anything from DefinitelyTyped.

@cjbarth cjbarth closed this as completed Feb 16, 2021
@cjbarth cjbarth removed the bug label Feb 16, 2021
@vbodnia
Copy link
Author

vbodnia commented Feb 21, 2021

@cjbarth In fact, the same issue happens without @types/passport-saml package. I have reinstalled modules with the version 2.0.5 and got the same error.

@Mikescops
Copy link

I confirm what @vbodnia says, please reopen this issue, it's a current bug in v2.0.5.

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

Is this a bug at the HEAD of master?

@vbodnia
Copy link
Author

vbodnia commented Apr 6, 2021

@cjbarth Yes it is:

image
image
image

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 6, 2021

It seems this might be related to #549. Would you agree?

In any case, I can't see from your screen shot exactly what the problem is. Do you have some sample code I can load up?

@fluggo
Copy link

fluggo commented Apr 7, 2021

The screen shot doesn't show it, but the original description is very similar to mine. I haven't gone back with tweezers yet to figure out where mine went wrong.

@vbodnia
Copy link
Author

vbodnia commented Apr 7, 2021

@cjbarth

  1. Version 2.0.5 and 2.1.0, definitely typed pkg removed. Gives the error of incompatible req type because we are extending express request with custom fields, as well as it is done in the passport-saml.
    Please see screenshots below:

image
image

  1. I have tried to install pkg from master branch, but the error still persists.

@Mikescops
Copy link

I think the fix #554 by @forty solves this issue but it should be backported to 2.x

@cjbarth
Copy link
Collaborator

cjbarth commented May 17, 2021

v3 of passport-saml has been released. If there is a PR to backport #554 to 2.x, we'll look at it there.

@cjbarth cjbarth closed this as completed May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants