Occ command to update group mapping information #14

Merged
merged 5 commits into from Oct 11, 2016

Projects

None yet

6 participants

@jvillafanez
Contributor

Original PR: owncloud/core#26029

Related issue: owncloud/enterprise#1464

We need to agree on backporting this PR (issue comes from version 8.2)

@jvillafanez jvillafanez Occ command to update group mapping information
47929f8
@jvillafanez jvillafanez added this to the 9.2 milestone Oct 4, 2016
@CLAassistant
CLAassistant commented Oct 4, 2016 edited

CLA assistant check
All committers have signed the CLA.

@mention-bot

@jvillafanez, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blizzz, @owncloud-bot and @nickvergessen to be potential reviewers.

@jvillafanez jvillafanez referenced this pull request in owncloud/core Oct 4, 2016
Closed

Occ command to update group mapping information #26029

4 of 9 tasks complete
lib/Command/UpdateGroup.php
+ protected function configure() {
+ $this
+ ->setName('ldap:update-group')
+ ->setDescription('update the specified group information')
@PVince81
PVince81 Oct 4, 2016 Contributor

does it mean we are reloading the LDAP groups ? syncing the specified LDAP groups to OC ?

if yes, maybe update the description to say so.

@jvillafanez
jvillafanez Oct 5, 2016 Contributor

It updates the group membership information stored in the DB. Does "update the specified group membership information stored locally" sound better?

@PVince81
PVince81 Oct 5, 2016 Contributor

sounds good

lib/Command/UpdateGroup.php
+ $availableConfigs = $helper->getServerConfigurationPrefixes();
+
+ // show configuration information, useful to debug
+ $output->writeln('group membership attribute is critical for this command to work properly, please verify', OutputInterface::VERBOSITY_VERBOSE);
@PVince81
PVince81 Oct 4, 2016 Contributor

is this an error message ?

it looks like it asks the user to verify something but doesn't ask whether the user wants to continue but continues directly ? should we add a prompt "continue yes/no" with an override --yes option ?

@jvillafanez
jvillafanez Oct 5, 2016 Contributor

I didn't want to keep bothering the user over and over when the attribute is properly set. However, adding the --yes option seems a better choice.

@PVince81
PVince81 Oct 5, 2016 Contributor

this, assuming that we cannot detect whether the property is properly set.

@PVince81
PVince81 Oct 5, 2016 Contributor

now thinking of it I think it's ok to keep the warning but not ask for confirmation.

If an admin really goes as far as using this command, it means they already have setup their groups in LDAP properly so there is no need to bother that much.

+
+ $groupProxy = new Group_Proxy($availableConfigs, $this->ldap);
+
+ foreach ($groupIDs as $groupID) {
@PVince81
PVince81 Oct 4, 2016 edited Contributor

would be good to have some unit tests where possible, as I see the logic here a bit complex

lib/Command/UpdateGroup.php
+ foreach ($groupIDs as $groupID) {
+ $output->writeln("checking group \"$groupID\"...", OutputInterface::VERBOSITY_VERBOSE);
+ if (!$groupProxy->groupExists($groupID)) {
+ $output->writeln("\"$groupID\" is missing, unmapping it", OutputInterface::VERBOSITY_VERBOSE);
@PVince81
PVince81 Oct 4, 2016 Contributor

"$groupID doesn't exist in LDAP any more, removing local mapping" ?

lib/Command/UpdateGroup.php
+ $output->writeln("\"$groupID\" is missing, unmapping it", OutputInterface::VERBOSITY_VERBOSE);
+ $this->removeGroupMapping($groupID);
+ } else {
+ $output->writeln("updating \"$groupID\" group DB information", OutputInterface::VERBOSITY_VERBOSE);
@PVince81
PVince81 Oct 4, 2016 Contributor

"group DB" or "group mapping" ? you seem to be mixing terms, better stay consistent with the wording to avoid confusion. Or are they different things and I'm already confused πŸ˜•

lib/Command/UpdateGroup.php
+ $userList = $groupProxy->usersInGroup($groupID);
+ $userChanges = $this->updateGroupMapping($groupID, $userList);
+
+ $output->writeln("sending hooks", OutputInterface::VERBOSITY_VERBOSE);
@PVince81
PVince81 Oct 4, 2016 Contributor

"triggering hooks" might sound better

+ /**
+ * Return and array with 2 lists: one for the users added and another for the users removed from
+ * the group:
+ * ['added' => ['user1', 'user20'], 'removed' => ['user22']]
@PVince81
PVince81 Oct 4, 2016 Contributor

Great to have a comment, even better to use PHPDoc style as well:

@param array $groupName
...

πŸ˜„

lib/Command/UpdateGroup.php
+ /**
+ * Make sure $groupNames doesn't contain duplicated values. This function could behave
+ * unexpectedly otherwise.
+
@PVince81
PVince81 Oct 4, 2016 Contributor

the invasion of the red blood spaces has begun!

@PVince81
Contributor
PVince81 commented Oct 4, 2016 edited
  • BUG: Display a message "no changes" when no changes are found for any of the selected groups ? (I ran it and was confused to see no output at all)
@PVince81
Contributor
PVince81 commented Oct 4, 2016

Hmmm, no sure what I'm doing wrong: whatever the situation there is never any output.

I have the following config:

Then I run this:

occ ldap:update-group Box10 -vvvvvvv

No output.
Then remove or add some users in the group Box10 in LDAP directly.
Then run the command again: no output either.

Only if I pass a wrong group id I'll get an error, so output does work.

I'm unable to paste my LDAP config as show-config is broken and I don't know where to find the config-id in the DB: #16

@PVince81
Contributor
PVince81 commented Oct 4, 2016 edited

Hmm okay, seems I passed to many v to the verbosity. Would be good if it could still work as other commands like occ files:scan accept any number of "v".

Here is the output after adding and deleting a user from "Box10":

group membership attribute is critical for this command to work properly, please verify
* localhost:389 -> memberUid
checking group "Box10"...
updating "Box10" group DB information
sending hooks
new users:
8c5e411a-1eba-1036-991b-57e71cf45853
removed users:
8cc327d8-1eba-1036-99a5-57e71cf45853

Hmm, ok so it seems to work.

  • BUG: display at least a summary output when no verbosity given, else one believes that it didn't work
@jvillafanez jvillafanez Show output with the default verbosity level
dfb8466
@jvillafanez
Contributor

Adjusted messages, plus the verbosity of some of them in order to show output when no verbosity is given.

There is one thing that might not be a good idea: the confirmation dialog is also hidden if the command runs with the "-q" option. The command hangs until the user confirms the hidden dialog, which is a very bad UX from my point of view.
I don't think there is a good solution for this other than blame the user's stupidity or just get rid of the confirmation dialog and just run the command.

If we decide to get rid of the dialog, we might need to show the warning message always (previous verbosity to make it appear was "-v") and add another option to hide the warning.

@PVince81
Contributor
PVince81 commented Oct 5, 2016

Yeah, please just get rid of the confirmation. I think it's not needed as per my comment here #14 (comment)

If an admin made it that far, then they already have proper group mappings.

@jvillafanez jvillafanez Remove dialog and add option to hide the warning message
84273c7
@PVince81
Contributor
PVince81 commented Oct 5, 2016

@jvillafanez let me know when this is ready for review again and set the label to "3 - To Review". Thanks

@PVince81
Contributor
PVince81 commented Oct 6, 2016

Verbosity is better now.
But when there is no change it looks like this:

checking group "Box10"...
new users:
removed users:
checking group "Box100"...
new users:
removed users:

Also I hope no one will get the idea trying to parse this output.
I'm fine keeping it this way for now.

@PVince81 PVince81 fix typo
0a70db2
@PVince81
Contributor
PVince81 commented Oct 6, 2016
  • BUG: group deletion πŸ’£

I deleted "Box100" which existed in openLDAP, then ran the command:

"Box100" doesn't exist in LDAP any more, removing local mapping


  [Doctrine\DBAL\Exception\SyntaxErrorException]                                                                                                 
  An exception occurred while executing 'DELETE FROM `oc_ldap_group_mapping` lgm WHERE lgm.`owncloud_name` = ?' with params ["Box100"]:          
  SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your Maria  
  DB server version for the right syntax to use near 'lgm WHERE lgm.`owncloud_name` = 'Box100'' at line 1                                        



  [Doctrine\DBAL\Driver\PDOException]                                                                                                            
  SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your Maria  
  DB server version for the right syntax to use near 'lgm WHERE lgm.`owncloud_name` = 'Box100'' at line 1                                        



  [PDOException]                                                                                                                                 
  SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your Maria  
  DB server version for the right syntax to use near 'lgm WHERE lgm.`owncloud_name` = 'Box100'' at line 1                                        
@jvillafanez jvillafanez Remove non-working table alias
eebe01a
@jvillafanez
Contributor

Also I hope no one will get the idea trying to parse this output.

I tried to keep the changes as minimum as possible, so showing a trace instead of a parseable output was easier. If it's needed then we should fix it in another PR.

BUG: group deletion

Solved now.

@PVince81
Contributor

Tested, works πŸ‘

Question: should this command be extended to also allow adding/syncing newly created groups that weren't known in OC before ? (or ones that were deleted and re-added)

@jvillafanez
Contributor

Question: should this command be extended to also allow adding/syncing newly created groups that weren't known in OC before ? (or ones that were deleted and re-added)

Probably, but taking into account we'll have to perform a search which might bring more groups (searching for Box1 will bring also Box11, Box12, Box100, etc), and I'm not sure about possible side effects specially with caching, I don't know if it's a good idea.

@DeepDiver1975
Member

@jvillafanez please press the button for the CLA - CLA assistant check

@PVince81
Contributor

looks like he did now, let's merge

@PVince81 PVince81 merged commit 1a8ed85 into master Oct 11, 2016

3 checks passed

Scrutinizer 5 new issues, 7 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the ldap_command_updategroup branch Oct 11, 2016
@PVince81
Contributor

As this is a new feature, I'm not sure about backporting... is this critical enough ? @felixboehm

@felixboehm

Yes definitiv for 9.0

@DeepDiver1975
Member

Yes definitiv for 9.0

@jvillafanez can i ask you to take care? thx

@jvillafanez
Contributor

Sure, taking into account the set of changes, I'll just copy & paste the file between the repos (with the minor change in the register_command.php file)

@jvillafanez
Contributor
@DeepDiver1975
Member

Stable9.1: owncloud/core#26341
Stable9: owncloud/core#26343

@felixboehm

@felixboehm

Great work, thanks!

@PVince81
Contributor

Raised #20 because I think that we shouldn't fail in case one group is not found, due to potential admin + cron use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment