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

Fix: Conflicting profile properties between profile and attributes #593

Merged

Conversation

kriss1897
Copy link
Contributor

Description

attributes.forEach((attribute) => {

It is currently possible to override some default properties of the profile object in processValidlySignedAssertionAsync method.
This can be done if any property like Issuer is also present in the attribute mapping. The expected behavior here should be that the properties (Issuer, inResponseTo, sessionIndex, nameID, nameIDFormat, nameQualifier, spNameQualifier) will probably already be present in the profile, and in that case they shouldn't be overridden by the same named attribute in attribute mapping.

This issue can be handled gracefully by simply skipping the value from attribute mapping or an error can be thrown. This PR handles the issue gracefully.

Checklist:

This attributes are also mounted to profile directly in a non
conflicting way.
@cjbarth
Copy link
Collaborator

cjbarth commented May 19, 2021

This looks good to me. However, either we need to make this PR only against the node-saml project, or we need to land this PR in both this and that project.

@kriss1897
Copy link
Contributor Author

@cjbarth We can add this PR on both the projects. That way it's not directly dependent on node-saml release.

@cjbarth
Copy link
Collaborator

cjbarth commented May 20, 2021

Sounds good to me. As soon as I see a corresponding PR in the node-saml project, we'll land it here.

@kriss1897
Copy link
Contributor Author

@cjbarth Added same PR on node-saml repo. node-saml/node-saml#5

@cjbarth cjbarth merged commit 2a1699b into node-saml:master Jun 17, 2021
@cjbarth cjbarth linked an issue Jun 17, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Possibility to overwrite profile fields with data from attributes
2 participants