Userinfo

[mamico]

Management of user properties from oidc userinfo.

Widely inspired by https://github.com/collective/pas.plugins.headers/blob/master/src/pas/plugins/headers/plugins.py#L404 and https://github.com/collective/pas.plugins.authomatic/blob/main/src/pas/plugins/authomatic/useridentities.py#L30.

The idea could be to also implement IUserEnumeration on the implemented properties storage (https://github.com/collective/pas.plugins.oidc/blob/userinfo/src/pas/plugins/oidc/plugins.py#L157) and completely eliminate the code related to the creation of a user (https://github.com/collective/pas.plugins.oidc/blob/userinfo/src/pas/plugins/oidc/plugins.py#L193) in the source_users storage.

@erral @mauritsvanrees opinions on this?

[coveralls]

Pull Request Test Coverage Report for Build 4690358589

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 55 of 69 (79.71%) changed or added relevant lines in 2 files are covered.
• 82 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+4.3%) to 57.959%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/pas/plugins/oidc/setuphandlers.py	1	2	50.0%
src/pas/plugins/oidc/plugins.py	54	67	80.6%

Files with Coverage Reduction	New Missed Lines	%
src/pas/plugins/oidc/setuphandlers.py	10	69.7%
src/pas/plugins/oidc/plugins.py	22	69.11%
src/pas/plugins/oidc/browser/view.py	50	30.41%

Totals
Change from base Build 4612745312:	4.3%
Covered Lines:	284
Relevant Lines:	490

---

💛 - Coveralls

[erral]

The idea of persisting the user properties in the plugin itself is great, I have used it in the past when developing "login with facebook" or "login with twitter" (in the pre-oauth-era).

I would add some documentation about the userinfo_to_memberdata field with some examples on how to fill that, but it's OK from my side.

[mauritsvanrees]

I did not try it, but the code seems fine.

[mauritsvanrees]

With self.create_user we create a standard Plone user like you would get when using the Users and Groups control panel, right?
I doubt that it is then still needed to set self._userdata_by_userid.

Ah, but when create_user is False for a while, and only later switched to True, and we do not update self._userdata_by_userid, it may get outdated and not in sync with the standard member data.
We could remove existing data when create_user is True.
Hard to say for sure what is best without trying it. But should be okay like it is now.

[mauritsvanrees]

I think this file is not used.

[mauritsvanrees]

The idea could be to also implement IUserEnumeration on the implemented properties storage (https://github.com/collective/pas.plugins.oidc/blob/userinfo/src/pas/plugins/oidc/plugins.py#L157) and completely eliminate the code related to the creation of a user (https://github.com/collective/pas.plugins.oidc/blob/userinfo/src/pas/plugins/oidc/plugins.py#L193) in the source_users storage.

I think that could work, yes, and make things a bit simpler.
But maybe first try the current code from this PR out for a while.

[mauritsvanrees]

I tried this PR out in combination with LDAP, and there it is working fine for our use case. Let me share all my notes.

On a test site (Plone Classic UI 6.0.8, Python 3.11) we have pas.plugins.oidc (2.0.0a1) and pas.plugins.ldap (p6 branch). How can we let them work together?

You could wonder if LDAP is even still needed in this case. But with the current oidc plugin, you only get information for users who have authenticated. For the moment, I think we want all users, so LDAP is still needed.

Note: in our setup, we want to handle groups in Plone, and are not interested in what we get here from either LDAP or oidc.

Some settings:

• /Plone/acl_users/oidc has "Create user / update user properties" true. Open id scopes: openid and profile.
• Plone/acl_users/pasldap has all interfaces activated.
• Using the standard login form works: this authenticates via LDAP.
• Authenticating via oidc works as well.

Sample oidc userinfo that we get from Keycloak:

[('sub', 'abcd717d-1234-4567-1234-1212af041212'),
 ('email_verified', False),
 ('name', 'Maurits van Rees'),
 ('preferred_username', 'ma.vanrees'),
 ('given_name', 'Maurits'),
 ('family_name', 'van Rees'),
 ('email', '[email protected]')]

The fullname that I get from LDAP is actually slightly different: "Maurits van Rees (ext)", for external.

Initially with this setup, I got two users, one with and one without "ext" in the fullname One was from LDAP, with user id 'ma.vanrees', one from oidc, with as user id the id from the "sub" property.

So I removed the user with the sub id from acl_users/source_users, and updated the oidc plugin settings with user_property_as_userid=preferred_username.
Login again with oidc, and only the existing ma.vanrees user remain.

One problem though: whenever I login with oidc, this plugin calls user.setProperties and this calls the LDAP plugin which tries to set properties in LDAP, and this fails. In the background you get an 'Insufficient access' error from LDAP. The error is ignored though, so you don't really notice.

So now let's check how the current PR influences the situation. Okay, switch to this branch, restart, apply the upgrade step.

Now the insufficient access error is gone. Why? Because when calling setProperties PlonePAS goes through all property sheets in order, and tries to set a property. Once a property (say the fullname) is set, PlonePAS no longer tries to set it in any further property sheets. So once it reaches the LDAP property sheet, no properties are left to be set, so there is no error.

So now for fun you can play with the order of the properties plugins in /Plone/acl_users/plugins/manage_plugins. If pasldap is on the top, I see "Maurits van Rees (ext)" as fullname. Moving oidc to the top I see "Maurits van Rees" as fullname. And as long as pasldap is below the standard mutable_properties, I do not get the LDAP error during login.

I was also testing the new "Mapping from userinfo to memberdata property" from this PR:

• In the oidc plugin add given_name|given_name in this property.
• In /Plone/@@member-fields add a given_name property.
• Login again and I see "Maurits" in the given_name property on the @@personal-information page.

So it works. Thanks!

It could use some tests, and documentation.
Oh, and the useridentities.py file in this PR is unused.

[mamico]

@mauritsvanrees thanks!

It could use some tests, and documentation.

I need to learn the new tests introduced by Erico. But I think I can make up some time this weekend.