Central membership and group table #29107

mrow4a · 2017-09-27T12:30:30Z

Related Issue

fixes #27623

Plan

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
- currently new methods are added to OCP\GroupInterface

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

How to test it:

https://github.com/owncloud/core/wiki/How-to-test-user_ldap

Main feature:

core/Migrations/Version20170927145820.php

lib/private/Group/BackendGroup.php
lib/public/AppFramework/Db/Mapper.php
lib/private/Group/GroupMapper.php
lib/private/MembershipManager.php

lib/private/Group/Group.php

lib/public/IGroupManager.php
lib/private/Group/Manager.php
lib/private/SubAdmin.php

Main sync feature:

lib/private/Group/SyncService.php
core/Command/Group/SyncBackend.php
core/register_command.php

Main changes:

lib/private/Group/Database.php
lib/private/User/Account.php
lib/private/User/AccountMapper.php
lib/private/User/Manager.php
lib/private/User/User.php

Required changes:

lib/base.php
lib/private/Server.php
lib/private/Setup.php
lib/private/Share/Share.php

settings/ajax/togglegroups.php
settings/users.php

core/Command/Group/ListGroups.php

Tests:

apps/federatedfilesharing/tests/TestCase.php
apps/files_sharing/tests/SharedMountTest.php
apps/files_sharing/tests/TestCase.php
apps/files_trashbin/tests/StorageTest.php
apps/provisioning_api/tests/AppsTest.php
apps/provisioning_api/tests/TestCase.php
tests/Core/Command/Group/AddMemberTest.php
tests/Core/Command/Group/AddTest.php
tests/Core/Command/Group/ListGroupMembersTest.php
tests/Core/Command/Group/ListGroupsTest.php
tests/Core/Command/Group/RemoveMemberTest.php
tests/Core/Command/Group/SyncBackendTest.php
tests/Core/Command/User/ListUserGroupsTest.php
tests/lib/App/ManagerTest.php
tests/lib/AppTest.php
tests/lib/Files/Config/UserMountCacheTest.php
tests/lib/Files/ViewTest.php
tests/lib/Group/Backend.php
tests/lib/Group/DatabaseTest.php
tests/lib/Group/DummyTest.php
tests/lib/Group/GroupMapperTest.php
tests/lib/Group/GroupTest.php
tests/lib/Group/ManagerTest.php
tests/lib/Group/SyncServiceTest.php
tests/lib/MembershipManagerTest.php
tests/lib/Share/ShareTest.php
tests/lib/SubAdminTest.php
tests/lib/Traits/GroupTrait.php
tests/lib/Traits/UserTrait.php
tests/lib/User/AccountMapperTest.php
tests/lib/User/ManagerTest.php
tests/lib/User/SyncServiceTest.php
tests/lib/User/UserTest.php
tests/lib/Util/Group/Dummy.php
tests/lib/Util/Group/MemoryGroupMapper.php
tests/lib/Util/MemoryMembershipManager.php
tests/lib/Util/User/MemoryAccountMapper.php

tomneedham · 2017-09-27T18:25:21Z

lib/private/Group/GroupEntity.php

+		}
+
+		return \OC::$server->getGroupManager()->getBackend($backendClass);
+	}


will we support a displayname as well?

Is it requirement for LDAP?

DeepDiver1975 · 2017-10-02T06:18:01Z

lib/private/Group/BackendGroup.php

+ */
+class BackendGroup extends Entity {
+
+	protected $groupId;


why? the table has id, group_id, display_name and backend
I intend to add a uuid column because group_id is more like a group_name. groups might be renamed. Display name is different, typically being more verbose.

I think because the entity class handles these properties for you. Not that you don't need a groupId

the entity only handles id, group_id is something different. a lot of the other things could be removed though.

DeepDiver1975 · 2017-10-02T06:18:06Z

lib/private/Group/BackendGroup.php

+class BackendGroup extends Entity {
+
+	protected $groupId;
+	protected $displayName;


DeepDiver1975 · 2017-10-02T06:18:14Z

lib/private/Group/BackendGroup.php

+
+	protected $groupId;
+	protected $displayName;
+	protected $backend;


phpdoc missing

DeepDiver1975 · 2017-10-02T06:18:26Z

core/Migrations/Version20170927145820.php

+
+		//TODO: Do it later since it requires changes here for Oracle https://github.com/owncloud/core/blob/master/lib/private/DB/OracleMigrator.php#L158-L160
+		// Add foreign keys on backend_group and accounts tables
+		//$table->addForeignKeyConstraint("{$prefix}backend_groups",array('backend_group_id'), array('id'));


please add right away

DeepDiver1975 · 2017-10-02T06:19:19Z

core/Migrations/Version20170927145820.php

+			'notnull' => true,
+			'length' => 64,
+		]);
+		$table->setPrimaryKey(['id']);


no further indexes?

I did not decide on it yet.

DeepDiver1975 · 2017-10-02T06:22:27Z

tests/lib/Group/GroupMapperTest.php

+	}
+
+	public static function tearDownAfterClass () {
+		\OC::$server->getDatabaseConnection()->rollBack();


I see - hmmmm - does this work?

DeepDiver1975 · 2017-10-02T06:22:45Z

tests/lib/Group/GroupMapperTest.php

+	 */
+	public function testGet() {
+		$result = $this->mapper->getGroup("TestFind1");
+		$this->assertInstanceOf("OC\Group\BackendGroup", $result);


BackendGroup::class

DeepDiver1975 · 2017-10-02T06:23:10Z

tests/lib/Group/GroupMapperTest.php

+		$backendGroup->setDisplayName("TestFind5");
+		$backendGroup->setBackend(self::class);
+
+		$mapper = \OC::$server->getGroupMapper();


use this->mapper

DeepDiver1975 · 2017-10-02T06:24:30Z

version.php


 // The human readable string
-$OC_VersionString = '10.0.3';
+$OC_VersionString = '10.0.4';


no - keep 10.0.3

I think if you change the schema, you need to increment version.

Not the version string but oc_version

DeepDiver1975 · 2017-10-02T06:24:43Z

version.php

@@ -25,10 +25,10 @@
 // We only can count up. The 4. digit is only for the internal patchlevel to trigger DB upgrades
 // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel
 // when updating major/minor version number.
-$OC_Version = [10, 0, 3, 3];


no - 10.0.3.4

DeepDiver1975 · 2017-10-02T10:50:09Z

tests/lib/DB/MigratorTest.php

+		$migrator->migrate($startSchema);
+		$migrator->migrate($startFKSchema);
+
+		$this->assertTrue(true);


we should assert that the fk was set

DeepDiver1975 · 2017-10-02T10:51:32Z

lib/private/DB/OracleMigrator.php

+				return $this->quoteIndex($index);
+			}, $tableDiff->addedForeignKeys);
+
+			$tableDiff->changedForeignKeys = array_map(function(Index $index) {


ForeignKeyConstraint not Index

Oh, yes, sorry

DeepDiver1975 · 2017-10-05T19:59:43Z

foreign key pr is merged to master - please rebase - THX

mrow4a · 2017-10-05T21:07:12Z

One of the approaches proposed in #29154

mrow4a · 2017-10-09T13:05:24Z

@jvillafanez

jvillafanez · 2017-10-10T06:55:30Z

lib/private/Server.php

@@ -232,13 +233,16 @@ public function __construct($webRoot, \OC\Config $config) {
 		$this->registerService('AccountMapper', function(Server $c) {
 			return new AccountMapper($c->getConfig(), $c->getDatabaseConnection(), new AccountTermMapper($c->getDatabaseConnection()));
 		});
+		$this->registerService('GroupMapper', function(Server $c) {


I wonder if this should be here. The groupMapper isn't intended to be used isolated and it will depend on the groupManager.
I guess we can let it pass because the accountMapper is also exposed...

It is not exposed because as per definition developers are only allowed to work on interface which are in the OCP namespace.

jvillafanez · 2017-10-10T07:18:56Z

lib/public/AppFramework/Db/Access.php

+ * may be subject to change in the future
+ * @since 10.0.0
+ */
+abstract class Access {


I'm unsure of the purpose of this class... maybe it's the name what doesn't fit...

Just some personal recommendations to recheck:

Make sure the purpose of this class is clear. This implies that only the specific methods to fufill its purpose should be there.

Make sure the abstract class has clear extension points, and evaluate how subclasses will complete this abstract class.

I guess this second point is what bothers me: abstract classes should have at least an abstract method that subclasses should implement. This isn't the case here.
Inheritance doesn't seem the way to go in this case. I'd rather use composition: just create the "Access" class and inject it wherever its needed.

jvillafanez · 2017-10-10T07:23:06Z

tests/lib/Group/GroupMapperTest.php

+		parent::setUpBeforeClass();
+		$mapper = \OC::$server->getGroupMapper();
+
+		\OC::$server->getDatabaseConnection()->beginTransaction();


I'd add a comment to make sure the trick won't be forgotten, otherwise it's quite weird that you have one transaction hanging around.

…Add unit tests

…d, and only allow memberships to be synced on login

mrow4a · 2018-03-11T22:22:18Z

@PVince81 @DeepDiver1975 @butonic @tomneedham I have been on the customgroups and guest, and I can see that they will need sync on login also.. thus I remove the requirement of the flag isSyncMaintained. LDAP will work out of the box with this PR since all the things will be synced on user login. Custom Groups also works, but will require small adjustment to work better

mrow4a · 2018-05-06T10:09:53Z

Updated the checklist for the discussions later if we want to go the direction proposed here.

PVince81 · 2018-07-24T12:43:45Z

lots of conflicts, PR needs reviving

mrow4a · 2018-08-07T06:07:52Z

@PVince81 I will resolve the conflicts and summarize advantages of this solution and disadvantages - last time I posted a report was in March (https://cloud.owncloud.com/index.php/s/eUajBR59vQNUEq1#pdfviewer), and there was no review. But by then central accounts were also fresh

DeepDiver1975 · 2019-07-30T06:01:21Z

As discussed in #35777 the master branch will from now on hold the ownCloud 10 codebase.

This PR targetted ownCloud 11 which is postponed to a far distant future.

Because of that I'm closing this PR and kindly ask you to re-submit this PR in a few days.

Thanks a lot for your patience

mrow4a added the 2 - Developing label Sep 27, 2017

mrow4a force-pushed the centr_groups branch 3 times, most recently from 2059e50 to f3dc66e Compare September 27, 2017 17:43

tomneedham reviewed Sep 27, 2017

View reviewed changes

mrow4a force-pushed the centr_groups branch 2 times, most recently from a2d2fdf to 8c6131d Compare September 27, 2017 19:14

tomneedham assigned mrow4a Sep 27, 2017

tomneedham added this to the development milestone Sep 27, 2017

mrow4a force-pushed the centr_groups branch 5 times, most recently from 9540ea0 to 84340ed Compare September 28, 2017 22:47

DeepDiver1975 requested changes Oct 2, 2017

View reviewed changes

mrow4a force-pushed the centr_groups branch from 5183985 to 22de02f Compare October 2, 2017 10:45

DeepDiver1975 requested changes Oct 2, 2017

View reviewed changes

mrow4a force-pushed the centr_groups branch 5 times, most recently from 0bad371 to 4caa4fc Compare October 2, 2017 15:00

mrow4a mentioned this pull request Oct 5, 2017

[Central group table] Split Mapper into two, more specialized classes #29152

Closed

mrow4a mentioned this pull request Oct 5, 2017

[Central group table] Add membership manager to central group table #29154

Merged

mrow4a force-pushed the centr_groups branch from 93ef106 to 240150d Compare October 5, 2017 21:06

mrow4a requested a review from jvillafanez October 9, 2017 13:10

jvillafanez reviewed Oct 10, 2017

View reviewed changes

mrow4a and others added 7 commits March 11, 2018 23:18

Generalize Membership Manager and add new column - maintenance_type. …

d84964e

…Add unit tests

Group sync service and sync backend command. Add unit tests

dc97dba

Adjust unit tests to SyncService refactor

9ba643e

cleanup whitespace, imports, is_null, entity

be29e21

Allow to sync on login with unit tests and group manager refactor

029a4e1

Allow to sync groups only with occ group:sync --groups-only .. comman…

1fd05e8

…d, and only allow memberships to be synced on login

createFromBackend will either create or sync

b24aae4

mrow4a force-pushed the centr_groups branch from 27d28cc to 707ac94 Compare March 11, 2018 22:20

mrow4a mentioned this pull request Mar 11, 2018

[WIP] Compatibility with central group and membership table owncloud/user_ldap#190

Closed

Dont use isSyncMaintained

b691b7f

mrow4a force-pushed the centr_groups branch from 707ac94 to b691b7f Compare March 12, 2018 00:12

ownclouders added the status/STALE label Mar 20, 2018

felixboehm modified the milestones: development, planned Apr 10, 2018

ownclouders added v11 and removed status/STALE labels Apr 24, 2018

ownclouders added the status/STALE label May 2, 2018

ownclouders removed the status/STALE label May 6, 2018

mrow4a mentioned this pull request May 6, 2018

Refactor and cleanup guests app. Add unit tests owncloud/guests#200

Closed

7 tasks

ownclouders added the status/STALE label May 14, 2018

ownclouders removed the status/STALE label Jul 24, 2018

PVince81 modified the milestones: development, backlog Jul 24, 2018

DeepDiver1975 closed this Jul 30, 2019

DeepDiver1975 deleted the centr_groups branch July 30, 2019 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Central membership and group table #29107

Central membership and group table #29107

mrow4a commented Sep 27, 2017 •

edited

Loading

tomneedham Sep 27, 2017

mrow4a Sep 27, 2017

DeepDiver1975 Oct 2, 2017

butonic Feb 20, 2018

tomneedham Feb 20, 2018

butonic Feb 20, 2018

DeepDiver1975 Oct 2, 2017

butonic Feb 20, 2018

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

mrow4a Oct 2, 2017

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

mrow4a Oct 2, 2017

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

DeepDiver1975 Oct 2, 2017

mrow4a Oct 2, 2017

DeepDiver1975 commented Oct 5, 2017

mrow4a commented Oct 5, 2017

mrow4a commented Oct 9, 2017

jvillafanez Oct 10, 2017

DeepDiver1975 Oct 10, 2017

jvillafanez Oct 10, 2017

jvillafanez Oct 10, 2017

mrow4a commented Mar 11, 2018

mrow4a commented May 6, 2018

PVince81 commented Jul 24, 2018

mrow4a commented Aug 7, 2018

DeepDiver1975 commented Jul 30, 2019

Central membership and group table #29107

Central membership and group table #29107

Conversation

mrow4a commented Sep 27, 2017 • edited Loading

Related Issue

Plan

Types of changes

Checklist:

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

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

DeepDiver1975 commented Oct 5, 2017

mrow4a commented Oct 5, 2017

mrow4a commented Oct 9, 2017

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

mrow4a commented Mar 11, 2018

mrow4a commented May 6, 2018

PVince81 commented Jul 24, 2018

mrow4a commented Aug 7, 2018

DeepDiver1975 commented Jul 30, 2019

mrow4a commented Sep 27, 2017 •

edited

Loading