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] Possibility to overwrite profile fields with data from attributes #543

Closed
gtokarz opened this issue Feb 19, 2021 · 7 comments · Fixed by #593
Closed

[BUG] Possibility to overwrite profile fields with data from attributes #543

gtokarz opened this issue Feb 19, 2021 · 7 comments · Fixed by #593
Labels

Comments

@gtokarz
Copy link

gtokarz commented Feb 19, 2021

Currently attributes are mapped to profile without any check, which might at best cause minor issues with previously present properties from profile object.

To Reproduce

Configure Idp/craft a SAML response with attribute Name set to i.e issuer or other profile field set before attribute mapping. Mapped profile object should now have overwritten issuer field.

Expected behavior
It should check if property already exists on the profile object and only map values when key is not present.

attributes.forEach((attribute) => {

Environment

  • Node.js version: 12.16.1
  • passport-saml version: 2.0.5
@cjbarth
Copy link
Collaborator

cjbarth commented Feb 20, 2021

@gtokarz would you be willing to create a PR with tests for this?

@gtokarz
Copy link
Author

gtokarz commented Feb 20, 2021

@cjbarth Yes, I might have some time on Monday to do so

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

@gtokarz We are preparing a 3.0 release. Would you be interested in getting this change up for PR in the next few weeks to make it in to 3.x?

@kriss1897
Copy link
Contributor

@cjbarth @gtokarz Is it okay if I raise a PR for this?

@cjbarth
Copy link
Collaborator

cjbarth commented May 13, 2021

@kriss1897 sure, anything with a pr-welcome tag we are happy to have someone help with. Please make sure to include tests for this.

@gtokarz
Copy link
Author

gtokarz commented May 14, 2021

yes, go ahead with PR

@kriss1897
Copy link
Contributor

@cjbarth @gtokarz I've raised a PR for fixing this, however I'm unable to assign any reviewers. Please take a took when you guys have time.

kriss1897 added a commit to kriss1897/passport-saml that referenced this issue May 19, 2021
This attributes are also mounted to profile directly in a non
conflicting way.
cjbarth pushed a commit that referenced this issue Jun 17, 2021
* Fix: Conflicting profile properties between profile and attributes (#543)

* Add assertion attributes to child object on profile (#543)

This attributes are also mounted to profile directly in a non
conflicting way.

Co-authored-by: Shashank Singh Solanki <shashank.solanki@postman.com>
@cjbarth cjbarth closed this as completed Jun 17, 2021
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 a pull request may close this issue.

3 participants