Check user matches logout request before reporting logout success

src/strategy.ts

@@ -60,11 +60,6 @@ export abstract class AbstractStrategy extends PassportStrategy {
     }) => {
       if (loggedOut) {
         if (profile != null) {
-          if (this._saml == null) {
-            throw new Error("Can't get logout response URL without a SAML provider defined.");
-          }
-
-          const getLogoutResponseUrl = this._saml.getLogoutResponseUrl;
           // When logging out a user, use the consumer's `validate` function to check that
           // the `profile` associated with the logout request resolves to the same user
           // as the `profile` associated with the current session.
@@ -89,16 +84,22 @@ export abstract class AbstractStrategy extends PassportStrategy {
             }
 
             const RelayState = req.query?.RelayState || req.body?.RelayState;
-            getLogoutResponseUrl(profile, RelayState, options, userMatch, redirectIfSuccess);
-
-            // Log out the current user no matter if we can verify the logged in user === logout requested user
-            req.logout();
-
-            if (!userMatch) {
+            if (this._saml == null) {
               return this.error(
-                new Error("Attempting to log out a user that differs from the current user.")
+                new Error("Can't get logout response URL without a SAML provider defined.")
+              );
+            } else {
+              this._saml.getLogoutResponseUrl(
+                profile,
+                RelayState,
+                options,
+                userMatch,
+                redirectIfSuccess
               );
             }
+
+            // Log out the current user no matter if we can verify the logged in user === logout requested user
+            req.logout();
           };
 
           if (this._passReqToCallback) {

src/types.ts

@@ -31,14 +31,11 @@ export type VerifiedCallback = (
 
 export type VerifyWithRequest = (
   req: express.Request,
-  profile: Profile | null | undefined,
+  profile: Profile | null,
   done: VerifiedCallback
 ) => void;
 
-export type VerifyWithoutRequest = (
-  profile: Profile | null | undefined,
-  done: VerifiedCallback
-) => void;
+export type VerifyWithoutRequest = (profile: Profile | null, done: VerifiedCallback) => void;
 
 export type StrategyOptionsCallback = (err: Error | null, samlOptions?: SamlConfig) => void;
 

test/capturedSamlRequests.spec.ts

@@ -925,7 +925,7 @@ describe("captured SAML requests /", function () {
         config.entryPoint = "https://wwwexampleIdp.com/saml";
         let profile: Profile;
         const strategy = new SamlStrategy(config, function (
-          _profile: Profile | null | undefined,
+          _profile: Profile | null,
           done: VerifiedCallback
         ) {
           if (_profile) {
@@ -1030,7 +1030,7 @@ describe("captured SAML requests /", function () {
       config.entryPoint = "https://wwwexampleIdp.com/saml";
       let profile: Profile;
       const strategy = new SamlStrategy(config, function (
-        _profile: Profile | null | undefined,
+        _profile: Profile | null,
         done: VerifiedCallback
       ) {
         if (_profile) {

test/capturedSamlResponses.spec.ts

@@ -110,10 +110,7 @@ describe("captured saml responses /", function () {
       config.callbackUrl = "http://localhost:3033/login";
       let profile: Profile;
       pp.use(
-        new SamlStrategy(config, function (
-          _profile: Profile | null | undefined,
-          done: VerifiedCallback
-        ): void {
+        new SamlStrategy(config, function (_profile: Profile | null, done: VerifiedCallback): void {
           if (_profile) {
             profile = _profile;
             done(null, { id: profile.nameID });
@@ -180,7 +177,7 @@ describe("captured saml responses /", function () {
       pp.use(
         new SamlStrategy(config, function (
           req: express.Request,
-          _profile: Profile | null | undefined,
+          _profile: Profile | null,
           done: VerifiedCallback
         ) {
           if (_profile) {

test/strategy.spec.ts

@@ -1,8 +1,8 @@
 "use strict";
 
 import * as sinon from "sinon";
-import { SAML, Strategy as SamlStrategy } from "../src";
-import { RequestWithUser } from "../src/types";
+import { Profile, SAML, Strategy as SamlStrategy } from "../src";
+import { RequestWithUser, VerifiedCallback } from "../src/types";
 import { FAKE_CERT } from "./types";
 
 const noop = () => undefined;
@@ -79,22 +79,29 @@ describe("strategy#authorize", function () {
   });
 
   it("determines that logout was unsuccessful where user doesn't match", function (done) {
-    const strategy = new SamlStrategy({ cert: FAKE_CERT }, noop);
+    const strategy = new SamlStrategy({ cert: FAKE_CERT }, function (
+      _profile: Profile | null,
+      done: VerifiedCallback
+    ) {
+      if (_profile) {
+        done(null, { name: _profile.nameID });
+      }
+    });
+
     validatePostResponseAsync.resolves({
       profile: {
         ID: "ID",
         issuer: "issuer",
-        nameID: "nameID",
+        nameID: "some other user",
         nameIDFormat: "nameIDFormat",
       },
       loggedOut: true,
     });
 
+    // Pretend we already loaded a users session from a cookie or something
+    // by calling `strategy.authenticate` when the request comes in
     requestWithUserPostResponse.user = {
-      ID: "ID",
-      issuer: "issuer",
-      nameID: "otherNameID",
-      nameIDFormat: "nameIDFormat",
+      name: "some user",
     };
 
     // This returns immediately, but calls async functions; need to turn event loop
@@ -115,22 +122,29 @@ describe("strategy#authorize", function () {
   });
 
   it("determines that logout was successful where user matches", function (done) {
-    const strategy = new SamlStrategy({ cert: FAKE_CERT }, noop);
+    const strategy = new SamlStrategy({ cert: FAKE_CERT }, function (
+      _profile: Profile | null,
+      done: VerifiedCallback
+    ) {
+      if (_profile) {
+        done(null, { name: _profile.nameID });
+      }
+    });
+
     validatePostResponseAsync.resolves({
       profile: {
         ID: "ID",
         issuer: "issuer",
-        nameID: "nameID",
+        nameID: "some user",
         nameIDFormat: "nameIDFormat",
       },
       loggedOut: true,
     });
 
+    // Pretend we already loaded a users session from a cookie or something
+    // by calling `strategy.authenticate` when the request comes in
     requestWithUserPostResponse.user = {
-      ID: "ID",
-      issuer: "issuer",
-      nameID: "nameID",
-      nameIDFormat: "nameIDFormat",
+      name: "some user",
     };
 
     // This returns immediately, but calls async functions; need to turn event loop