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

[BUG] TypeScript Strategy is incompatible with Passport Strategy #549

Closed
fluggo opened this issue Mar 4, 2021 · 14 comments
Closed

[BUG] TypeScript Strategy is incompatible with Passport Strategy #549

fluggo opened this issue Mar 4, 2021 · 14 comments

Comments

@fluggo
Copy link

fluggo commented Mar 4, 2021

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

passport.use(new passportSaml.Strategy({} as any, (profile: any, done: passportSaml.VerifiedCallback) => {
  done(null, profile, {});
});

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

  • Node.js version: 14.15.4
  • TypeScript version: 4.1.5
  • passport-saml version: 2.0.5
@fluggo fluggo added the bug label Mar 4, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Mar 4, 2021

@fluggo What docs suggest this? I'm not sure what you're trying to accomplish here. Also, it seems you're missing a ), as there aren't enough, so the syntax isn't correct. Sorry if I'm missing something obvious.

@fluggo
Copy link
Author

fluggo commented Mar 5, 2021

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:

image

This is with @types/[email protected] and [email protected].

@cjbarth cjbarth modified the milestones: 2.1.1, 3.0.0 Apr 5, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

@fluggo I still can't seem to duplicate this problem. I've checked out passport-saml at v2.0.5 and npm install --save-dev @types/[email protected] and then put the following code in passport-saml/src/test.ts with the following code:

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);
        });
      }
    }
  )
);

@cjbarth cjbarth added need-more-info and removed bug labels Apr 5, 2021
@cjbarth cjbarth removed this from the 3.0.0 milestone Apr 5, 2021
@fluggo
Copy link
Author

fluggo commented Apr 5, 2021

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.

@fluggo
Copy link
Author

fluggo commented Apr 7, 2021

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.

@fluggo
Copy link
Author

fluggo commented Apr 7, 2021

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?

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 7, 2021

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.

@cjbarth
Copy link
Collaborator

cjbarth commented May 17, 2021

A significant refactor has taken place for the 3.0.0 release. If this issue persists in that release, please reopen this issue.

@cjbarth cjbarth closed this as completed May 17, 2021
@bvds
Copy link

bvds commented Nov 9, 2022

I am seeing the exact same error with [email protected] and [email protected]
Screenshot from 2022-11-09 11-12-57
Please re-open this issue.

@bvds
Copy link

bvds commented Nov 9, 2022

Also, the two examples in the post above #549 (comment) appear to be identical. Am I missing something?

@jbinto
Copy link

jbinto commented Nov 10, 2022

We're starting to see this after upgrading from [email protected] to @node-saml/[email protected]. [email protected] remained unchanged.

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 where we patch express.Request and express.User:

// 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
    }
  }
}

@fluggo
Copy link
Author

fluggo commented Nov 10, 2022

Also, the two examples in the post above #549 (comment) appear to be identical. Am I missing something?

Yes. Look closely at the interface C declaration.

@cjbarth
Copy link
Collaborator

cjbarth commented Nov 12, 2022

We're starting to see this after upgrading from [email protected] to @node-saml/[email protected]. [email protected] remained unchanged.

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.

@bvds
Copy link

bvds commented Nov 14, 2022

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 Request.user which has nothing to do with SAML:

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;
  }
}

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