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

SamlAuth: Can't build profile if User attribute names are uris #221

Closed
Iblis opened this issue Aug 2, 2019 · 5 comments
Closed

SamlAuth: Can't build profile if User attribute names are uris #221

Iblis opened this issue Aug 2, 2019 · 5 comments
Labels
Milestone

Comments

@Iblis
Copy link

Iblis commented Aug 2, 2019

When using Saml, the buildProfile method uses 'getAttributeNames' to map the SamlResponse user attributes to profile claims.
Some IdPs return uris as attribute names, which does not work well with mustache (the hostnames of the uris have dots and dots can't be escaped in mustache).
This makes building the profile impossible in those cases.
The used saml2-js lib has a neat feature to prettify attributes like that.
https://github.com/apim-haufe-io/saml2/blob/bc740c26623244758c89a96134e425e8d01bc82f/lib/saml2.coffee#L468

The prettified attributes are directly put into the user object, which works well, but the saml provider form wicked.auth does not retrieve them form there.

Options that come to my mind:

  • check if no attributes where mapped. If so, search for the attributes in the user object
  • let wicked-saml2-js put it in another object, like 'prettyAttributes' so that it is easier to detect if there was the need to prettify. If the object exists/not null, use that instead of 'attributes'
  • let the config decide by extending the mustache templating (eg. user.email / attribute.email instead of just 'email' in the template)

Probably there are more options. So any ideas how to proceed here?

@DonMartin76
Copy link
Member

Thanks for filing this issue; I wasn’t aware of this. Can you give me an example of an IdP implementation which does this?

Pull requests are always welcome in case you have something which would fix this already? Preferably in a way which keeps backwards compatibility.

In any case: Can you provide me with a sample SAML assertion so that I can see how it looks like?

@DonMartin76 DonMartin76 added the bug label Aug 2, 2019
@Iblis
Copy link
Author

Iblis commented Aug 3, 2019

I can't provide the pure SamlResponse right now, but here is what Wicked works with (copied from the logs:

{
    "response_header": {
        "version": "2.0",
        "destination": "https://api-host/auth/vx-saml/assert",
        "in_response_to": "_de086aedac7b25377a67663559a9f82e42a2453792",
        "id": "_9f5e70b3-7d38-4fb7-9225-a990f0802e1f"
    },
    "type": "authn_response",
    "user": {
        "name_id": "example\\testUser",
        "session_index": "_eb172d13-2298-4be8-ba35-3c026a060f07",
        "email": "test@example.com",
        "upn": "testUser@example.ads",
        "surname": "User",
        "given_name": "Test",
        "attributes": {
            "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress": [
                "test@example.com"
            ],
            "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn": [
                "testUser@example.ads"
            ],
            "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname": [
                "User"
            ],
            "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname": [
                "Test"
            ]
        }
    }
},
"delta": 0,
"level": "debug"
}

I don't have a fix yet, but will try providing a PR with my first proposal: If required attributes like sub are not mapped, map again using the user object.

@Iblis
Copy link
Author

Iblis commented Aug 3, 2019

apim-haufe-io/wicked.auth#8 did the trick for me

@DonMartin76
Copy link
Member

Thanks a lot for your PR! I had one small thing, then this looks good for me.

@DonMartin76 DonMartin76 added this to the 1.0.0-rc.10 milestone Aug 11, 2019
@DonMartin76
Copy link
Member

Merged.

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

2 participants