feat: account info auto-update #222

mirekys · 2022-03-28T15:39:04Z

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 Update user information during logging in #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:

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

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
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:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

...

CLAassistant · 2022-03-28T15:45:28Z

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.}

DeepDiver1975

I'd move the account update logic into the auto provisioning service - the core change will be much less then and things belonging together will stay in the place where they belong.

besides that: THX a lot! 🙏

DeepDiver1975 · 2022-03-28T19:26:06Z

README.md

+      // enable the user info auto-update mode
+      'enabled' => true, 
+      // defines account attributes that will be auto-updated
+      'attributes' => ['email', 'display-name'],


using different setup for auto-provisioning and auto-update sounds really spooky.

from my pov update should use the same setup as the initial user creations - anything else seems wrong to me.

DeepDiver1975 · 2022-03-28T19:27:31Z

lib/Service/AccountUpdateService.php

@@ -0,0 +1,89 @@
+<?php
+/**
+ * @author Thomas Müller <[email protected]>


please update

DeepDiver1975 · 2022-03-28T19:27:38Z

lib/Service/AccountUpdateService.php

+<?php
+/**
+ * @author Thomas Müller <[email protected]>
+ * @author Ilja Neumann <[email protected]>


DeepDiver1975 · 2022-03-28T19:27:51Z

lib/Service/AccountUpdateService.php

+ * @author Thomas Müller <[email protected]>
+ * @author Ilja Neumann <[email protected]>
+ *
+ * @copyright Copyright (c) 2020, ownCloud GmbH


mmattel · 2022-05-17T09:34:13Z

@mirekys 👋 any update on this ?

micbar · 2022-05-22T18:20:22Z

@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.

sonarqubecloud · 2022-06-07T08:36:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

mmattel · 2022-07-05T07:11:55Z

@mirekys 👋 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 · 2022-07-07T11:52:20Z

This code change is made under MIT license

mirekys · 2022-07-07T11:58:34Z

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

DeepDiver1975 · 2022-07-08T09:01:03Z

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 #222 (comment)

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

mirekys · 2022-07-08T12:45:09Z

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 · 2022-07-08T13:31:26Z

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 · 2022-07-11T10:35:20Z

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 · 2022-07-11T10:52:37Z

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

mirekys · 2022-07-11T11:13:35Z

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

sonarqubecloud · 2022-07-11T13:16:33Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

jnweiger · 2023-01-13T12:24:05Z

Hmm, is #278 something we overlooked here?

mirekys added 3 commits March 28, 2022 11:48

Merge remote-tracking branch 'upstream/master' into autoupdate-userinfo

578046a

implement account info auto-update

9c016bf

fix servicename in testEnabled

abcb2fd

mirekys requested a review from DeepDiver1975 as a code owner March 28, 2022 15:39

mirekys added 3 commits March 28, 2022 18:20

phpstyle fix

4a7764c

fix missing bucks

79cf21a

trigger update from login flow

4eabf91

DeepDiver1975 requested changes Mar 28, 2022

View reviewed changes

backwards oc compatibility

c8d9b84

mirekys added 6 commits June 3, 2022 10:08

move update logic to auto provision service

7bf040e

update README auto-update docs

81acdad

fix autoformat mistakes

89f1c37

fixing tests

f86eea5

fix tests

3d2494c

fix phpstyle issues

c8c9c50

micbar requested a review from DeepDiver1975 July 7, 2022 13:04

This was referenced Jul 7, 2022

openidconnect: Account info auto-update owncloud/docs-server#514

Closed

Update config.apps.sample - openidconnect auto-update owncloud/core#40186

Merged

remove updated attributes config

da0edea

DeepDiver1975 changed the title ~~Account info auto-update~~ feat: account info auto-update Jul 12, 2022

DeepDiver1975 merged commit 783202d into owncloud:master Jul 12, 2022

DeepDiver1975 mentioned this pull request Jul 13, 2022

fix: auto update function return #239

Merged

jnweiger mentioned this pull request Jul 13, 2022

openidconnect 2.2.0 #241

Closed

42 tasks

DeepDiver1975 mentioned this pull request Oct 4, 2022

[QA] autoprovisioning fails with 'picture-claim' => 'picture' #138

Closed

nodens mentioned this pull request Dec 21, 2023

Regression ? upgrade to 2.2.0 via market makes it unable to login with OpenID #283

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: account info auto-update #222

feat: account info auto-update #222

mirekys commented Mar 28, 2022 •

edited

Loading

CLAassistant commented Mar 28, 2022

DeepDiver1975 left a comment

DeepDiver1975 Mar 28, 2022

DeepDiver1975 Mar 28, 2022

DeepDiver1975 Mar 28, 2022

DeepDiver1975 Mar 28, 2022

mmattel commented May 17, 2022

micbar commented May 22, 2022 •

edited

Loading

sonarqubecloud bot commented Jun 7, 2022

mmattel commented Jul 5, 2022

mirekys commented Jul 7, 2022

mirekys commented Jul 7, 2022

DeepDiver1975 commented Jul 8, 2022

mirekys commented Jul 8, 2022 •

edited

Loading

DeepDiver1975 commented Jul 8, 2022

mirekys commented Jul 11, 2022 •

edited

Loading

DeepDiver1975 commented Jul 11, 2022

mirekys commented Jul 11, 2022 •

edited

Loading

sonarqubecloud bot commented Jul 11, 2022

jnweiger commented Jan 13, 2023

feat: account info auto-update #222

feat: account info auto-update #222

Conversation

mirekys commented Mar 28, 2022 • edited Loading

WIP

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Open tasks:

CLAassistant commented Mar 28, 2022

DeepDiver1975 left a comment

Choose a reason for hiding this comment

DeepDiver1975 Mar 28, 2022

Choose a reason for hiding this comment

DeepDiver1975 Mar 28, 2022

Choose a reason for hiding this comment

DeepDiver1975 Mar 28, 2022

Choose a reason for hiding this comment

DeepDiver1975 Mar 28, 2022

Choose a reason for hiding this comment

mmattel commented May 17, 2022

micbar commented May 22, 2022 • edited Loading

sonarqubecloud bot commented Jun 7, 2022

mmattel commented Jul 5, 2022

mirekys commented Jul 7, 2022

mirekys commented Jul 7, 2022

DeepDiver1975 commented Jul 8, 2022

mirekys commented Jul 8, 2022 • edited Loading

DeepDiver1975 commented Jul 8, 2022

mirekys commented Jul 11, 2022 • edited Loading

DeepDiver1975 commented Jul 11, 2022

mirekys commented Jul 11, 2022 • edited Loading

sonarqubecloud bot commented Jul 11, 2022

jnweiger commented Jan 13, 2023

mirekys commented Mar 28, 2022 •

edited

Loading

micbar commented May 22, 2022 •

edited

Loading

mirekys commented Jul 8, 2022 •

edited

Loading

mirekys commented Jul 11, 2022 •

edited

Loading

mirekys commented Jul 11, 2022 •

edited

Loading