-
Notifications
You must be signed in to change notification settings - Fork 475
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
[BUG] TypeScript Strategy is incompatible with Passport Strategy #549
Comments
@fluggo What docs suggest this? I'm not sure what you're trying to accomplish here. Also, it seems you're missing a |
Ah, you're right about the parentheses. Sorry, I was in a hurry, and was adapting my code. Let's try with a complete TypeScript file, this time directly from the main readme: import * as passport from 'passport';
import { Strategy as SamlStrategy } from 'passport-saml';
passport.use(new SamlStrategy(
{
path: '/login/callback',
entryPoint: 'https://openidp.feide.no/simplesaml/saml2/idp/SSOService.php',
issuer: 'passport-saml'
},
function(profile, done) {
findByEmail(profile.email, function(err, user) {
if (err) {
return done(err);
}
return done(null, user);
});
})
); TypeScript gives this error: This is with @types/[email protected] and [email protected]. |
@fluggo I still can't seem to duplicate this problem. I've checked out import * as passport from "passport";
import { Strategy as SamlStrategy } from "./passport-saml";
import { Profile, VerifiedCallback } from "./passport-saml/types";
function findByEmail(email: string, cb: VerifiedCallback) {
cb(null);
}
passport.use(
new SamlStrategy(
{
path: "/login/callback",
entryPoint:
"https://openidp.feide.no/simplesaml/saml2/idp/SSOService.php",
issuer: "passport-saml",
},
function (profile: Profile | null | undefined, done: VerifiedCallback) {
if (profile != null && typeof profile.email === "string") {
findByEmail(profile.email, function (err, user) {
if (err) {
return done(err);
}
return done(null, user);
});
}
}
)
); |
Hmm. I can't explain it. I have one project where this fails, and a fresh project where this succeeds. I haven't yet figured out what the difference is between the two. I'll have to get back to you. |
Okay, this has to be a bug in TypeScript. Check this out: interface A { thing: string; }
interface B { stuff: string; thing: string; }
interface C {
authenticate(req: A): void;
}
const dd = {
authenticate(req: B): void {
console.log(req.stuff.toLowerCase());
}
}
const cc: C = dd;
cc.authenticate({thing: 'happy'});
interface E {
(req: A): void;
}
type F = C["authenticate"];
type G = (req: A) => void;
const ee: E = (req: B) => 0;
const ff: F = (req: B) => 0;
const gg: G = (req: B) => 0; TypeScript should be calling out errors on the assignments to cc, ee, ff, and gg because B is not assignable to A, but it's only seeing errors on the assignments to ee and gg. This program crashes, by the way, because dd.authenticate() receives an object that's the wrong shape. |
One deep dive later, and I think I'm a little closer to understanding what's going on here—not exactly, but the above assignability behavior comes from strictFunctionTypes, which has the caveat that function arguments (including functions as object properties) are considered contravariant under this flag, but method arguments are considered bivariant. Under bivariance, the passport-saml types should work. Under contravariance, they should not. In other words, this passes under strictFunctionTypes: interface A { thing: string; }
interface B { stuff: string; thing: string; }
interface C {
authenticate(req: A): void;
}
const dd = {
authenticate(req: B): void {
console.log(req.stuff.toLowerCase());
}
}
const cc: C = dd; This does not: interface A { thing: string; }
interface B { stuff: string; thing: string; }
interface C {
authenticate: (req: A): void;
}
const dd = {
authenticate(req: B): void {
console.log(req.stuff.toLowerCase());
}
}
const cc: C = dd; Now the mystery that's left—what about our code is making TypeScript see one situation one way, and the other a different way? |
That is some good sleuthing! Having said that, it seems the problem is related to the types that Passport presents for a Strategy. I'd love to hear what you find though. I'll keep this open until you find a resolution, because I'm sure others will appreciate your good work here. |
A significant refactor has taken place for the 3.0.0 release. If this issue persists in that release, please reopen this issue. |
I am seeing the exact same error with [email protected] and [email protected] |
Also, the two examples in the post above #549 (comment) appear to be identical. Am I missing something? |
We're starting to see this after upgrading from I've spent a long time debugging so I have lots of theories but also a lot of dead ends, so I don't want to muddy the waters too much. Some things I've observed which may or may not be a factor:
Update: I was able to get it to typecheck, this isn't ideal but I was determined to get our app to build with 4.x, hopefully a better way emerges. We had to add some passport-saml specific types to our already existing // global.d.ts
import { Profile } from '@node-saml/node-saml'
declare global {
namespace Express {
export interface Request {
// [REDACTED: lots of properties we already had defined]
// Patch express.Request to match what @node-saml/passport-saml expects
// (Monitor https://github.com/node-saml/passport-saml/issues/549 to see if this improves)
samlLogoutRequest?: Profile
}
export interface User {
// [REDACTED: lots of properties we already had defined]
// Patch express.User to match what @node-saml/passport-saml expects
// (Monitor https://github.com/node-saml/passport-saml/issues/549 to see if this improves)
[key: string]: unknown
}
}
} |
Yes. Look closely at the |
Since so much has changed since this bug was opened, let's create a new one instead of reopening this one. I still haven't seen problems with the types, but that doesn't mean there aren't any. |
Here is a work-around that works for me, even for cases where SAML is not used. In my case, I have an existing modification to import { Profile } from 'passport-saml';
export interface CustomUser {
// an object with various identifiers, not shown here
}
declare module 'express-serve-static-core' {
interface Request {
user: CustomUser & Profile;
samlLogoutRequest: Profile;
}
} |
Small bug: When using the types from the latest passport-saml, a new Strategy object can't be used in a passport.use() call.
To Reproduce
Expected behavior
The docs suggest this very usage, so it should pass TypeScript checks without errors. Unfortunately, the strategy's authenticate() method requires a samlLogoutRequest property to appear on the request object, and since express doesn't provide that, it fails.
Environment
The text was updated successfully, but these errors were encountered: