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

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

Open
wants to merge 1 commit into
base: link-master
Choose a base branch
from

Conversation

amercader
Copy link

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:

name_id = _take_from_saml_or_user('id', saml_info, data_dict)

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.

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
Copy link
Member

Engerrs commented Apr 16, 2019

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 d9b1678

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants