Skip to content

ignore non existing users when retrieving details of group members #13644

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.

Sign up for GitHub

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

Jump to bottom
Merged
MorrisJobke merged 1 commit into from
Jan 24, 2019
Merged

ignore non existing users when retrieving details of group members #13644

MorrisJobke merged 1 commit into from
Jan 24, 2019

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Jan 17, 2019

Fixes an exception leading to a 404 return of the user details request on the members of a specified group. Please read #12991 (comment) for detailed reasonings.

To reproduce

  1. Have LDAP configured
  2. Have a local group "group1"
  3. Add an LDAP user to "group1", and another user (local or ldap does not matter, for comparison "stillExistingUser")
  4. Delete the user from LDAP (or change the filter to exclude the user)
  5. Use "occ ldap:check-user" to ensure this user is detected as deleted on LDAP (beware caching)
  6. Use the curl call against the provisioning API to get the user details (or go to Users page and click on the group)
  7. curl -H "OCS-APIRequest: true" "http://admin:[email protected]/ocs/v2.php/cloud/groups/group1/users/details"
    Before:
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>404</statuscode>
  <message>Invalid query, please check the syntax. API specifications are here: http://www.freedesktop.org/wiki/Specifications/open-collaboration-services.
</message>
 </meta>
 <data/>
</ocs>

After:

<?xml version="1.0"?>
<ocs>
 <meta>
  <status>ok</status>
  <statuscode>200</statuscode>
  <message>OK</message>
 </meta>
 <data>
  <users>
   <stillExistingUser>
    <enabled>1</enabled>
    <storageLocation>/path/to/data/stillExistingUser</storageLocation>
    <id>stillExistingUser</id>
    <lastLogin>1546614345000</lastLogin>
    <backend>Database</backend>
    <!-- ... -->
   </stillExistingUser>
  </users>
 </data>
</ocs>

@blizzz blizzz added this to the Nextcloud 16 milestone Jan 17, 2019
skjnldsv
skjnldsv reviewed Jan 17, 2019
View reviewed changes
apps/provisioning_api/lib/Controller/GroupsController.php
$usersDetails[$userId] = ['id' => $userId];
}
} catch(OCSNotFoundException $e) {
// continue if a users ceased to exist.
Copy link
Member

@skjnldsv skjnldsv Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe at least log something?
What's the use case of having a list of users where the user does not exists here?

Copy link
Member Author

@blizzz blizzz Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user is not added to the list when this Exception pops up. Or do you mean something else?

Copy link
Member

@skjnldsv skjnldsv Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I don't really like having blank catches :p

Copy link
Member Author

@blizzz blizzz Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not an error condition, but a valid scenario.

rullzer
rullzer reviewed Jan 18, 2019
View reviewed changes
apps/provisioning_api/lib/Controller/AUserData.php
# cf. https://github.com/nextcloud/server/issues/12991
$data['storageLocation'] = $targetUserObject->getHome();
} catch (NoUserException $e) {
throw new OCSNotFoundException($e->getMessage(), $e);
Copy link
Member

@rullzer rullzer Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why throw the OCS not found exception? This is mainly osmething to throw when the middleware will handle it. The NoUserException seems perfectly valid here.

Copy link
Member Author

@blizzz blizzz Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it is also done in line 95 (https://github.com/nextcloud/server/pull/13644/files/4915d64de8d0ee862f0fdc92fe9bf856f3e8cbe1#diff-4d20146890eb1004590c5327656f93bdR95), so it is consistent and catches both cases.

@blizzz blizzz requested a review from MorrisJobke January 24, 2019 09:42
@blizzz
Copy link
Member Author

blizzz commented Jan 24, 2019 •
edited
Loading

so, dear reviewers, is it OK to go in?

skjnldsv
skjnldsv approved these changes Jan 24, 2019
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then good for me :)

MorrisJobke
MorrisJobke approved these changes Jan 24, 2019
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code makes sense 👍

@MorrisJobke MorrisJobke merged commit 6cdb392 into master Jan 24, 2019
@MorrisJobke MorrisJobke deleted the fix/12991/catch-nouserexception-groupusersdetails branch January 24, 2019 13:23
@MorrisJobke
Copy link
Member

MorrisJobke commented Jan 24, 2019

@blizzz Backport to which branches?

@MorrisJobke
Copy link
Member

MorrisJobke commented Jan 24, 2019

/backport to stable15

@MorrisJobke
Copy link
Member

MorrisJobke commented Jan 24, 2019

/backport to stable14

@MorrisJobke
Copy link
Member

MorrisJobke commented Jan 24, 2019

Background: the report was against 14.0.4

@MorrisJobke MorrisJobke mentioned this pull request Jan 24, 2019
Closed
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 24, 2019
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Jan 24, 2019

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

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Jan 24, 2019

backport to stable15 in #13792

@rullzer
Copy link
Member

rullzer commented Jan 29, 2019

@blizzz backport to 14 failed. Could you do that manually?

@blizzz
Copy link
Member Author

blizzz commented Jan 30, 2019

yes, this was expected. doing it now.

@blizzz blizzz mentioned this pull request Jan 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzer rullzer rullzer left review comments

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug feature: ldap feature: users and groups

Projects

None yet

Milestone

Nextcloud 16

Development

Successfully merging this pull request may close these issues.

5 participants

@blizzz @MorrisJobke @rullzer @skjnldsv