Breaking change in v5.0.0, docs not updated (TypeError: callbackUrl is required) about passport-saml HOT 10 OPEN

You are right, path no longer works because it was removed from node-saml on which passport-saml depends. The README should have its example updated. Please submit a PR. Also, note in the README how it does reference node-saml, so that might help you see other changes that are noteworthy during migration.

from passport-saml.

This is related to node-saml/node-saml#214

While I completely agree that removing complexity is a good thing, I now miss the possibility to dynamically configure the callbackUrl: in an environment where I do not know my public hostname, I could just use a relative path. This is not possible anymore, and I cannot even guess my hostname using the current request, since configuration takes place outside of a request context. Or am I missing something?

from passport-saml.

So the "new" way when you can only use path (but do not know your own host) is this:

passport.use(
    new SamlStrategy(
        {
            callbackUrl: 'http://some-random-host/some-random-url/', // required but not used, see below
            ...
        }
    )
);

app.get(
    "/login",
    (req, res, next) => {
        path = '/login/callback';
        callbackUrl = (req.protocol || 'http').concat('://') + (req.headers?.host || 'localhost') + path;
        passport.authenticate("saml", { callbackUrl })(req, res, next),
    }
);

Right?

from passport-saml.

I would probably use const path = ..., etc. I would probably also use a URL builder API instead of string concatenation like https://nodejs.org/api/url.html#url_url_format_urlobject, but yes, you have the right idea. Another approach would be to use a pattern like passport-saml has for multi-SAML strategy.

Basically, the node-saml library isn't the in the business of building URLs anymore. There were problems with the implementation, and to make the problems go away it was decided that we'd simplify the code and offload that work to the consumer (who could use another library). You're case does seem a little strange, and it isn't always wise to trust req.headers.host; see node-saml/node-saml#214, https://portswigger.net/web-security/host-header, and https://cqr.company/web-vulnerabilities/http-host-header-attacks/

from passport-saml.

library isn't the in the business of building URLs anymore

I agree. However there are legitimate cases where the service does not know its hostname, e.g. when it is deployed in staging branches, etc. I cannot hard-code a callbackUrl there, otherwise we'd risk surprises when redirecting back to the wrong host. So there should be a "best practice" guide on how to do this in case other users also need to rely on the previous path behaviour.

I'll try to make a sensible README PR...

from passport-saml.

Our official suggestion is to pass in a constructed URL, like you're currently doing. We'd be open to a PR that would take a function for callbackUrl, so that you can build it on invocation.

from passport-saml.

Ok I need to withdraw my solution: callbackUrl is not used at all in the authenticate method. So the solution needs to be a function for callbackUrl.

from passport-saml.

Honestly, I don't know why you don't have some parameters for your different environments that you can use to build a URL specific to the environment. An environment variable could be used to switch which URL is passed in. Or, you could use the multi-SAML functionality. I'm open to the function idea still, but it seems that this might be pointing to a more endemic situation. How do you change your DB config or other such-like things per environment?

from passport-saml.

... Or, you could use the multi-SAML functionality. I'm open to the function idea still, but it seems that this might be pointing to a more endemic situation. ...

@nicokaiser as @cjbarth is trying to say: Multisaml approach provides access to request (to extract whatever request specific information) and it provides mechanism to return e.g. fixed set of other config variables and change callbackUrl config per request (which would solve this issue reporter's case).

This comment entry's actual point is: why even consider new round of fine-grained configuration stuff because (and especially in this case/for this case) there is goarse-grained approach (from passport-saml pov) already available which provides mechanism to change about everything per request.

Sidenote: host parameters of various methods at node-saml are not used anymore https://github.com/node-saml/node-saml/blob/v5.0.0/src/saml.ts

Seems that those were missed during node-saml/node-saml#214 review. Unfortunately removing those is breaking change.

from passport-saml.