DataShades / ckanext-saml2

SAML2 Authentication extension for CKAN
5 stars 23 forks source link

Consolidate the id stored against the Saml2 user to avoid duplicates #102

Open amercader opened 5 years ago

amercader commented 5 years ago

At the beggining of the indentify process, a NameId is extracted from the payload that the Saml2 backend sends, and that is set on environ['REMOTE_USER'] by repoze.who:

https://github.com/DataShades/ckanext-saml2/blob/91ccffd3ec96d76d9f3b4fdc59d6140a0c04ef8e/ckanext/saml2/plugin.py#L295:L298

In my case this payload was:

REMOTE_USER = "2=urn%3Aoasis%3Anames%3Atc%3ASAML%3A1.1%3Anameid-format%3AemailAddress,4=amercader%40rockfound.org"

So the NameId extracted was amercader@rockfound.org.

This NameId is immediately used to check if a Saml2 user with the same name id exists in the database.

The problem is that when creating a new instance of a Saml2User, the extension is using a different id:

https://github.com/DataShades/ckanext-saml2/blob/91ccffd3ec96d76d9f3b4fdc59d6140a0c04ef8e/ckanext/saml2/plugin.py#L429

This saml_info comes from environ["repoze.who.identity"]["user"] and in my case environ["repoze.who.identity"]["user"]["id"] (what is used when creating the new SAML2User) was just amercader.

This meant that every time I tried to log in, the extension couldn't find any existing user with NameId amercader@rockfound.org, so it tried to create a new one (and failed because of the SAML2User.name_id unique constraint).

This patch ensures that the same key that is used to check if a user exists is the one that is actually stored.

I assume that in most scenarios these two ids are the same and that's why this hasn't been an issue so far.

Engerrs commented 5 years ago

Hi @amercader , you are totally right about name_id. I created a new PR and merged it into the branch but with a small update, in order not to break existing logic to people that are already using SAML2 extension. Here are the details of the change https://github.com/DataShades/ckanext-saml2/commit/d9b1678ae14e07c5710631c2f389ce63bdb2576e