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

Audience must be supplied #814

Closed
bvds opened this issue Nov 17, 2022 · 4 comments
Closed

Audience must be supplied #814

bvds opened this issue Nov 17, 2022 · 4 comments
Labels

Comments

@bvds
Copy link

bvds commented Nov 17, 2022

The documentation says that audience is optional. I had to explicitly set it to false.

Environment

  • Node.js version: v14.19.3
  • passport-saml version: 4.0.1
@bvds bvds added the bug label Nov 17, 2022
@srd90
Copy link

srd90 commented Nov 23, 2022

For those who run into same issue and ends up reading this bug report.

audience check was changed on by default >= 4.0.0.

It is implemented at @node-saml/node-saml which provides core SAML functionality for @node-saml/passport-saml.
Unfortunately @node-saml/passport-saml side documentation is not quite up to date yet regarding various parameters.

Quote from @node-saml/node-saml 4.0.0 README.md:

  • audience: expected saml response Audience, defaults to value of Issuer (if false, Audience won't be verified)

Related issue is #137 and related PR node-saml/node-saml#25

fwiw2, Recipient validation on the other hand is/should be always mandatory but @node-saml/passport-saml / @node-saml/node-saml doesn't support it yet ( #509 ).

@markstos
Copy link
Contributor

@cjbarth I'm afraid if I manually edit the Changelog file it will break tooling that automatically updates, but it seems worth mentioning retroactively that this is a breaking change for passport-saml 4.0. The reference I found to link to is this PR: node-saml/node-saml#25

@cjbarth
Copy link
Collaborator

cjbarth commented May 16, 2023

The tooling is pretty robust. There is a slight problem with how we did the beta releases for node-saml that causes the old changelog to look weird, and there is one line for the CVE that must me manually maintained, but that is super easy to do manually. Otherwise, just adding tags to a PR will adjust how it looks in the changelog. This change is currently listed as a "Major Change" in the changelog. Do you think it needs to be listed differently?

@markstos
Copy link
Contributor

That's perfect! I missed it when combing over the various Major changes in the 4.0 beta releases. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants