Implement group backend #1

PVince81 · 2016-11-29T14:04:23Z

Implement group manager.

To test:

Enable app
Create three users "user1", "user2", "user3"
insert into oc_custom_group values (NULL, 'myfriends', 'My friends');
insert into oc_custom_group_member values (1, 'user2', 1);
insert into oc_custom_group_member values (1, 'user3', 0);
Login as "user1"
Create a folder "test"
Share with "My fri" let it autocomplete: select "customgroup_1" (need future work to support display names)
Login as "user2" and see that the folder is visible

Some concerns:

current impl lets any user share with anybody, unless admin ticks in admin page "only allow sharing with groups in which user is member", this could be fine as is...
need display name support in core, might need to change some group manager APIs to support returning arrays instead of just the group ids

@DeepDiver1975 @butonic @jvillafanez what do you think ?

CLAassistant · 2016-11-29T14:04:24Z

All committers have signed the CLA.

PVince81 · 2016-11-29T14:07:15Z

also:

custom groups are visible in user manager. Can't hide them because making getGroups return an empty would break searching in the sharing autocomplete. Might need changing core APIs to let the manager specify if listable or not.

jvillafanez · 2016-11-29T15:27:59Z

lib/CustomGroupsManager.php

+class CustomGroupsManager implements \OCP\GroupInterface {
+
+	const GROUP_ID_PREFIX = 'customgroup_';
+	const GROUP_ID_PREFIX_LEN = 12;


I don't think it's a good idea to hardcode this. The extra cost of calculating the string's length should be meaningless, and we would only maintain one constant.

jvillafanez · 2016-11-29T15:29:00Z

lib/CustomGroupsManager.php

+		// FIXME: search by display name instead
+		if ($search !== '') {
+			$parameters[] = '%' . $search . '%';
+			$searchLike = ' WHERE LOWER(`displayname`) LIKE LOWER(?)';


Do we want lowercase comparations?

For display names I'd say yes

jvillafanez · 2016-11-29T15:44:02Z

lib/CustomGroupsManager.php

+	 * @return bool
+	 */
+	public function inGroup($uid, $gid) {
+		$numericGroupId = $this->extractNumericGroupId($gid);


Isn't this a bit complex? Why do we want to use prefix?

The prefix is to avoid collisions with groups created in other backends like LDAP, etc.

Also we're using a numeric group id in this backend internally to be able to rename the display name.
Another alternative would be to generate a random hash string id instead of numeric... not sure what's best, I thought this approach would be the simplest.

I don't think it's the backend responsability to resolve the collisions nor to prevent them. Any other backend could have a group with the same name "customgroup_12", and there shouldn't be any reason to avoid it.

I think it's core the one who should take care of how to resolve the collisions. Generally, just saying "I want this group" is enough to know what is the group you're refering to, but maybe sometimes you should say "I want this group which is located in this backend" so there is no doubt about the group you want.

While I agree that it's not this backend's responsibility, we'll still get into trouble if all our groups are called "1", "2", "3", etc... And I don't intend to fix core to have its own collision detection / namespacing as part of this task, which is likely a bigger task on its own which might not be backward compatible.

PVince81 · 2016-11-29T15:53:39Z

TODO: add unit tests
TODO: remove LEN constant

jvillafanez · 2016-11-29T15:58:22Z

Just a couple of extra things:

From my point of view ( @DeepDiver1975 correct me if I'm wrong) there should be only one group manager and user manager, which are the ones provided by core. Creating and using another manager shouldn't be possible because both core and any app are interested in using the "official" manager.
Custom backends, such as LDAP or this one could be registered in the core manager. This also includes the DB backend which is used by default in ownCloud.

PVince81 · 2016-11-29T16:02:25Z

@jvillafanez yes sorry, there is only one group manager.

What this PR implements is a group backend like LDAP does. (I mixed up both names...)

PVince81 · 2016-11-29T16:19:24Z

might need adjusting the whole PR depending on how we implement the new group backend interfaces as described in Group name vs display name core#7918 (comment) for display name support

PVince81 · 2016-11-29T16:19:55Z

Anyway, my first goal with this PR was to experiment to find out whether sharing would work automatically with this approach. It did!

PVince81 · 2016-12-01T14:06:36Z

now requires Adding group display name support in group backends core#26750 for display name support and the new IGroupBackend interface

If you test this you'll see that the display name is correctly shown in the sharee list 😄

PVince81 · 2016-12-03T17:40:40Z

TODO: every member can also be a group admin, need to change the data structure

PVince81 · 2016-12-05T19:16:30Z

Ok, I spent too much time already implementing the Sabre stuff but not enough writing actual tests for GroupsManager 😄

Will do tomorrow so we can get this first PR merged soon.

PVince81 · 2016-12-06T11:56:25Z

I've added unit tests now for the group backend and group manager.
Note that for the tests to run properly you also need to checkout the matching core branch owncloud/core#26750
(or ideally review that one first).

I've removed the DAV stuff from this PR into a separate PR: #7

Please review @DeepDiver1975 @jvillafanez

PVince81 · 2016-12-08T10:39:38Z

@DeepDiver1975 mind reviewing so I can continue with the DAV stuff ? Thanks

jvillafanez · 2016-12-09T08:36:20Z

lib/CustomGroupsManager.php

+/**
+ * Manager for custom group
+ */
+class CustomGroupsManager {


I'm questioning the purpose of this class... does it make the code too complex if we move this code to the backend?

Anyway, we should rename the class. The backend class will have a manager above (provided by core, where the backend will be added) and also below (this class) which is confusing. CustomGroupsDBHandler seems a better choice.

The purpose of the manager is to be used only by this one app to manage custom groups.

The backend itself only uses a subset of operations that it exposes to core because we don't want the users page to allow managing these, but we still want the search to work for the sharing autocomplete. Furthermore the backend class needs to work with core-style group ids (strings) while the manager works with this app's specific numeric id, so a conversion needs to be done.
I prefer to keep these separate to make the code clearer.

If I would move the extra operations to manage group members to the backend class as well, I don't want to expose this to the core interface. So we'd have two methods addToGroup, one that is empty for the backend and another one that is only known by the customgroups app. That's not very clean and clear to understand.

Hope this clears it out a bit 😄

ok, got it, but I'd still rename the class to reduce confusion.

you're right, this is more a database accessor. But we never used the name "DBHandler" before.

In core we used manager for "CommentsManager" and "SystemTagsManager" which do the same.
Some older call calls it "Mapper" like "TagMapper", but it's mostly because they use real objects instead of just arrays.

What do you think ?

jvillafanez · 2016-12-09T08:51:06Z

appinfo/database.xml

+				<notnull>false</notnull>
+			</field>
+			<index>
+				<name>cg_gid_uid_index</name>


I'm not sure if we should add another index only for the user_id. I expect the queries for groups will use this index, but I don't think the queries for users will.

Ah, I thought that this was the proper way to define multi-column primary unique keys ?
I copied this idea from core's group_user table.

@DeepDiver1975

The multi-column index is fine because we want group_id + user_id be unique. I trust the DBMSs be smart enough to use this index when we query for group information, but I don't think this will happen for user information.

jvillafanez · 2016-12-09T08:59:49Z

lib/Application.php

+	 * Register the group manager
+	 */
+	public function registerGroupBackend() {
+		$backend = $this->getContainer()->query('\OCA\CustomGroups\CustomGroupsBackend');


I don't know where this come from.

What do you mean ? This method is called in appinfo/app.php

The \OCA\CustomGroups\CustomGroupsBackend needs to be register in the container, but I don't know where this registration is made.

No, it doesn't. DI container power !

It works because the class name matches exactly so it's able to resolve it.
You only need explicit registration when the names different or when your constructor requires parameters that cannot be injected (ex: strings)

PVince81 · 2016-12-09T09:30:05Z

Restarted travis build now that master contains the code we need

PVince81 · 2016-12-09T09:41:49Z

rename CustomGroupsManager to something that sounds more like it's a DB accessor

PVince81 · 2016-12-09T10:45:38Z

challenge: implement initial DB structure with doctrine migration

PVince81 · 2016-12-09T14:38:31Z

Looks like for the Mapper I can throw away almost all my code (and unit tests) and start over. I don't think the benefit of having the mapper pattern here is bringing any significant benefit at this point. (it might even introduce more hurdles for the API consumer)

I'll abandon this approach and simply rename the manager to CustomGroupsDatabaseHandler.

PVince81 · 2016-12-09T14:46:45Z

Raised #10 to use doctrine migrations for initial DB.

I'd like to get this PR here merged so I can move forward with the Sabre plugins / API. That is, unless there is a no-go in this PR.

PVince81 · 2016-12-12T10:23:43Z

Also, looks like sqlite not happy with my DB stuff. Will look into it. (Mysql worked fine)

PVince81 · 2016-12-12T10:53:40Z

Something is wrong with the unique constraint. Looks like SQLite set the primary key only on "group_id" instead of both "group_id" + "user_id" 😢

PVince81 · 2016-12-12T10:55:05Z

sqlite> pragma table_info(oc_custom_group_member);
cid|name|type|notnull|dflt_value|pk
0|group_id|INTEGER|1||1
1|user_id|VARCHAR(64)|1||0
2|is_admin|INTEGER|1|0|0

primary key is only set on group_id even though I asked it to set it on both:

    $table->setPrimaryKey(['group_id', 'user_id']);

debugging....

PVince81 · 2016-12-12T11:03:16Z

grrr, this is the resulting SQL generated by Doctrine:

  $sql[2]                        = (string[156]) 'CREATE TABLE oc_custom_group_member (group_id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, user_id VARCHAR(64) NOT NULL, is_admin INTEGER DEFAULT 0 NOT NULL)';

I didn't even ask for autoincrement... Looks like a Doctrine bug.

We do have a table in core with a similar schema and that one looks correct:

sqlite> pragma table_info(oc_group_user);
cid|name|type|notnull|dflt_value|pk
0|gid|VARCHAR(64)|1|''|1
1|uid|VARCHAR(64)|1|''|2

so this means SQLite supports multi-column primary keys but Doctrine doesn't seem to be able to create that.

Googling now, I hope there is maybe a missing flag or something in the API.

PVince81 · 2016-12-12T11:17:37Z

Facepalm, I forgot to remove "autoincrement" when copy-pasting the definitions.
Funnily enough, Mysql didn't complain.

PVince81 · 2016-12-12T11:18:55Z

@jvillafanez I fixed all your comments and additionally fixed the DB schema.

PVince81 · 2016-12-12T14:11:43Z

@DeepDiver1975 can you create the repo for clover coverage ? https://travis-ci.org/owncloud/customgroups/jobs/183218691#L831

jvillafanez · 2016-12-13T07:40:56Z

👍

PVince81 · 2016-12-13T07:58:46Z

I'm going to remove the code coverage thing for now and let @DeepDiver1975 sort it out separately.

PVince81 · 2016-12-13T08:01:25Z

Raised here #12

jvillafanez reviewed Nov 29, 2016

View reviewed changes

PVince81 changed the title ~~Implement group manager~~ Implement group backend Nov 29, 2016

PVince81 mentioned this pull request Nov 29, 2016

Basic implementation #2

Closed

9 tasks

PVince81 self-assigned this Dec 3, 2016

PVince81 added this to the 9.2 milestone Dec 3, 2016

PVince81 mentioned this pull request Dec 5, 2016

Webdav API for groups management #3

Closed

28 tasks

PVince81 force-pushed the group-manager branch from 251b0e7 to 762497f Compare December 6, 2016 11:54

PVince81 mentioned this pull request Dec 6, 2016

DAV endpoint #7

Merged

1 task

PVince81 added the 3 - To Review label Dec 6, 2016

PVince81 force-pushed the group-manager branch from 762497f to d4b48b0 Compare December 6, 2016 12:42

PVince81 mentioned this pull request Dec 6, 2016

PHPUnit doesn't report all risky tests #8

Closed

jvillafanez reviewed Dec 9, 2016

View reviewed changes

PVince81 force-pushed the group-manager branch from 11f89fe to c2a442d Compare December 9, 2016 14:45

PVince81 force-pushed the group-manager branch from 397356c to bba7adc Compare December 12, 2016 10:15

Vincent Petry and others added 3 commits December 12, 2016 11:19

Implement database handler for custom groups

6589777

Use phpunit from core

0a469cb

Be strict about empty tests

0cfef05

Remove bogus autoincrement on members table

f0b5648

PVince81 force-pushed the group-manager branch from bba7adc to f0b5648 Compare December 12, 2016 11:18

Fix travis script

d64a029

PVince81 force-pushed the group-manager branch from c780116 to d64a029 Compare December 13, 2016 08:12

PVince81 merged commit 4ae443d into master Dec 13, 2016

PVince81 deleted the group-manager branch December 13, 2016 08:29

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

3c96b5c

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

c589dec

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

6b0ad17

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

2ebd050

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

cfbb19c

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

158c382

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

6dd856f

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups installable for Nextcloud 12.03 owncloud#1

211f796

fgsl pushed a commit to fgsl/customgroups that referenced this pull request Nov 3, 2017

Make Customgroups enable for Nextcloud 12.03 owncloud#1

ae4ebdf

Implement group backend #1

Implement group backend #1

Conversation

PVince81 commented Nov 29, 2016 • edited Loading

CLAassistant commented Nov 29, 2016 • edited Loading

PVince81 commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Nov 29, 2016 • edited Loading

jvillafanez commented Nov 29, 2016

PVince81 commented Nov 29, 2016

PVince81 commented Nov 29, 2016 • edited Loading

PVince81 commented Nov 29, 2016

PVince81 commented Dec 1, 2016 • edited Loading

PVince81 commented Dec 3, 2016 • edited Loading

PVince81 commented Dec 5, 2016

PVince81 commented Dec 6, 2016

PVince81 commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Dec 9, 2016

PVince81 commented Dec 9, 2016 • edited Loading

PVince81 commented Dec 9, 2016 • edited Loading

PVince81 commented Dec 9, 2016

PVince81 commented Dec 9, 2016

PVince81 commented Dec 12, 2016

PVince81 commented Dec 12, 2016

PVince81 commented Dec 12, 2016

PVince81 commented Dec 12, 2016

PVince81 commented Dec 12, 2016

PVince81 commented Dec 12, 2016

PVince81 commented Dec 12, 2016

jvillafanez commented Dec 13, 2016

PVince81 commented Dec 13, 2016

PVince81 commented Dec 13, 2016

PVince81 commented Nov 29, 2016 •

edited

Loading

CLAassistant commented Nov 29, 2016 •

edited

Loading

PVince81 commented Nov 29, 2016 •

edited

Loading

PVince81 commented Nov 29, 2016 •

edited

Loading

PVince81 commented Dec 1, 2016 •

edited

Loading

PVince81 commented Dec 3, 2016 •

edited

Loading

PVince81 commented Dec 9, 2016 •

edited

Loading

PVince81 commented Dec 9, 2016 •

edited

Loading