resolve user and groups in nested groups first before filtering the results #14464

blizzz · 2019-03-01T23:39:38Z

Continuation of #8227

perhaps unit tests need adjustments
add integration tests (needs LDAP: add nested groups and work around memberof-bug in used docker image docker-ci#137)

As described by @Cybso in #8227 (comment), when users were not part directly of a whitelisted group, but of an excluded subgroup, they would wrongly not appear as member of the top group. While the excluded groups should not be used and shown in Nextcloud, their users still qualify to be associated with the top group. Users are only subject to user filter and user base restrictions.

We have had three places covering the nesting logic aka recursive lookup. I continued @Cybso approach to cover all cases, and consolidating it into one. Plus, integration tests make sure that all desired users are being found :)

blizzz · 2019-03-05T09:33:36Z

At least some tests fail due to a regression which should be fixed with Fix missing $config dependency in RetryJob #14529. Will rebase afterwards.

MorrisJobke · 2019-03-05T10:02:10Z

At least some tests fail due to a regression which should be fixed with #14529. Will rebase afterwards.

Merged.

Currently groupsMatchFilter is called before nested groups are resolved. This basicly breaks this feature since it is not possible to inherit membership in a group from another group. Minimal example: Group filter: (&(objectClass=group),(cn=nextcloud)) Nested groups: enabled cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local objectClass: group cn=IT,ou=groups,dn=company,dn=local objectClass: group memberOf: cn=nextcloud,ou=Nextcloud,ou=groups,dn=company,dn=local cn=John Doe,ou=users,dn=company,dn=local objectClass: person memberOf: cn=IT,ou=groups,dn=company,dn=local Since 'cn=IT,ou=groups,dn=company,dn=local' doesn't match the group filter, John wouldn't be a member of group 'nextcloud'. This patch fixes this by filtering the groups after all nested groups have been collected. If nested groups is disabled the result will be the same as without this patch. Signed-off-by: Roland Tapken <[email protected]>

The previous patch fixed the problem only for one level of indirection because groupsMatchFilter() had been applied on each recursive call (and thus there would be no second level if the first level fails the check). This new implementation replaces the recursive call with a stack that iterates all nested groups before filtering with groupsMatchFilter(). Signed-off-by: Roland Tapken <[email protected]>

Nested groups are now cached in a CappedMemoryCache object to reduce queries to the LDAP backend. Signed-off-by: Roland Tapken <[email protected]>

…anymore. Signed-off-by: Roland Tapken <[email protected]>

and also consolidate logic in one method Signed-off-by: Arthur Schiwon <[email protected]>

…more Signed-off-by: Arthur Schiwon <[email protected]>

Signed-off-by: Arthur Schiwon <[email protected]>

faily-bot · 2019-03-06T00:02:10Z

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 16802: failure

TESTS=acceptance, TESTS-ACCEPTANCE=app-files

tests/acceptance/features/app-files.feature:79

Show full log

  Scenario: show recent files for a second time                               # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files.feature:79
    Given I am logged in                                                      # LoginPageContext::iAmLoggedIn()
    And I open the "Recent" section                                           # AppNavigationContext::iOpenTheSection()
    And I see that the current section is "Recent"                            # AppNavigationContext::iSeeThatTheCurrentSectionIs()
    And I open the "All files" section                                        # AppNavigationContext::iOpenTheSection()
    And I see that the current section is "All files"                         # AppNavigationContext::iSeeThatTheCurrentSectionIs()
    And I create a new folder named "Folder just created"                     # FileListContext::iCreateANewFolderNamed()
    When I open the "Recent" section                                          # AppNavigationContext::iOpenTheSection()
    Then I see that the current section is "Recent"                           # AppNavigationContext::iSeeThatTheCurrentSectionIs()
    Then I see that the file list contains a file named "Folder just created" # FileListContext::iSeeThatTheFileListContainsAFileNamed()
      Row for file Folder just created in file list could not be found after 100 seconds (NoSuchElementException)

TESTS=acceptance, TESTS-ACCEPTANCE=app-files-sharing

tests/acceptance/features/app-files-sharing.feature:23
tests/acceptance/features/app-files-sharing.feature:86

Show full log

  Scenario: share a file with another user who already has a file with that name # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files-sharing.feature:23
    Given I act as John                                                          # ActorContext::iActAs()
    And I am logged in as the admin                                              # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I act as Jane                                                            # ActorContext::iActAs()
    And I am logged in                                                           # LoginPageContext::iAmLoggedIn()
    And I act as John                                                            # ActorContext::iActAs()
    When I share "welcome.txt" with "user0"                                      # FilesAppSharingContext::iShareWith()
      Share action for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
    And I see that the file is shared with "user0"                               # FilesAppSharingContext::iSeeThatTheFileIsSharedWith()
    And I act as Jane                                                            # ActorContext::iActAs()
    And I open the Files app                                                     # FilesAppContext::iOpenTheFilesApp()
    Then I see that the file list contains a file named "welcome (2).txt"        # FileListContext::iSeeThatTheFileListContainsAFileNamed()
    And I open the details view for "welcome (2).txt"                            # FileListContext::iOpenTheDetailsViewFor()
    And I see that the details view is open                                      # FilesAppContext::iSeeThatTheDetailsViewIsOpen()
    And I open the "Sharing" tab in the details view                             # FilesAppContext::iOpenTheTabInTheDetailsView()
    And I see that the "Sharing" tab in the details view is eventually loaded    # FilesAppContext::iSeeThatTheTabInTheDetailsViewIsEventuallyLoaded()
    And I see that the file is shared with me by "admin"                         # FilesAppSharingContext::iSeeThatTheFileIsSharedWithMeBy()

  Scenario: owner sees reshares with other users                              # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files-sharing.feature:86
    Given I act as John                                                       # ActorContext::iActAs()
    And I am logged in as the admin                                           # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I act as Jane                                                         # ActorContext::iActAs()
    And I am logged in                                                        # LoginPageContext::iAmLoggedIn()
      User name field in Login page could not be found after 100 seconds (NoSuchElementException)
    And I act as John                                                         # ActorContext::iActAs()
    And I rename "welcome.txt" to "farewell.txt"                              # FileListContext::iRenameTo()
    And I see that the file list contains a file named "farewell.txt"         # FileListContext::iSeeThatTheFileListContainsAFileNamed()
    And I share "farewell.txt" with "user0"                                   # FilesAppSharingContext::iShareWith()
    And I see that the file is shared with "user0"                            # FilesAppSharingContext::iSeeThatTheFileIsSharedWith()
    And I act as Jane                                                         # ActorContext::iActAs()
    And I open the Files app                                                  # FilesAppContext::iOpenTheFilesApp()
    And I share "farewell.txt" with "user1"                                   # FilesAppSharingContext::iShareWith()
    And I see that the file is shared with "user1"                            # FilesAppSharingContext::iSeeThatTheFileIsSharedWith()
    When I act as John                                                        # ActorContext::iActAs()
    And I open the Files app                                                  # FilesAppContext::iOpenTheFilesApp()
    And I open the details view for "farewell.txt"                            # FileListContext::iOpenTheDetailsViewFor()
    And I see that the details view is open                                   # FilesAppContext::iSeeThatTheDetailsViewIsOpen()
    And I open the "Sharing" tab in the details view                          # FilesAppContext::iOpenTheTabInTheDetailsView()
    And I see that the "Sharing" tab in the details view is eventually loaded # FilesAppContext::iSeeThatTheTabInTheDetailsViewIsEventuallyLoaded()
    Then I see that the file is shared with "user0"                           # FilesAppSharingContext::iSeeThatTheFileIsSharedWith()
    And I see that the file is shared with "user1"                            # FilesAppSharingContext::iSeeThatTheFileIsSharedWith()
sh: 1: kill: No such process

TESTS=acceptance, TESTS-ACCEPTANCE=app-files-sharing-link

tests/acceptance/features/app-files-sharing-link.feature:26

Show full log

  Scenario: show download again in a public shared link             # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files-sharing-link.feature:26
    Given I act as John                                             # ActorContext::iActAs()
    And I am logged in                                              # LoginPageContext::iAmLoggedIn()
    And I share the link for "welcome.txt"                          # FilesAppSharingContext::iShareTheLinkFor()
      Share action for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
    And I set the download of the shared link as hidden             # FilesAppSharingContext::iSetTheDownloadOfTheSharedLinkAsHidden()
    And I set the download of the shared link as shown              # FilesAppSharingContext::iSetTheDownloadOfTheSharedLinkAsShown()
    And I write down the shared link                                # FilesAppSharingContext::iWriteDownTheSharedLink()
    When I act as Jane                                              # ActorContext::iActAs()
    And I visit the shared link I wrote down                        # PublicShareContext::iVisitTheSharedLinkIWroteDown()
    And I see that the current page is the shared link I wrote down # PublicShareContext::iSeeThatTheCurrentPageIsTheSharedLinkIWroteDown()
    Then I see that the download button is shown                    # PublicShareContext::iSeeThatTheDownloadButtonIsShown()
    And I open the Share menu                                       # PublicShareContext::iOpenTheShareMenu()
    And I see that the Share menu is shown                          # PublicShareContext::iSeeThatTheShareMenuIsShown()

blizzz · 2019-03-06T09:10:06Z

failing tests are unrelated

blizzz · 2019-03-06T11:33:20Z

/backport to stable15

blizzz · 2019-03-06T11:33:25Z

/backport to stable14

ChristophWurst

🐘, but 👍 because there are tests 😉

rullzer

Makes sense. And tests so lets 🎢

backportbot-nextcloud · 2019-03-07T21:32:48Z

The backport to stable15 failed. Please do this backport manually.

backportbot-nextcloud · 2019-03-07T21:33:27Z

The backport to stable14 failed. Please do this backport manually.

blizzz added bug 2. developing Work in progress feature: ldap labels Mar 1, 2019

blizzz added this to the Nextcloud 16 milestone Mar 1, 2019

blizzz mentioned this pull request Mar 1, 2019

user_ldap: Filter groups after nested groups #8227

Closed

MorrisJobke mentioned this pull request Mar 4, 2019

16.0.0 Alpha 1 #14501

Merged

45 tasks

blizzz force-pushed the fix/noid/ldap-nested-group-filter branch from 1e00d56 to 09357b8 Compare March 5, 2019 00:14

blizzz added 3. to review Waiting for reviews backport-request and removed 2. developing Work in progress labels Mar 5, 2019

Roland Tapken and others added 5 commits March 5, 2019 11:07

Reduce queries to LDAP by caching nested groups

e7c506c

Nested groups are now cached in a CappedMemoryCache object to reduce queries to the LDAP backend. Signed-off-by: Roland Tapken <[email protected]>

Fixed unit test: groupsMatchFilter will not be called multiple times …

459b8a4

…anymore. Signed-off-by: Roland Tapken <[email protected]>

fix nested group retrieval also for 2 other cases

5dd2207

and also consolidate logic in one method Signed-off-by: Arthur Schiwon <[email protected]>

blizzz force-pushed the fix/noid/ldap-nested-group-filter branch 3 times, most recently from 14315cc to ca963fa Compare March 5, 2019 15:53

faily-bot-test bot deleted a comment from faily-bot bot Mar 5, 2019

with LDAP server set offline, config cannot be controlled via ocs any…

dfc7007

…more Signed-off-by: Arthur Schiwon <[email protected]>

blizzz force-pushed the fix/noid/ldap-nested-group-filter branch from ca963fa to dfc7007 Compare March 5, 2019 22:14

nextcloud deleted a comment from faily-bot bot Mar 5, 2019

add missing config bits to integration tests

987db8e

Signed-off-by: Arthur Schiwon <[email protected]>

nextcloud deleted a comment from faily-bot bot Mar 5, 2019

blizzz requested review from MorrisJobke and rullzer March 5, 2019 23:28

blizzz requested review from juliusknorr, Cybso and kesselb March 5, 2019 23:28

remove unused use statement

e36cede

Signed-off-by: Arthur Schiwon <[email protected]>

nextcloud deleted a comment from faily-bot bot Mar 6, 2019

MorrisJobke mentioned this pull request Mar 6, 2019

16.0.0 Beta 1 #14573

Merged

9 tasks

ChristophWurst approved these changes Mar 7, 2019

View reviewed changes

rullzer approved these changes Mar 7, 2019

View reviewed changes

ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 7, 2019

blizzz merged commit 3af7f2c into master Mar 7, 2019

blizzz deleted the fix/noid/ldap-nested-group-filter branch March 7, 2019 21:31

This was referenced Mar 7, 2019

[stable15] resolve user and groups in nested groups first before filtering the results #14591

Merged

[stable14] resolve user and groups in nested groups first before filtering the results #14592

Merged

MorrisJobke removed the backport-request label Mar 8, 2019

kesselb mentioned this pull request Apr 11, 2019

Sabre\DAV\Exception\ServiceUnavailable: TypeError: Argument 1 passed to OCA\User_LDAP\Group_LDAP::walkNestedGroups() must be of the type string, null given #15050

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve user and groups in nested groups first before filtering the results #14464

resolve user and groups in nested groups first before filtering the results #14464

blizzz commented Mar 1, 2019 •

edited

Loading

blizzz commented Mar 5, 2019 •

edited

Loading

MorrisJobke commented Mar 5, 2019

faily-bot bot commented Mar 6, 2019

blizzz commented Mar 6, 2019

blizzz commented Mar 6, 2019

blizzz commented Mar 6, 2019

ChristophWurst left a comment

rullzer left a comment

backportbot-nextcloud bot commented Mar 7, 2019

backportbot-nextcloud bot commented Mar 7, 2019

resolve user and groups in nested groups first before filtering the results #14464

resolve user and groups in nested groups first before filtering the results #14464

Conversation

blizzz commented Mar 1, 2019 • edited Loading

blizzz commented Mar 5, 2019 • edited Loading

MorrisJobke commented Mar 5, 2019

faily-bot bot commented Mar 6, 2019

Status of 16802: failure

TESTS=acceptance, TESTS-ACCEPTANCE=app-files

TESTS=acceptance, TESTS-ACCEPTANCE=app-files-sharing

TESTS=acceptance, TESTS-ACCEPTANCE=app-files-sharing-link

blizzz commented Mar 6, 2019

blizzz commented Mar 6, 2019

blizzz commented Mar 6, 2019

ChristophWurst left a comment

Choose a reason for hiding this comment

rullzer left a comment

Choose a reason for hiding this comment

backportbot-nextcloud bot commented Mar 7, 2019

backportbot-nextcloud bot commented Mar 7, 2019

blizzz commented Mar 1, 2019 •

edited

Loading

blizzz commented Mar 5, 2019 •

edited

Loading