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] Empty and Null Attributes are not returned in profile.attributes #227

Open
maxwellgerber opened this issue Nov 23, 2022 · 3 comments
Labels
bug Something isn't working pr-welcome

Comments

@maxwellgerber
Copy link

To Reproduce

Encounter a SAML assertion containing an empty Attribute tag with no nested AttributeValue.

example:<saml2:Attribute Name="employee_id"/>

I've encountered these empty tags in the wild using Google Workspaces as a SAML IDP

        it("A null value given with an object should be null", async () => {
          const xml =
            '<Response xmlns="urn:oasis:names:tc:SAML:2.0:protocol" ID="response0">' +
            '<saml2:Assertion xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion" Version="2.0">' +
            "<saml2:AttributeStatement>" +
            '<saml2:Attribute Name="attributeName" ' +
            'NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified" />' +
            "</saml2:AttributeStatement>" +
            "</saml2:Assertion>" +
            "</Response>";

          const signingKey = fs.readFileSync(__dirname + "/static/key.pem");
          const signingCert = fs.readFileSync(__dirname + "/static/cert.pem", "utf-8");
          const signedXml = signXmlResponse(xml, { privateKey: signingKey });

          const base64xml = Buffer.from(signedXml).toString("base64");
          const container = { SAMLResponse: base64xml };
          const samlObj = new SAML({
            cert: signingCert,
            audience: false,
            issuer: "onesaml_login",
            wantAssertionsSigned: false,
          });
          const { profile } = await samlObj.validatePostResponseAsync(container);
          assertRequired(profile, "profile must exist");
          expect(profile.attributes?.attributeName).to.be.null;
        });

Expected behavior

The current behavior of omitting the value entirely makes it difficult to determine if the attribute is configured at all by the IdP, or
if the data associated with the user is incorrect. For example {first_name: null} means that the user doesn't have a first name, while {} means that maybe the user doesn't have a first name, maybe the attribute isn't configured properly.

No Attribute Value

SAML Core line 1219 states

Within an <AttributeStatement>, if the SAML attribute exists but has no values, then the <AttributeValue> element MUST be omitted.

I believe that the most idiomatic way to represent something that exists but has no values - e.g. <saml2:Attribute Name="employee_id"/> - would be as { "employee_id": null }.

Empty Attribute Value

SAML Core line 1246 states:

If a SAML attribute includes an empty value, such as the empty string, the corresponding
<AttributeValue> element MUST be empty (generally this is serialized as <AttributeValue/>).
This overrides the requirement in Section 1.3.1 that string values in SAML content contain at least one
non-whitespace character.

I believe the test case An undefined value given with an object should still be undefined is incorrect - the empty attribute with type xs:string should be returned as an empty string literal (''), not as undefined.

I'm more than happy to update the code + testcases - I understand these would be breaking changes - so please let me know if there is a preferred way to structure the PRs / some future branch I should target.

Environment

  • Node.js version: 18
  • @node-saml/node-saml version: 4.0.2
@maxwellgerber maxwellgerber added the bug Something isn't working label Nov 23, 2022
@cjbarth
Copy link
Collaborator

cjbarth commented Jan 11, 2023

Feel free to submit a PR with test cases and I'll have a closer look. Just from reading your description it does seem that undefined would be improper for xs:string.

We'll do a breaking-change release with the deprecation of Node 14, so we still have 2-3 months to get this right before releasing it.

@Fabiomad85
Copy link

Same problem here. also for the number. I got the string instead of the right value

@cjbarth
Copy link
Collaborator

cjbarth commented Feb 6, 2024

I'm getting ready to do another semver-major release soon. @Fabiomad85 , would you like to submit a PR to address this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pr-welcome
Projects
None yet
Development

No branches or pull requests

3 participants