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

Searching sharee with displayname #547

Closed
jesmrec opened this issue Sep 10, 2020 · 15 comments · Fixed by #4229
Closed

Searching sharee with displayname #547

jesmrec opened this issue Sep 10, 2020 · 15 comments · Fixed by #4229

Comments

@jesmrec
Copy link

jesmrec commented Sep 10, 2020

Only posible to get sharees by using the username:

Username: einstein
Display Name: Albert Einstein

results are fetched only matching einstein
Albert, Einstein, Ein, Albe do not match and response is empty (always forgetting the case sensitiveness)

oC10: matches with both.

Backend: EOS storage via via https://github.com/owncloud-docker/compose-playground/blob/master/examples/hetzner-deploy/make_ocis_eos_compose_test.sh
Version: v1.0.0-rc1
Client: iOS App v11.4 over iOS13

CC @pmaier1

@refs
Copy link
Member

refs commented Jan 13, 2021

I added the EOS storage tag but most likely is related with the Reva userprovider.

@micbar
Copy link
Contributor

micbar commented Jul 14, 2022

re tested:

Created User

Username: hans
Display Name: Peter Dampf
E-Mail: pd@example.com

We can find:

hans
Peter
pd@

We cannot find:

Hans (should not be case sensitive)
ans(middle of the username)
Dampf (End of the string Displayname)
example.com (End of the email address)

Expectation

at least
Hans (username search should not be case sensitive)
Dampf (End or middle of the string Displayname)

Should work.

@micbar micbar added this to Qualification in Infinite Scale Team Board via automation Jul 14, 2022
@micbar micbar added Priority:p2-high Escalation, on top of current planning, release blocker and removed Storage:EOS labels Jul 14, 2022
@micbar micbar moved this from Qualification to Bugs Prio 2 in Infinite Scale Team Board Jul 14, 2022
@micbar micbar added this to the 2.0.0 General Availability milestone Jul 14, 2022
@rhafer
Copy link
Contributor

rhafer commented Jul 14, 2022

Expectation

at least
Hans (username search should not be case sensitive)

This is caused by a bug in idm. Fix is on its way.

Dampf (End or middle of the string Displayname)

This is currently not working "on purpose". At some point we agreed on limiting search to prefix searches. As doing full substring searches will get pretty expensive easily. We can of course change that (just need to adjust the filter https://github.com/cs3org/reva/blob/75f2c2f836578f8280749569f7f01a3e7d2b5939/pkg/utils/ldap/identity.go#L480).

@micbar If you still want it. Should we maybe make that configurable?

@rhafer
Copy link
Contributor

rhafer commented Jul 14, 2022

Hans (username search should not be case sensitive)

This part should be fixed with libregraph/idm#75

@rhafer rhafer moved this from Bugs Prio 2 to In progress in Infinite Scale Team Board Jul 14, 2022
@rhafer rhafer self-assigned this Jul 14, 2022
@micbar
Copy link
Contributor

micbar commented Jul 14, 2022

@micbar If you still want it. Should we maybe make that configurable?

That would be awesome, same for the character length.

@rhafer
Copy link
Contributor

rhafer commented Jul 19, 2022

same for the character length.

@micbar The API does not enforce any limit on the minimum (or maximum) length of the search phrase. Can you elaborate on what you are referring to?

@micbar
Copy link
Contributor

micbar commented Jul 19, 2022

Currently the system starts searching for users after you enter 3 characters. This is fine as a default.

Some use cases and the test suite would like to use at least 2 chars to start the user enumeration. I would like to have that configurable. Is that possible?

@phil-davis
Copy link
Contributor

For example, oC10 has:

'user.search_min_length' => 2,

So the default on oC10 is to start searching for matches after 2 characters are typed in.

For test scenarios, it will be easier if we can get oCIS to start with this set to 2 characters (either change the oCIS default from 3 to 2, or have a way that we can configure that when we start oCIS in CI - I don't really care)

There are some scenarios that change the setting on-the-fly and check that different values work. Tht is out-of-scope for oCIS at the moment - we don't have a general way to modify system-wide settings on-the-fly in oCIS, so those sort of test scenarios will have to be re-engineered anyway if there comes a time when we actually want to test such things.

@pmaier1
Copy link
Contributor

pmaier1 commented Jul 19, 2022

Configurable would be great, default like oC10 (2).

rhafer added a commit to rhafer/ocis that referenced this issue Jul 19, 2022
This includes a fix for case-insensitive substring filtering

Partial-Fix: owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 19, 2022
This introduces new settings for the users and groups services.
"group_substring_filter_type" for the group services and
"user_substring_filter_type" for the users service. They allow to set
the type of LDAP filter that is used for substring user searches.
Possible values are: "initial", "final" and "any" to do either prefix,
suffix or full substring searches.

Fixes owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 19, 2022
This adds the "search_min_length" setting to the frontend service which
allows to set the search_min_length capabilty which is e.g. used by
web.

Partial: owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 19, 2022
This includes a fix for case-insensitive substring filtering

Partial-Fix: owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 19, 2022
This introduces new settings for the users and groups services.
"group_substring_filter_type" for the group services and
"user_substring_filter_type" for the users service. They allow to set
the type of LDAP filter that is used for substring user searches.
Possible values are: "initial", "final" and "any" to do either prefix,
suffix or full substring searches.

Fixes owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 19, 2022
This adds the "search_min_length" setting to the frontend service which
allows to set the search_min_length capabilty which is e.g. used by
web.

Partial: owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 20, 2022
This includes a fix for case-insensitive substring filtering

Partial-Fix: owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 20, 2022
This introduces new settings for the users and groups services.
"group_substring_filter_type" for the group services and
"user_substring_filter_type" for the users service. They allow to set
the type of LDAP filter that is used for substring user searches.
Possible values are: "initial", "final" and "any" to do either prefix,
suffix or full substring searches.

Fixes owncloud#547
rhafer added a commit to rhafer/ocis that referenced this issue Jul 20, 2022
This adds the "search_min_length" setting to the frontend service which
allows to set the search_min_length capabilty which is e.g. used by
web.

Partial: owncloud#547
Infinite Scale Team Board automation moved this from In progress to Done Jul 20, 2022
rhafer added a commit that referenced this issue Jul 20, 2022
This includes a fix for case-insensitive substring filtering

Partial-Fix: #547
rhafer added a commit that referenced this issue Jul 20, 2022
This introduces new settings for the users and groups services.
"group_substring_filter_type" for the group services and
"user_substring_filter_type" for the users service. They allow to set
the type of LDAP filter that is used for substring user searches.
Possible values are: "initial", "final" and "any" to do either prefix,
suffix or full substring searches.

Fixes #547
rhafer added a commit that referenced this issue Jul 20, 2022
This adds the "search_min_length" setting to the frontend service which
allows to set the search_min_length capabilty which is e.g. used by
web.

Partial: #547
@SagarGi
Copy link
Member

SagarGi commented Jul 21, 2022

This PR got merged #4229 fixing this issue and needs a follow-up for the expected to fail.

#### [Searching sharee with displayname](https://github.com/owncloud/ocis/issues/547)
- [apiSharees/sharees.feature:350](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L350)
- [apiSharees/sharees.feature:351](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L351)
- [apiSharees/sharees.feature:370](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L370)
- [apiSharees/sharees.feature:371](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L371)
- [apiSharees/sharees.feature:390](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L390)
- [apiSharees/sharees.feature:391](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L391)
- [apiSharees/sharees.feature:410](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L410)
- [apiSharees/sharees.feature:411](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L411)
- [apiSharees/sharees.feature:430](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L430)
- [apiSharees/sharees.feature:431](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L431)

TODO QA-team

@SagarGi SagarGi reopened this Jul 21, 2022
Infinite Scale Team Board automation moved this from Done to In progress Jul 21, 2022
@SagarGi SagarGi moved this from In progress to Done in Infinite Scale Team Board Jul 21, 2022
@SagarGi SagarGi added QA:team and removed Type:Bug Priority:p2-high Escalation, on top of current planning, release blocker labels Jul 21, 2022
@SwikritiT SwikritiT self-assigned this Jul 21, 2022
@SwikritiT
Copy link
Contributor

SwikritiT commented Jul 21, 2022

The tests are failing because they search for the exact match for a searched string but this field is always empty for OCIS

<exact>
      <users/>
      <groups/>
      <remotes/>
    </exact>

For instance if I create a multiple variations of user einstein in OC10 and search it with curl

curl -vk -uadmin:admin http://localhost/core/ocs/v1.php/apps/files_sharing/api/v1/sharees\?search\=einstein\&itemType\=file -H"OCS-APIREQUEST:1" | xmllint --format -

It responds with

<?xml version="1.0"?>
<ocs>
  <meta>
    <status>ok</status>
    <statuscode>100</statuscode>
    <message>OK</message>
    <totalitems/>
    <itemsperpage/>
  </meta>
  <data>
    <exact>
      <users>
        <element>
          <label>einstein</label>
          <value>
            <shareType>0</shareType>
            <shareWith>einstein</shareWith>
            <userType>0</userType>
          </value>
        </element>
      </users>
      <groups/>
      <remotes/>
    </exact>
    <users>
      <element>
        <label>Albert Einstein</label>
        <value>
          <shareType>0</shareType>
          <shareWith>Albert Einstein</shareWith>
          <userType>0</userType>
        </value>
      </element>
      <element>
        <label>einstein-einstein</label>
        <value>
          <shareType>0</shareType>
          <shareWith>einstein-einstein</shareWith>
          <userType>0</userType>
        </value>
      </element>
      <element>
        <label>einstein1</label>
        <value>
          <shareType>0</shareType>
          <shareWith>einstein1</shareWith>
          <userType>0</userType>
        </value>
      </element>
    </users>
    <groups/>
    <remotes/>
  </data>
</ocs>

Inside <exact><users> there's exact match of the string

But in the case of OCIS if i do the same

curl -vk -uadmin:admin https://host.docker.internal:9200/ocs/v1.php/apps/files_sharing/api/v1/sharees\?search\=einstein\&itemType\=file -H"OCS-APIREQUEST:1" | xmllint --format - 
<?xml version="1.0" encoding="UTF-8"?>
<ocs>
  <meta>
    <status>ok</status>
    <statuscode>100</statuscode>
    <message>OK</message>
  </meta>
  <data>
    <exact>
      <users/>
      <groups/>
      <remotes/>
    </exact>
    <users>
      <element>
        <label>Albert Einstein</label>
        <value>
          <shareType>0</shareType>
          <shareWith>einstein</shareWith>
          <shareWithAdditionalInfo>einstein@example.org</shareWithAdditionalInfo>
        </value>
      </element>
      <element>
        <label>Einstein Albert</label>
        <value>
          <shareType>0</shareType>
          <shareWith>einstein-einstein</shareWith>
          <shareWithAdditionalInfo>a@c.com</shareWithAdditionalInfo>
        </value>
      </element>
      <element>
        <label>Einstein</label>
        <value>
          <shareType>0</shareType>
          <shareWith>einstein1</shareWith>
          <shareWithAdditionalInfo>a@b.com</shareWithAdditionalInfo>
        </value>
      </element>
    </users>
    <groups/>
    <remotes/>
  </data>
</ocs>

The exact match is rather given in <users>

curl -vk -uadmin:admin https://host.docker.internal:9200/ocs/v1.php/apps/files_sharing/api/v1/sharees\?search\=einstein-einstein\&itemType\=file -H"OCS-APIREQUEST:1" | xmllint --format -
<?xml version="1.0" encoding="UTF-8"?>
<ocs>
  <meta>
    <status>ok</status>
    <statuscode>100</statuscode>
    <message>OK</message>
  </meta>
  <data>
    <exact>
      <users/>
      <groups/>
      <remotes/>
    </exact>
    <users>
      <element>
        <label>Einstein Albert</label>
        <value>
          <shareType>0</shareType>
          <shareWith>einstein-einstein</shareWith>
          <shareWithAdditionalInfo>a@c.com</shareWithAdditionalInfo>
        </value>
      </element>
    </users>
    <groups/>
    <remotes/>
  </data>
</ocs>

The tests are looking inside exact and they don't find the expected value there.

Is this expected behaviour?

cc: @micbar @rhafer @phil-davis

@rhafer
Copy link
Contributor

rhafer commented Jul 22, 2022

@SwikritiT thanks for looking into that. I think this deserves a separate issue though, would you mind creating a new one for this?

I think the original bug reported here (which was about broken substring and case-sensitiveness) was solved.

It seems the new issue is basically caused by an incomplete implementation of the ocs sharing api in ocis/reva. It always returns an empty exact list and stuffs all the results into the top level users list (https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/handlers/apps/sharing/sharees/sharees.go#L91)

@SwikritiT
Copy link
Contributor

@SwikritiT thanks for looking into that. I think this deserves a separate issue though, would you mind creating a new one for this?

I think the original bug reported here (which was about broken substring and case-sensitiveness) was solved.

It seems the new issue is basically caused by an incomplete implementation of the ocs sharing api in ocis/reva. It always returns an empty exact list and stuffs all the results into the top level users list (https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/handlers/apps/sharing/sharees/sharees.go#L91)

Okay I'll create a separate issue and close this one

@SwikritiT
Copy link
Contributor

@rhafer new issue created here #4265

@SwikritiT
Copy link
Contributor

SwikritiT commented Jul 25, 2022

This PR got merged #4229 fixing this issue and needs a follow-up for the expected to fail.

#### [Searching sharee with displayname](https://github.com/owncloud/ocis/issues/547)
- [apiSharees/sharees.feature:350](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L350)
- [apiSharees/sharees.feature:351](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L351)
- [apiSharees/sharees.feature:370](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L370)
- [apiSharees/sharees.feature:371](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L371)
- [apiSharees/sharees.feature:390](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L390)
- [apiSharees/sharees.feature:391](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L391)
- [apiSharees/sharees.feature:410](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L410)
- [apiSharees/sharees.feature:411](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L411)
- [apiSharees/sharees.feature:430](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L430)
- [apiSharees/sharees.feature:431](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharees/sharees.feature#L431)

TODO QA-team

Closing this issue. I'll make a PR changing the issue link in expected to fail file

dragonchaser pushed a commit that referenced this issue Jul 27, 2022
This includes a fix for case-insensitive substring filtering

Partial-Fix: #547
dragonchaser pushed a commit that referenced this issue Jul 27, 2022
This introduces new settings for the users and groups services.
"group_substring_filter_type" for the group services and
"user_substring_filter_type" for the users service. They allow to set
the type of LDAP filter that is used for substring user searches.
Possible values are: "initial", "final" and "any" to do either prefix,
suffix or full substring searches.

Fixes #547
dragonchaser pushed a commit that referenced this issue Jul 27, 2022
This adds the "search_min_length" setting to the frontend service which
allows to set the search_min_length capabilty which is e.g. used by
web.

Partial: #547
@micbar micbar mentioned this issue Aug 11, 2022
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants