GROUP_BASE_SECURITY_R0

[boyan]

GROUP_BASE_SECURITY_R0

Ticket	Summary	Effort	Status
#315	BASE_SECURITY_MODEL_USERS_R0	1	closed
#318	BASE_SECURITY_MODEL_GROUPS_R0	1	closed
#1294	S2S_CORE_SECURITY_R1	1	closed

[boyan]

starting analysis

[boyan]

finished in a total of 3h

[dido]

It will be useful if the user could manage (or just to see) in witch groups he belongs. So I guess this info is also kept somehow in the user profile, also does the groups contain a list of members.
review analysis 3.5p (30m)

[boyan]

the group contains a list of members (mentioned in the implementation idea), this comment will be considered in the design and implementation

starting design

[boyan]

finsihed in a total of 3h

[meddle]

This design will work, but I had a discussion with Milo about the security and we saw that in the past there were no resource references, so we needed the services to find all groups for user or all users in group, now when we have the references, we can have for example property "groups" in User and "users" in UserGroup that return the resource refs, so the methods that do that in the services will be unnecessary and methods such like getGroupByName and getUserByUsername of the services will work with that new properties.
Other thing that we forgot is that every UserGroup needs an administrator (owner user), that created it and can add/remove users from it if the group is something like private UserGroup.

I will fail this design because that things are important to have in mind, their are small thing to fix. We need stable designs, that's why I asked Milo and we agreed on the above ideas.

2.5p (50m)

[boyan]

starting redesign

[boyan]

finished in 30m

[meddle]

The design is OK, the only thing I'm concert is if this private property is necessary, because in the future there will be class for the Permissions of groups and users, but may be it will have meaning only with books, I don't know, for now that works for me...

3p (10m)

[boyan]

starting implementation

[boyan]

finished implementation

[meddle]

• Problems with the tests:
  • There are not running tests, that were running before like ServerGroupManagerTest... The reason is that you don't have mechanism to tell if a given resource is already persisted and use the old one with the "-1" ids... Maybe your MockPersister needs a method that checks if a resource is already persisted...
  • You must not broke already running tests! If the refactoring is successful that will be no problem.
• Problems with the implementation itself:
  • I don't like the idea that the MockPersister is something out of the old hierarchies, I mean it can extend the DatabaseManager. And the name is bad, that is not a Sophie2 Persister, it's something like MemoryDatabaseManager...
  • It is good the new DatabaseManagers to work with Resources and not AbstractEntities, good idea, but why you saved the old id of the AbstractEntity then?? Work only eith the entity ids. Removing the old id will point to the places where the checks if a given resource is persisted are.
  • Your managers have to work with the services but there are places where they have repeted code that is already in the services, like in the ServerGroupManager:

    	@Override
    	public List<User> getUsers(UserGroup group) {
    		return group.users().get().transform(
    				new ProListTransformer<ResourceRef, User>() {
    
    					@Override
    					public User translate(ResourceRef source) {
    						return source.get(User.class);
    					}
    				});
    	}
    

  • The code is not runnable through the true runner, because you have no run parameters, and now every runner has run parameter, your will be "server". The other are "reader" and "author"...
• Problems with the branch status:
  • There is no revision when you are adding your new security module for the first time... That is very bad, and makes the code unmergeable... I succeeded, copying the module from your branch, but was hard, time-taking and buggy, please make a clean branch and give me all the thing in one revision. I'm talking about the code till now and the fixes for the above notes.

2p (110m)

[boyan]

restarting implementation

[boyan]

reimplementation finished in about 4 hours

[meddle]

This time I'll pass the implementation but with the following notes:

• Configuration related stuff:
  • You didn't add all your modules to the server.bundles.config file, so you didn't test with the true run configuration!
• Logic related notes:
  • Why didn't you add the books to your fake DB, now we have exceptions when going to the pages related to the groups!
• Code related:
  • The isUserpersisted method in the fake implementation of DatabaseManager is more appropriate to be in the FakeUserDao...

The last one you can fix in your new GROUP_APP_SERVER_RESOURCE_ACCESS_R0 task.

3p (150m)

[deyan]

Batch update from file query-obsoleted.csv