feat: account info auto-update

[mirekys]

WIP

Description

This PR introduces the auto-update mode, that updates user's account information from OIDC userInfo data after successful authentication.

Related Issue

• Fixes https://github.com/owncloud/openidconnect/issues/187

Motivation and Context

auto-provision mode of openid-connect app creates user account from OIDC claims, but after that, there is currently no way to further update account information, whenever claims coming from an OIDC provider change over time (e. g. user changes an e-mail contact at the provider).

How Has This Been Tested?

Unit tests added for the following scenarios:

1. updates disabled, not forced
2. update disabled by config, but forced from a newly provisioned account (setting of inital DN & email)
3. updates enabled, but missing claims configuration
4. updates enabled, used together with auto-provisioning mode
5. updates enabled, used without auto-provisioning mode
6. configured to update display name only
7. configured to update e-mail only
8. not updating if attributes are missing in userInfo
9. not updating email if not allowed by user's backend
10. not updating display name if not allowed by user's backend
11. not updating if no change happened

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)

• [x] New feature (non-breaking change which adds functionality)

• [ ] Breaking change (fix or feature that would cause existing functionality to change)

• [ ] Technical debt

• [x] Tests

• Adds the new configuration section:

  'auto-update' => [
    // enable the user info auto-update mode
    'enabled' => true, 
    // defines account attributes that will be auto-updated
    'attributes' => ['email', 'display-name'],
    // only relevant if auto-provision is disabled,  defines the claims used to update account info
    'email-claim' => 'email', 
    // defines the claim which holds the display name of the user
    'display-name-claim' => 'given_name', 
  ]

• Adds the new AccountUpdateService (with updateAccountInfo(IUser $user, $userInfo, bool $force = false)

• AutoProvisioningService and Client refactored to faciliate code reuse

Checklist:

• [x] Code changes
• [x] Unit tests added
• [ ] Acceptance tests added
• [ ] Documentation ticket raised:

Open tasks:

• [ ] ...

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[mmattel]

@mirekys :wave: any update on this ?

[micbar]

@mirekys thank you for your contribution!

Can you give us this Code change under MIT license?

according to @hodyroff We could skip the CLA check.

Please check @DeepDiver1975 s comments, address them and get the tests passing. Coding Standard and static analyzers are complaining.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

[mmattel]

@mirekys :wave: we would like to proceed with this PR. To do so, it would be great if you could add the statement as suggested by @micbar above. It is totaly fine to just drop a comment like: This code change is made under MIT license. Many thanks.

[mirekys]

This code change is made under MIT license

[mirekys]

I also hopefully addressed all of the change requests made by @DeepDiver1975

[DeepDiver1975]

Unless I miss something: there still seems to be a different setup for the update scenario.

Why would the initial provisioning use different attributes compared to the update case?

Maintaining two configs is error prone for my understanding.

This has already been asked for in https://github.com/owncloud/openidconnect/pull/222#discussion_r836773549

Please let me know if there are any questions or missunderstandings - thx

[mirekys]

I reduced and moved the configs needed for the update scenario to the following config options: https://github.com/owncloud/openidconnect/pull/222/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R130-R136

Everything should now be under an already existing auto-provision config section, and the current state of the PR only adds a way to toggle auto-update mode and to tell, which of the user provisioning attributes should be auto-updated (e.g. one may want to configure only email attribute to be auto-updated while leaving displayName untouched after the first-time provisioning). Apart from this, it shouldn't be using any different provisioning attribute configs just for doing auto-updates.

Could you please point me to the code parts where it still causes misunderstandings, or maybe specify, how the configuration for this should look? Thanks a lot :)

[DeepDiver1975]

Could you please point me to the code parts where it still causes misunderstandings, or maybe specify, how the configuration for this should look? Thanks a lot :)

https://github.com/owncloud/openidconnect/pull/222/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R135

Config example in readme still has attributes under update. THX

[mirekys]

Could you please point me to the code parts where it still causes misunderstandings, or maybe specify, how the configuration for this should look? Thanks a lot :)

https://github.com/owncloud/openidconnect/pull/222/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R135

Config example in readme still has attributes under update. THX

Could you please point me to the code parts where it still causes misunderstandings, or maybe specify, how the configuration for this should look? Thanks a lot :)

https://github.com/owncloud/openidconnect/pull/222/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R135

Config example in readme still has attributes under update. THX

Yep, but this update->attributes option has a different meaning from auto-provision options (mappings of claims to user attributes) and is related to auto-update mode only. It is just a whitelist of user attributes, that will be updated with fresh oidc data upon each user log-in. Any other user attribute not in this list won't be touched after the initial user provisioning.

We could simplify it by throwing it away and saying that if the update mode is enabled, we simply update all of the user attributes upon each log-in.

Should I move/rename the option to something else that better describes its meaning? Or does it sound better to go with the simplest solution for now and drop it? 🙂

[DeepDiver1975]

Yep, but this update->attributes option has a different meaning from auto-provision options (mappings of claims to user attributes) and is related to auto-update mode only. It is just a whitelist of user attributes, that will be updated with fresh oidc data upon each user log-in. Any other user attribute not in this list won't be touched after the initial user provisioning.

We could simplify it by throwing it away and saying that if the update mode is enabled, we simply update all of the user attributes upon each log-in.

Should I move/rename the option to something else that better describes its meaning? Or does it sound better to go with the simplest solution for now and drop it? slightly_smiling_face

Now I understand - thanks! Sorry took a while to understand. :see_no_evil:

What would be a reasonable scenario where only some attributes shall be updated?

If there is none - I'd vote for removing this attributes whitelist - THX a lot

[mirekys]

Yep, but this update->attributes option has a different meaning from auto-provision options (mappings of claims to user attributes) and is related to auto-update mode only. It is just a whitelist of user attributes, that will be updated with fresh oidc data upon each user log-in. Any other user attribute not in this list won't be touched after the initial user provisioning. We could simplify it by throwing it away and saying that if the update mode is enabled, we simply update all of the user attributes upon each log-in. Should I move/rename the option to something else that better describes its meaning? Or does it sound better to go with the simplest solution for now and drop it? slightly_smiling_face

Now I understand - thanks! Sorry took a while to understand. 🙈

What would be a reasonable scenario where only some attributes shall be updated?

If there is none - I'd vote for removing this attributes whitelist - THX a lot

It may make sense after the user group membership auto-update is implemented, as one may want to handle group memberships of users differently (e.g. manually/statically). In such cases preventing it from being constantly auto-updated from oidc would be desirable. But as of current implementation, I see the point that this option is unnecessary & excessive

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

[jnweiger]

Hmm, is #278 something we overlooked here?