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

[10.0.0] "User Search Attributes" does not search in those attributes #96

Closed
cdamken opened this issue May 10, 2017 · 14 comments
Closed

Comments

@cdamken
Copy link
Contributor

cdamken commented May 10, 2017

Steps to reproduce it:

Add this attributes in User Search Attributes

name
sn
displayName
givenName
distinguishedName
userPrincipalName
uSNChanged

Expected behavior:

Look for : 8814486 who is the uSNChanged attribute from one user and show that user

Actual behavior:

Look for : 8814486 who is the uSNChanged attribute from one user and No users Found

This is REALLY important to keep the same behavior as before!

@tomneedham @felixboehm @jvillafanez @butonic

@tomneedham
Copy link
Contributor

From rough research, old functionality was to call each user backend with the search:
https://github.com/owncloud/core/blob/stable9.1/lib/private/User/Manager.php#L219-L228
Current status, just search in accounts table:
https://github.com/owncloud/core/blob/master/lib/private/User/Manager.php#L232-L241

@tomneedham
Copy link
Contributor

And @DeepDiver1975 for accounts table expertise

@tomneedham
Copy link
Contributor

IDEA: Do we need to pre-cache these search attributes in a accounts table column and let core search this column? Each backend provides a 'search' string with concatenated values? i.e: db users give '$email $displayname $userid' and maybe LDAP gives implode(' ',$searchattributes) ?

@tomneedham
Copy link
Contributor

With owncloud/core#27832 we search displayname, user_id (login) and email which covers a lot of uses since ldap attributes are used to map these values.

@DeepDiver1975
Copy link
Member

This is a regression which we cannot fix quickly. Why are all these different attributes necessary anyhow?

@jvillafanez
Copy link
Member

jvillafanez commented May 10, 2017

This is a change in the behaviour, but it's consistent with the whole user page. All operations are performed against the account table. If we're listing whatever accounts are in that table, is perfectly fine that the search is performed against it.
It doesn't make sense that you search for a user that you aren't listing.

If this isn't fine, then we shouldn't list what is in the accounts table but ask each backend for the users.

@tomneedham
Copy link
Contributor

It doesn't make sense that you search for a user that you aren't listing.

Perhaps, but yes, this is a change from previous behaviour, and we certainly should remove the UI + backend stuff that doesn't do anything. Can you take care of this?

@tomneedham
Copy link
Contributor

Talked with @DeepDiver1975 and @butonic about this and studied the use cases for the search attributes. Many cases are covered by the accounts table mapping of displayname, email and user_id. The only loss of functionality is where custom attributes are used that are not included in displayname, email and user_id.

Suggest we do the following:

  • document change in search behaviour with LDAP in OC10.01
  • remove search attributes UI and backend .
  • implement search by email, displayname and user_id

@jvillafanez
Copy link
Member

Perhaps, but yes, this is a change from previous behaviour, and we certainly should remove the UI + backend stuff that doesn't do anything. Can you take care of this?

It does something. The occ ldap:search use them. Being a LDAP-only stuff I think it's fine to leave it as it is.

@butonic
Copy link
Member

butonic commented May 11, 2017

I think we can postpone adding an additional search column because there is a workaround that should cover some cornercases: you can set the (2nd User Displayname Field)[https://doc.owncloud.org/server/10.0/admin_manual/configuration_user/user_auth_ldap.html#directory-settings] to anything you want, eg: uSNChanged. When syncing the users the displayname in the account table will become <displayname> (<2nd-displayname>) and finding users by the second username becomes possible.

  • In the LDAP settings we should kill the search attributes in the ui because they aren't used any longer. Only by occ ldap:search but that command should instead use the attributes configured as the displayname, 2nd displayname, email and uuid.
  • after 10.0.1 add a new table for the uuid to account id mapping, add a ui field for the login name and make sure the uuid is a uuid and NOT samaccountname or userprincipalname.

@cdamken searching by displayname, email or login should allow end users to find and distinguish all users they know. Why is it REALLY important that searching by uSNChanged works? Configure it as the 2nd displayname and you will be able to find users. But ... please explain the use case. As far as we can tell end users don't know technical names. Probably the login name, but that shouldn't even be known to everyone because it weakens security (a little).

@tomneedham We already have three ILIKEs in the query and I am starting to get concerned about the performance if 100000k accounts are in the table... did you by chance test the search performance in this case?

@cdamken
Copy link
Contributor Author

cdamken commented May 11, 2017

Why is it REALLY important that searching by uSNChanged works?

It was just an example from testing with our AD, but customer uses attributtes like gecos, second last name, description, etc.

@butonic
Copy link
Member

butonic commented May 18, 2017

@cdamken do you need to be able to find 'Alice' when searching by 'lic' or is it enough if your query always has to start like the possiple search terms. So 'Ali' to find 'Alice'.

@cdamken
Copy link
Contributor Author

cdamken commented May 18, 2017

@cdamken do you need to be able to find 'Alice' when searching by 'lic' or is it enough if your query always has to start like the possiple search terms. So 'Ali' to find 'Alice'.

Actually owncloud don't do that now, otherwise the search will be like *search*
oC only does for now search*

As an example from one of the customers:

they use:

Carlos Manuel Damken Castell

sn
second_surename
principal_name

sn= Damken
second_surename= Castell
principal_name= Carlos Manuel

Looking for Manuel is not needed not.

But, if so, they have to create an extra attribute.

@butonic
Copy link
Member

butonic commented May 23, 2017

searching in additional search fields fixed in 10.0.1. for further improvements add new issues.

@butonic butonic closed this as completed May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants