[full-ci] Expose group's displayname

[jvillafanez]

Expose the group's displayname and store the group id in the "owncloud_name" column of the "oc_ldap_group_mapping" table.

For new installations
By default, the "owncloud_name" will likely be entryuuid (at least checked with openldap, need to test with more providers). This will guarantee uniqueness. The default displayname will still be the cn.

TODO (in this PR):

• Configurable ldap attribute for the "owncloud_name" in the expert tab of the wizard. The restrictions will be the same as the username: immutable, unique and only a-zA-Z0-9+_.@- chars allowed
• Migration for updates. We'll force the configured displayname to be used instead of the default one.
• Caching. For now, the displayname isn't cached. We probably should cache it at least to keep consistency with the rest of the app. Right now, it seems the updating the displayname in the LDAP automatically updates the name shown in ownCloud, but this might be the only case, so we might want consistency. Moreover, caching will reduce the hits the ldap server needs to take.

Ref: https://github.com/owncloud/enterprise/issues/5071

[ownclouders]

💥 Acceptance tests pipeline apiProvisioningLDAP-master-mysql8.0-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/4056/25

[jvillafanez]

We need to decide if we raise the minimum version or not. The code is technically compatible with previous core versions, but by default it will show the uuid of the group which will look ugly. We'll need to verify, but the cases should be:

• OC 10.10- and user_ldap 0.16.0 => current behavior, it shows the cn
• OC 10.10- and user_ldap 0.17.0 (fresh) => it shows the uuid by default unless other attribute was configured. It can't be changed after it's set up.
• OC 10.10- and user_ldap 0.17.0 (updated) => it shows the cn due to migration, in order to keep the behavior from 0.16.0
• OC 10.11+ and user_ldap 0.16.0 => it shows the cn
• OC 10.11+ and user_ldap 0.17.0 (fresh) => it shows the cn. The internal groupname will be the uuid unless configured otherwise
• OC 10.11+ and user_ldap 0.17.0 (updated) => it shows the cn. The internal groupname will be the cn due to the migration. This can't be changed. The group displayname can be changed if needed.

The cases to check are the second and third. Second case looks ugly, but it should work. Note that OC 10.10 doesn't show the displayname of the group in most cases.

In addition, we've added caching for the group details. You might need to clear the cache (if it's being used) for the changes to appear.

[jvillafanez]

@phil-davis I think we'll need to adjust the tests here. The provisioning API uses the gid, which is mapped to the entryuuid by default in this PR.
The alternative is to set the new expert groupname attribute to use the old cn. That should make the tests pass.

[phil-davis]

"It has the same restrictions as the internal username" - but probably a larger set of characters will work.
Do we try to say what characters are allowed?

[jvillafanez]

The internal username has a-zA-Z0-9+_.@- explicitly written, so those should be ok.
As you've said, a larger char set will likely work, but to keep it on the safe side, I'd rather keep the char set small so we don't face issues with weird chars.
The main idea is that it will behave the same way as the internal username, so it makes sense to have the same restrictions. We might want to include the whitespace explicitly in the char set because it's a common char, but other than that, I think it's fine as it is.

[phil-davis]

The core daily-master-qa tarball was a week out of date. That was noticed today and it has been recreated a few hours ago. So it now has the latest code from core master, including from PR owncloud/core#40229

So I restarted CI just now. That will get a fresh result, and then I will look at what tests fail and how we can fix them.

[phil-davis]

@jvillafanez see my comment #749 (comment)

There is a problem with group names that have "special characters" in them. Probably the group id or display name needs some escaping somewhere in the code - can you have a look?

I adjusted the test settings in commit 8ba4029 - now it defines ldapExpertGroupnameAttr in the config. And most of the test pass, which is good. I think that in CI the LDAP app was being installed, and occ upgrade happened that runs the migration. But that happens before the settings are created - the CI creates a whole set of settings with tests/acceptance/setConfig.sh and now those have to include ldapExpertGroupnameAttr

For installation that are already set up, the migration code should work nicely.

For new installations, ldapExpertGroupnameAttr has to be set. Maybe it would be a bit easier if, when it is not set, it defaults to using the group id as the group display name. That would avoid making everybody have to set ldapExpertGroupnameAttr

When we get all the tests passing, then I can sort out some scenarios in a separate test suite that has the group id and group display name different.

[jvillafanez]

For new installations, ldapExpertGroupnameAttr has to be set.

It should be already defaulting to the entryuuid (or any other uuid attribute) of the group. The behavior should be the same as with the username.

[phil-davis]

I pushed commit 8b21e28 to my test branch.
That avoids a problem that I noticed:
Trying to access array offset on value of type bool at /home/phil/git/owncloud/core/apps-external/user_ldap/lib/Group_LDAP.php#1022

That seems to happen because getGroupDetails can be called even when the group does not exist - see my comments in the "Fixme" of that commit. Can you have a look at those code paths and make an appropriate fix to this PR.

[jvillafanez]

I pushed commit 8b21e28 to my test branch.

It might be better to return null if the displayname isn't found and let core handle it.

if (!\is_array($displayname)) {
  // displayname not found
  return null;
}

[phil-davis]

It might be better to return null if the displayname isn't found and let core handle it.

core owncloud/core#40261 refactored getGroupObject so that it should only call into getGroupDetails when the group exists in the backend. So that should prevent the case where you commented FIXME: It seems local groups also end up going through here...

And some of my comments in 8b21e28 are also now out-of-date.

So yes, add your suggestion to return null if the display name is not found, if you like.

Then I think that this PR is ready.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

[jnweiger]

Notes to self:

• There seems to be an unreleased 0.17.0 in circulation (seen in LDAP Mapping Collision of 1 user prevents all new LDAP users from being synced into ownCloud #751 ), so the next release should probably be 0.18.0 to be on the safe side.
• Release after 10.11.0 - no prio (input from @hodyroff ) or even after 10.12 -- see below.

[pako81]

Is there anything open here? /cc @enbrnz

[jvillafanez]

Maybe QA can confirm if we should rise the minimum version to 10.12 due to owncloud/core#40412 . Other than that, I think @mrow4a needs to give the green light

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

0.0% Coverage
0.0% Duplication

[jvillafanez]

I think #748 (comment) still applies.
Basically, problematic cases seems to be with core 10.11 and earlier, and ldap with this PR, in a fresh installation. The problem with a fresh 10.10 core (and earlier) is explained in the linked comment. A fresh 10.11 will be affected by owncloud/core#40412.

Options are:

• Document the limitation without raising the minimum version. If people don't want problems, they'll likely need to change the new groupname id attribute, which might not happen.
• Rise the minimum version to 10.12 (or 10.11.1 or whatever next version is) so people won't be affected.

Note that the problem only happens in new installations. There is a migration in this PR in so the updates will keep the current behavior.

[mrow4a]

@jvillafanez @cdamken I think the option agreed was no. 1 -> release new user_ldap that includes this PR AFTER oc10.12 is released.

If there is corner case customer (with new instalation on oc10.11) like you explained, we would just recommend to upgrade to oc10.12 to have this feature on new user_ldap working. I think raising minimal version is a bit too much here.

[pako81]

I do agree with @mrow4a's comment, probably raising core minimum version is too much and be sure to release user_ldap 0.17.0 after next core version + properly document this should be already enough.

• OC 10.10- and user_ldap 0.17.0 (fresh) => it shows the uuid by default unless other attribute was configured. It can't be changed after it's set up. --> this would be solved with an upgrade to 10.12.0

• OC 10.11 and user_ldap 0.17.0 (fresh) => it shows the cn. The internal groupname will be the uuid unless configured otherwise --> it is configurable and Files_external mount config will show group displaynames core#40412 would be solved with an upgrade to 10.12.0

[phil-davis]

@jvillafanez GitHub is reporting conflicts in appinfo/info.xml
Can you rebase some time? (it should be very easy to resolve)

[jvillafanez]

Conflicts in the info.xml were due to the different app version. I've set the version to 0.17.0 because we'll have to run the migration included in the PR and I'm not sure if it would run with just a minor version update.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

4.3% Coverage
0.0% Duplication

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

4.3% Coverage
0.0% Duplication