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

Add additional user profile attributes from LDAP #41793

Closed
wants to merge 12 commits into from

Conversation

slapcat
Copy link
Contributor

@slapcat slapcat commented Nov 28, 2023

Summary

  • Extend user_ldap User Profile Attributes to include birthday and anniversary.
  • Update CardDAV converter to include these attributes in contacts for the Accounts address book.
  • Add required constants to AccountManager.

Checklist

@slapcat slapcat force-pushed the ldap_profile_attributes branch 3 times, most recently from 14e328f to 3cad7c7 Compare December 2, 2023 18:31
@kesselb
Copy link
Contributor

kesselb commented Dec 2, 2023

Thank you 👍

Will try to give it a test next week.

I see a few failing tests.
The following patch should fix one of them.

Index: tests/lib/Accounts/AccountManagerTest.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php
--- a/tests/lib/Accounts/AccountManagerTest.php	(revision 7eaa573bac0f82b5b2b837104a508dab37c6e231)
+++ b/tests/lib/Accounts/AccountManagerTest.php	(date 1701547210963)
@@ -632,6 +632,18 @@
 				'scope' => IAccountManager::SCOPE_LOCAL,
 			],
 
+			[
+				'name' => IAccountManager::PROPERTY_BIRTHDATE,
+				'value' => '',
+				'scope' => IAccountManager::SCOPE_LOCAL,
+			],
+
+			[
+				'name' => IAccountManager::PROPERTY_ANNIVERSARYDATE,
+				'value' => '',
+				'scope' => IAccountManager::SCOPE_LOCAL,
+			],
+
 			[
 				'name' => IAccountManager::PROPERTY_PROFILE_ENABLED,
 				'value' => '1',

@kesselb
Copy link
Contributor

kesselb commented Dec 2, 2023

This should fix the second failing tests by updating the expected data.

Index: apps/settings/tests/UserMigration/assets/account.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/apps/settings/tests/UserMigration/assets/account.json b/apps/settings/tests/UserMigration/assets/account.json
--- a/apps/settings/tests/UserMigration/assets/account.json	(revision 7eaa573bac0f82b5b2b837104a508dab37c6e231)
+++ b/apps/settings/tests/UserMigration/assets/account.json	(date 1701548042464)
@@ -1,1 +1,0 @@
-{"displayname":{"name":"displayname","value":"Emma Jones","scope":"v2-federated","verified":"0","verificationData":""},"address":{"name":"address","value":"920 Grass St","scope":"v2-local","verified":"0","verificationData":""},"website":{"name":"website","value":"","scope":"v2-local","verified":"0","verificationData":""},"email":{"name":"email","value":"","scope":"v2-federated","verified":"0","verificationData":""},"avatar":{"name":"avatar","value":"","scope":"v2-federated","verified":"0","verificationData":""},"phone":{"name":"phone","value":"","scope":"v2-local","verified":"0","verificationData":""},"twitter":{"name":"twitter","value":"","scope":"v2-local","verified":"0","verificationData":""},"fediverse":{"name":"fediverse","value":"","scope":"v2-local","verified":"0","verificationData":""},"organisation":{"name":"organisation","value":"","scope":"v2-local","verified":"0","verificationData":""},"role":{"name":"role","value":"","scope":"v2-local","verified":"0","verificationData":""},"headline":{"name":"headline","value":"","scope":"v2-local","verified":"0","verificationData":""},"biography":{"name":"biography","value":"","scope":"v2-local","verified":"0","verificationData":""},"profile_enabled":{"name":"profile_enabled","value":"1","scope":"v2-local","verified":"0","verificationData":""},"additional_mail":[]}
\ No newline at end of file
Index: apps/settings/tests/UserMigration/assets/account-complex.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/apps/settings/tests/UserMigration/assets/account-complex.json b/apps/settings/tests/UserMigration/assets/account-complex.json
--- a/apps/settings/tests/UserMigration/assets/account-complex.json	(revision 7eaa573bac0f82b5b2b837104a508dab37c6e231)
+++ b/apps/settings/tests/UserMigration/assets/account-complex.json	(date 1701548067344)
@@ -1,1 +1,0 @@
-{"displayname":{"name":"displayname","value":"Steve Smith","scope":"v2-local","verified":"0","verificationData":""},"address":{"name":"address","value":"123 Water St","scope":"v2-local","verified":"0","verificationData":""},"website":{"name":"website","value":"https:\/\/example.org","scope":"v2-local","verified":"0","verificationData":""},"email":{"name":"email","value":"[email protected]","scope":"v2-federated","verified":"0","verificationData":""},"avatar":{"name":"avatar","value":"","scope":"v2-local","verified":"0","verificationData":""},"phone":{"name":"phone","value":"+12178515387","scope":"v2-private","verified":"0","verificationData":""},"twitter":{"name":"twitter","value":"steve","scope":"v2-federated","verified":"0","verificationData":""},"fediverse":{"name":"fediverse","value":"@[email protected]","scope":"v2-federated","verified":"0","verificationData":""},"organisation":{"name":"organisation","value":"Mytery Machine","scope":"v2-private","verified":"0","verificationData":""},"role":{"name":"role","value":"Manager","scope":"v2-private","verified":"0","verificationData":""},"headline":{"name":"headline","value":"I am Steve","scope":"v2-local","verified":"0","verificationData":""},"biography":{"name":"biography","value":"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Mauris porttitor ullamcorper dictum. Sed fermentum ut ligula scelerisque semper. Aliquam interdum convallis tellus eu dapibus. Integer in justo sollicitudin, hendrerit ligula sit amet, blandit sem.\n\nSuspendisse consectetur ultrices accumsan. Quisque sagittis bibendum lectus ut placerat. Mauris tincidunt ornare neque, et pulvinar tortor porttitor eu.","scope":"v2-local","verified":"0","verificationData":""},"profile_enabled":{"name":"profile_enabled","value":"1","scope":"v2-local","verified":"0","verificationData":""},"additional_mail":[{"name":"additional_mail","value":"[email protected]","scope":"v2-published","verified":"0","verificationData":""},{"name":"additional_mail","value":"[email protected]","scope":"v2-local","verified":"0","verificationData":""}]}
\ No newline at end of file

@slapcat
Copy link
Contributor Author

slapcat commented Dec 5, 2023

Side note: I thought the biography field would be converted to the carddav notes, but see now it is not and has no correlated field for the contact card. @kesselb would it be okay if I add this to this PR, or is this kept separate for a reason?

@kesselb
Copy link
Contributor

kesselb commented Dec 5, 2023

Side note: I thought the biography field would be converted to the carddav notes, but see now it is not and has no correlated field for the contact card. @kesselb would it be okay if I add this to this PR, or is this kept separate for a reason?

Good catch, sounds good to me.

@miaulalala @ChristophWurst mapping the about field from our profile, which should be populated from the ldap biography field if available, to the notes field in vcard. Wdyt?

@ChristophWurst
Copy link
Member

I think we should map LDAP attributes to profile properties, and profile properties to the system contact. Then users can set the scope of the birthday and anniversary fields.

Without a scope setting we either

  1. Have to make the property private, which means nobody will see it so it's useless.
  2. Have to make the property local, which means all other users can see your birthday, even if you don't want to expose this.

@slapcat slapcat force-pushed the ldap_profile_attributes branch 2 times, most recently from cc38d6a to 5674b2d Compare December 27, 2023 18:01
@slapcat
Copy link
Contributor Author

slapcat commented Jan 4, 2024

I added back the scoping as suggested to the code I already wrote. I'm not as familiar with the frontend and would find it difficult to add these features correctly to the profile page. Can someone assist with that portion? Or can we split that into a separate PR?

Signed-off-by: Jake Nabasny <[email protected]>
Signed-off-by: Jake Nabasny <[email protected]>
Remove properties from PersonalInfo, since not visible in profile

Signed-off-by: Jake Nabasny <[email protected]>
- Add back including the birthdate and anniversary in the profile

Signed-off-by: Jake Nabasny <[email protected]>
Signed-off-by: Jake Nabasny <[email protected]>
@slapcat slapcat force-pushed the ldap_profile_attributes branch from 7cbd75a to 459355f Compare February 5, 2024 17:50
This reverts commit 459355f.

Signed-off-by: Jake Nabasny <[email protected]>
@st3iny
Copy link
Member

st3iny commented Feb 19, 2024

Hey @slapcat, a colleague of mine and me hacked together a birthday field and added it to the profile a while ago. Perhaps the frontend code could be reused here to allow configuring the birthday and set the scope.

There is also working backend code for saving a user's birthday property.

The frontend is mostly in:
https://github.com/nextcloud/server/blob/feat/account/add-birthday-prop/apps/settings/src/components/PersonalInfo/BirthdaySection.vue

The full PR is at: #39592

@skjnldsv skjnldsv added the 2. developing Work in progress label Feb 21, 2024
@susnux susnux added this to the Nextcloud 30 milestone Apr 18, 2024
@st3iny st3iny self-assigned this Apr 29, 2024
@st3iny
Copy link
Member

st3iny commented Jun 3, 2024

The alternative PR including a documentation PR are merged.

Ref #45512
Ref nextcloud/documentation#11323

@slapcat
Copy link
Contributor Author

slapcat commented Jun 3, 2024

Thanks @st3iny !

@slapcat slapcat closed this Jun 3, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
Development

Successfully merging this pull request may close these issues.

More LDAP User Profile Attributes
6 participants