don't store empty groupids #27295

Merged
merged 2 commits into from Mar 3, 2017

Conversation

Projects
None yet
2 participants
@butonic
Member

butonic commented Mar 2, 2017

@PVince81 an empty string as groupid shouldnt make it to the DB, right? oracle doesnt like empty string on non nullable columns (which the systemtag_group.gid is)

This PR hardens the code should someone try to add an empty group id. Likely there is an empty group in

if (isset($data['groups'])) {
$groups = $data['groups'];
if (is_string($groups)) {
$groups = explode('|', $groups);
}
}
but that is a different issue.

@butonic butonic requested a review from PVince81 Mar 2, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 2, 2017

Member

Ew... how is that even possible...

Unit test ?

Member

PVince81 commented Mar 2, 2017

Ew... how is that even possible...

Unit test ?

@PVince81 PVince81 added this to the 10.0 milestone Mar 2, 2017

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 2, 2017

Member
{"reqId":"WLfJPKwV0uMAAFnMeU4AAAAC","remoteAddr":"10.181.130.238","app":"admin_audit","message":"user xxx created system tag \"Gedoens (invisible)\" [CLIENT_IP: 10
.181.130.238] [USER_AGENT: Mozilla\/5.0 (Windows NT 6.1; WOW64; Trident\/7.0; rv:11.0) like Gecko]","level":1,"time":"2017-03-02T08:26:52+01:00","method":"POST","url":"\/remote.php
\/dav\/systemtags\/","user":"xxx"}
{"reqId":"WLfJPKwV0uMAAFnMeU4AAAAC","remoteAddr":"10.181.130.238","app":"webdav","message":"Exception: {\"Message\":\"An exception occurred while executing 'INSERT INTO \\\"oc_systemtag_group\\\" (\\\"systemtagid\\\", \\\"gid\\\") VALUES(?, ?)' with params [24, \\\"\\\"]:

ORA-01400: cannot insert NULL into (\\\"OWNCLOUD\\\".\\\"oc_systemtag_group\\\".\\\"gid\\\")\",\"Exception\":\"Doctrine\\\\DBAL\\\\Exception\\
otNullConstraintViolationException\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/DBALException.php(116): Doctrine\\\\DBAL\\\\Driver\\\\AbstractOracleDriver->convertException('An exception oc...', Object(Doctrine\\\\DBAL\\\\Driver\\\\OCI8\\\\OCI8Exception))
#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Connection.php(996): Doctrine\\\\DBAL\\\\DBALException::driverExceptionDuringQuery(Object(Doctrine\\\\DBAL\\\\Driver\\\\OCI8\\\\Driver), Object(Doctrine\\\\DBAL\\\\Driver\\\\OCI8\\\\OCI8Exception), 'INSERT INTO \\\"oc...', Array)
#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/DB\\\/Connection.php(209): Doctrine\\\\DBAL\\\\Connection->executeUpdate('INSERT INTO \\\"oc...', Array, Array)
#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Query\\\/QueryBuilder.php(208): OC\\\\DB\\\\Connection->executeUpdate('INSERT INTO `*P...', Array, Array)
#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/DB\\\/QueryBuilder\\\/QueryBuilder.php(141): Doctrine\\\\DBAL\\\\Query\\\\QueryBuilder->execute()
#5 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/SystemTag\\\/SystemTagManager.php(403): OC\\\\DB\\\\QueryBuilder\\\\QueryBuilder->execute()
#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/SystemTag\\\/SystemTagPlugin.php(205): OC\\\\SystemTag\\\\SystemTagManager->setTagGroups(Object(OC\\\\SystemTag\\\\SystemTag), Array)
#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/SystemTag\\\/SystemTagPlugin.php(132): OCA\\\\DAV\\\\SystemTag\\\\SystemTagPlugin->createTag('{\\\"name\\\":\\\"Gedoen...', 'application\\\/jso...')
#8 [internal function]: OCA\\\\DAV\\\\SystemTag\\\\SystemTagPlugin->httpPost(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))
#9 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/event\\\/lib\\\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)
#10 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(459): Sabre\\\\Event\\\\EventEmitter->emit('method:POST', Array)
#11 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(248): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))
#12 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Server.php(178): Sabre\\\\DAV\\\\Server->exec()
#13 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v2\\\/remote.php(30): OCA\\\\DAV\\\\Server->exec()
#14 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/remote.php(164): require_once('\\\/srv\\\/www\\\/htdocs...')
#15 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Driver\\\/AbstractOracleDriver.php\",\"Line\":68,\"User\":\"xxx\"}","level":4,"time":"2017-03-02T08:26:52+01:00","method":"POST","url":"\/remote.php\/dav\/systemtags\/","user":"xxx"}
{"reqId":"WLfJPKwV0uMAAFnMeU4AAAAC","remoteAddr":"10.181.130.238","app":"stats","message":"request took 0.35226702690125","level":0,"time":"2017-03-02T08:26:52+01:00","method":"POST","url":"\/remote.php\/dav\/systemtags\/","user":"xxx"}

I assume in mysql we have empty strings in that table ...

Member

butonic commented Mar 2, 2017

{"reqId":"WLfJPKwV0uMAAFnMeU4AAAAC","remoteAddr":"10.181.130.238","app":"admin_audit","message":"user xxx created system tag \"Gedoens (invisible)\" [CLIENT_IP: 10
.181.130.238] [USER_AGENT: Mozilla\/5.0 (Windows NT 6.1; WOW64; Trident\/7.0; rv:11.0) like Gecko]","level":1,"time":"2017-03-02T08:26:52+01:00","method":"POST","url":"\/remote.php
\/dav\/systemtags\/","user":"xxx"}
{"reqId":"WLfJPKwV0uMAAFnMeU4AAAAC","remoteAddr":"10.181.130.238","app":"webdav","message":"Exception: {\"Message\":\"An exception occurred while executing 'INSERT INTO \\\"oc_systemtag_group\\\" (\\\"systemtagid\\\", \\\"gid\\\") VALUES(?, ?)' with params [24, \\\"\\\"]:

ORA-01400: cannot insert NULL into (\\\"OWNCLOUD\\\".\\\"oc_systemtag_group\\\".\\\"gid\\\")\",\"Exception\":\"Doctrine\\\\DBAL\\\\Exception\\
otNullConstraintViolationException\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/DBALException.php(116): Doctrine\\\\DBAL\\\\Driver\\\\AbstractOracleDriver->convertException('An exception oc...', Object(Doctrine\\\\DBAL\\\\Driver\\\\OCI8\\\\OCI8Exception))
#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Connection.php(996): Doctrine\\\\DBAL\\\\DBALException::driverExceptionDuringQuery(Object(Doctrine\\\\DBAL\\\\Driver\\\\OCI8\\\\Driver), Object(Doctrine\\\\DBAL\\\\Driver\\\\OCI8\\\\OCI8Exception), 'INSERT INTO \\\"oc...', Array)
#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/DB\\\/Connection.php(209): Doctrine\\\\DBAL\\\\Connection->executeUpdate('INSERT INTO \\\"oc...', Array, Array)
#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Query\\\/QueryBuilder.php(208): OC\\\\DB\\\\Connection->executeUpdate('INSERT INTO `*P...', Array, Array)
#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/DB\\\/QueryBuilder\\\/QueryBuilder.php(141): Doctrine\\\\DBAL\\\\Query\\\\QueryBuilder->execute()
#5 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/SystemTag\\\/SystemTagManager.php(403): OC\\\\DB\\\\QueryBuilder\\\\QueryBuilder->execute()
#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/SystemTag\\\/SystemTagPlugin.php(205): OC\\\\SystemTag\\\\SystemTagManager->setTagGroups(Object(OC\\\\SystemTag\\\\SystemTag), Array)
#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/SystemTag\\\/SystemTagPlugin.php(132): OCA\\\\DAV\\\\SystemTag\\\\SystemTagPlugin->createTag('{\\\"name\\\":\\\"Gedoen...', 'application\\\/jso...')
#8 [internal function]: OCA\\\\DAV\\\\SystemTag\\\\SystemTagPlugin->httpPost(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))
#9 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/event\\\/lib\\\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)
#10 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(459): Sabre\\\\Event\\\\EventEmitter->emit('method:POST', Array)
#11 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(248): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))
#12 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Server.php(178): Sabre\\\\DAV\\\\Server->exec()
#13 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v2\\\/remote.php(30): OCA\\\\DAV\\\\Server->exec()
#14 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/remote.php(164): require_once('\\\/srv\\\/www\\\/htdocs...')
#15 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/3rdparty\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Driver\\\/AbstractOracleDriver.php\",\"Line\":68,\"User\":\"xxx\"}","level":4,"time":"2017-03-02T08:26:52+01:00","method":"POST","url":"\/remote.php\/dav\/systemtags\/","user":"xxx"}
{"reqId":"WLfJPKwV0uMAAFnMeU4AAAAC","remoteAddr":"10.181.130.238","app":"stats","message":"request took 0.35226702690125","level":0,"time":"2017-03-02T08:26:52+01:00","method":"POST","url":"\/remote.php\/dav\/systemtags\/","user":"xxx"}

I assume in mysql we have empty strings in that table ...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 2, 2017

Member

Hmm, that must be a bug. It is not supposed to write empty gids into that table.

Maybe the calling code somehow managed to produce an array [''] for some reason, maybe while parsing the DAV input. Anyway, your fix is fine.

Member

PVince81 commented Mar 2, 2017

Hmm, that must be a bug. It is not supposed to write empty gids into that table.

Maybe the calling code somehow managed to produce an array [''] for some reason, maybe while parsing the DAV input. Anyway, your fix is fine.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 2, 2017

Member

hm https://github.com/owncloud/systemtags_management/blob/master/js/systemtags_management.js#L114 leads to JSON containing groups:'', which then is picked up by https://github.com/owncloud/core/blob/master/apps/dav/lib/SystemTag/SystemTagPlugin.php#L187-L192

<?php var_dump(explode('|', ''));

produces

/home/jfd/www/stable9.1/emptystring.php:1:
array (size=1)
  0 => string '' (length=0)

The explode creates an array with an emptystring element

Member

butonic commented Mar 2, 2017

hm https://github.com/owncloud/systemtags_management/blob/master/js/systemtags_management.js#L114 leads to JSON containing groups:'', which then is picked up by https://github.com/owncloud/core/blob/master/apps/dav/lib/SystemTag/SystemTagPlugin.php#L187-L192

<?php var_dump(explode('|', ''));

produces

/home/jfd/www/stable9.1/emptystring.php:1:
array (size=1)
  0 => string '' (length=0)

The explode creates an array with an emptystring element

butonic added some commits Mar 2, 2017

don't store empty groupids
gives an exception on oracle and shouldn't be added in the first place
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 2, 2017

Member

The explode creates an array with an emptystring element

Ah yes... unintuitive... Need to filter this out.

Member

PVince81 commented Mar 2, 2017

The explode creates an array with an emptystring element

Ah yes... unintuitive... Need to filter this out.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 2, 2017

Member

The test shows what should not happen:

14:23:17 1) Test\SystemTag\SystemTagManagerTest::testEmptyTagGroup
14:23:17 Failed asserting that two arrays are equal.
14:23:17 --- Expected
14:23:17 +++ Actual
14:23:17 @@ @@
14:23:17  Array (
14:23:17 +    0 => ''
14:23:17  )

this is for sqlite though. will submit fix now.

Member

butonic commented Mar 2, 2017

The test shows what should not happen:

14:23:17 1) Test\SystemTag\SystemTagManagerTest::testEmptyTagGroup
14:23:17 Failed asserting that two arrays are equal.
14:23:17 --- Expected
14:23:17 +++ Actual
14:23:17 @@ @@
14:23:17  Array (
14:23:17 +    0 => ''
14:23:17  )

this is for sqlite though. will submit fix now.

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 2, 2017

Member

weird. github does not reflect the order of commits as seen here:

commit a5b0e2e5af52d6057878dc6d4f14d912e0d417d1
Author: Jörn Friedrich Dreyer <jfd@butonic.de>
Date:   Thu Mar 2 11:36:20 2017 +0100

    don't store empty groupids
    
    gives an exception on oracle and shouldn't be added in the first place

commit 357386bbf6e2bd24fae8d67c9088db50962226d3
Author: Jörn Friedrich Dreyer <jfd@butonic.de>
Date:   Thu Mar 2 14:17:20 2017 +0100

    test setTagGroups with empty groupids

Member

butonic commented Mar 2, 2017

weird. github does not reflect the order of commits as seen here:

commit a5b0e2e5af52d6057878dc6d4f14d912e0d417d1
Author: Jörn Friedrich Dreyer <jfd@butonic.de>
Date:   Thu Mar 2 11:36:20 2017 +0100

    don't store empty groupids
    
    gives an exception on oracle and shouldn't be added in the first place

commit 357386bbf6e2bd24fae8d67c9088db50962226d3
Author: Jörn Friedrich Dreyer <jfd@butonic.de>
Date:   Thu Mar 2 14:17:20 2017 +0100

    test setTagGroups with empty groupids

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 2, 2017

Member

"butonic-patch-2" something tells me you've been editing files directly on Github, didn't you ? 😉

Member

PVince81 commented Mar 2, 2017

"butonic-patch-2" something tells me you've been editing files directly on Github, didn't you ? 😉

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 2, 2017

Member

the initial three line fix was a copy'n'paste from a patch sent to the customer. Web ui was faster than CLI & IDE magic

Member

butonic commented Mar 2, 2017

the initial three line fix was a copy'n'paste from a patch sent to the customer. Web ui was faster than CLI & IDE magic

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 3, 2017

Member

fixes problem at the customer. backport to 9.1 requested

Member

butonic commented Mar 3, 2017

fixes problem at the customer. backport to 9.1 requested

@butonic butonic requested a review from phisch Mar 3, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 3, 2017

Member

👍

Member

PVince81 commented Mar 3, 2017

👍

@PVince81 PVince81 merged commit ce48d6f into master Mar 3, 2017

4 checks passed

Scrutinizer No new issues
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the butonic-patch-2 branch Mar 3, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 3, 2017

Member

@butonic please backport to 9.1 then (or let us know if someone else should do it)

Member

PVince81 commented Mar 3, 2017

@butonic please backport to 9.1 then (or let us know if someone else should do it)

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Mar 3, 2017

Member

please take over

Member

butonic commented Mar 3, 2017

please take over

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 3, 2017

Member

stable9.1: #27300

Member

PVince81 commented Mar 3, 2017

stable9.1: #27300

@MorrisJobke MorrisJobke referenced this pull request in nextcloud/server Mar 20, 2017

Merged

Don't store empty groupids #3931

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