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

LDAP identity provider: fix support of an associated Shibboleth auth provider #42

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

jouvin
Copy link
Contributor

@jouvin jouvin commented Jan 4, 2021

The Indico documentation suggests that it is a good idea to use an LDAP identity provider, when you have one like AD, with a Shibboleth auth provider, to benefit from the group definitions coming from LDAP. The problem is that the LDAP identity provider requires the user to exist in LDAP to create its identity, something that is not true if remote users are authenticated through Shibboleth (e.g. eduGAIN).

The original approach to fixe the issue was by addding a new identity provider boolean configuration parameter, auth_data_if_identifier_missing: if True, create the identity from the AuthInfo object (returned by the Shibboleth auth provider) if the user is not found in LDAP. When False, the identity provider behaviour is unchanged. After some discussion it was decided to implement basically the same code as a variant of the LDAP identity provider that fallbacks to AuthInfo contents if the identifier cannot be retrieved from LDAP.

**Note: for this configuration to work, it is also necessary to define the attribute to use to build the identifier, adding an entry in PROVIDER_MAP that defines it, with something like:`

# shib-sso is the Shibboleth auth provider, ldap is the LDAP identity provider
'shib-sso': {'identity_provider': 'ldap', 'mapping': {'identifier': 'mail'}}

This PR has been tested successfully at IJCLab.

@jouvin jouvin changed the title LDAP identity provider: allow to configure the identifier field LDAP identity provider: fix support of an associated Shibboleth auth provider Jan 4, 2021
@jouvin
Copy link
Contributor Author

jouvin commented Jan 4, 2021

@ThiefMaster I decided to add the support for non local users as an option in the standard LDAP provider as it is really a one line difference. I think it could interest others. It is not clear for me if we should also set support_get and support_refresh to False when accepted_users=all.

@jouvin jouvin force-pushed the ldap_configurable_identifier branch from b344bae to 6489fd9 Compare January 6, 2021 18:47
@ThiefMaster
Copy link
Member

This isn't needed in the end, because you can rename fields via the auth-to-identity-provider mapping. See this Indico forum post for an example

@jouvin
Copy link
Contributor Author

jouvin commented Dec 13, 2023

@ThiefMaster I updated the PR (and its description) to remove the identifier_field parameter handling from the original PR as an alternative way of doing it exists as you said in #42 (comment). But I believe the other part of PR is still required if you want to be able to support remote users through Shibboleth/eduGAIN. Unfortunately I cannot reopen the PR and thus the updated source branch is not reflected here...

@ThiefMaster
Copy link
Member

I think the PR can be reopened if you force-push commit 6489fd9 to the branch (since that was the last commit before it was closed). Once it's open you can force-push your new commit again.

git push -f whatever 6489fd9f2:ldap_configurable_identifier

@jouvin
Copy link
Contributor Author

jouvin commented Dec 13, 2023

It is what I did... but it seems it was not enough... Is it because I also rebased on the last master?

@ThiefMaster
Copy link
Member

Yes, you need to push that exact commit to the branch. Then I can probably reopen it (you probably can't do it yourself since it was closed by a maintainer). Afterwards you can rebase and forcepush as much as you want again.

@jouvin
Copy link
Contributor Author

jouvin commented Dec 13, 2023

Ah ok, I misunderstood...

@jouvin
Copy link
Contributor Author

jouvin commented Dec 13, 2023

Done!

@ThiefMaster ThiefMaster reopened this Dec 13, 2023
@ThiefMaster
Copy link
Member

there we go, now you can forcepush your latest version

@jouvin jouvin force-pushed the ldap_configurable_identifier branch from 6489fd9 to a8ac6d3 Compare December 13, 2023 10:37
@jouvin jouvin force-pushed the ldap_configurable_identifier branch 2 times, most recently from e82ec39 to 73a048c Compare December 13, 2023 13:34
@jouvin
Copy link
Contributor Author

jouvin commented Dec 13, 2023

I push a new approach to the problem described, as per our offline discussion, that implement by default the access_useres=all behaviour, without the need of an additional parameter. It provides a new variant of the LDAP Identity Provider that can be selected with identity type ldap_shibboleth.

Test in progress at IJCLab...

@jouvin jouvin force-pushed the ldap_configurable_identifier branch 2 times, most recently from cddd775 to 046ccc4 Compare December 13, 2023 14:50
@jouvin
Copy link
Contributor Author

jouvin commented Dec 13, 2023

Test done successfully at IJCLab (rebuilding manually the wheel after cherry-picking the commit on top of v0.4.9).

@ThiefMaster
Copy link
Member

great, let's just apply the name change i mentioned on mattermost and this is good to go

- Provides the ability to retrieve the information from Shibboleth
  auth-data if identity identifier is missing. Allows to support remote
  users not part of the LDAP directory
- Available by using the identity type ldap_or_authinfo
@jouvin jouvin force-pushed the ldap_configurable_identifier branch from 046ccc4 to 24a2591 Compare December 13, 2023 15:07
@jouvin
Copy link
Contributor Author

jouvin commented Dec 13, 2023

Names for class and identity provider type updated (as well as the comment) to make it more general...

@ThiefMaster ThiefMaster enabled auto-merge (squash) December 13, 2023 17:19
@ThiefMaster ThiefMaster merged commit d12c780 into indico:master Dec 13, 2023
6 checks passed
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