Simplify callback URL options; remove path, protocol, and host.

[cjbarth]

Description

Remove callback options path, protocol, and host, use existing callbackUrl instead.

Addresses node-saml/passport-saml#677 (comment)

[srd90]

Just a few thoughts about this pending breaking change.

IMHO breaking change could be that this code block along with related configuration options would be removed completely:

node-saml/src/saml.ts

Lines 145 to 158 in 7bf1593

	} else {
	const url = new URL("http://localhost");
	if (host) {
	url.host = host;
	} else {
	url.host = this.options.host;
	}
	if (this.options.protocol) {
	url.protocol = this.options.protocol;
	}
	url.pathname = this.options.path;
	return url.toString();
	}
	}

Now there is unnecessary "complexity" even though at the end of the day from library API point of view developer has to provide exactly same information
Option 1:

  ...
  host: "mysite.local",
  path: "/ACS",
  protocol: "https",
  ...

Option2

  ...
  callbackUrl: "https://mysite.local/ACS"
  ...

Hard to see "added value" of multiple ways to configure exactly same information. I'm suggesting that Option2 would be way forward.

ACS callback url configuration has been throughout the history a bit problematic due multiple ways to configure that information see e.g. discussion at those pull requests: https://github.com/node-saml/passport-saml/pulls?q=is%3Apr+callbackurl+is%3Aclosed

Once there is exactly one way to configure it then solving this node-saml/passport-saml#509 would be easy because there would not be too many code paths to consider. I.e. it would be just matter of comparing callbackUrl with Recipient information(*):

4.1.4.3 <Response> Message Processing Rules

Regardless of the SAML binding used, the service provider MUST do the following:

• Verify any signatures present on the assertion(s) or the response
• Verify that the Recipient attribute in any bearer <SubjectConfirmationData> matches the
  assertion consumer service URL to which the <Response> or artifact was delivered
• ... and so one ...

source: http://docs.oasis-open.org/security/saml/v2.0/saml-profiles-2.0-os.pdf

Furthermore it seems that after passport-saml --> core node-saml split one problematic codepath was already removed because callbackUrl generation does not use values from req anymore:

node-saml/src/passport-saml/saml.ts

Lines 219 to 227 in 0d983aa

	} else {
	let host;
	if (req.headers) {
	host = req.headers.host;
	} else {
	host = this.options.host;
	}
	return this.getProtocol(req) + host + this.options.path;
	}

(*) When SP is introduced to IdP (trust relationship is established) one has to configure to IdP a SP's ACS URL (i.e. Recipient) and IdP must/should not "release" authn response to ACS URL which is not configured for SP. If SP sits behind some API gateway then developer must (and has already had to) configure that API gateway's address to IdP. Now (if this simplification is done in order to make aforementioned issue implementation easier) developer would be forced to configure that same information to @node-saml/passport-saml / @note-saml/node-saml also.

[markstos]

Furthermore it seems that after passport-saml --> core node-saml split one problematic codepath was already removed because callbackUrl generation does not use values from req anymore

I agree. So doing nothing here is also a reasonable option.

As I understand, "localhost SSL certs" can be a pain and don't do much security. Using plain http for local development is a nice and reasonable feature. So https://localhost feels more like security theater here.

So I'd be in favor if either doing nothing or to taking @srd90's suggestion to simplify the code with a breaking change to how callbackUrl is defined to remove the else clause there.

[cjbarth]

@markstos @srd90 , based on your feedback, what do you think of these changes?

[codecov]

Codecov Report

Merging #214 (068bcfd) into master (40b4620) will decrease coverage by 0.29%.
The diff coverage is 100.00%.

❗ Current head 068bcfd differs from pull request most recent head 3cad8a2. Consider uploading reports for the commit 3cad8a2 to get more accurate results

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
- Coverage   80.82%   80.54%   -0.29%     
==========================================
  Files          11       11              
  Lines         824      812      -12     
  Branches      252      248       -4     
==========================================
- Hits          666      654      -12     
  Misses         68       68              
  Partials       90       90              

Impacted Files	Coverage Δ
src/types.ts	100.00% <ø> (ø)
src/saml.ts	77.83% <100.00%> (-0.49%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[markstos]

LGTM.

[srd90]

@cjbarth maybe this #214 PR's title should be modified prior to node-saml v5 release because

1. implementation of this PR ended up leaving just callbackUrl option and it is up to client SW to provide value
2. node-saml v5 change log would not provide correct info about this breaking change if title is not modified

[cjbarth]

@srd90 , thanks for pointing this out. Change made.