Ticket #1697 (closed unplanned_task: obsolete)

Opened 10 years ago

Last modified 10 years ago

GROUP_BASE_SECURITY_R0

Reported by: boyan Owned by: boyan
Priority: major Milestone: M08_ALPHA3
Component: BASE_SECURITY_MODEL Version: 2.0
Keywords: users,groups,security,model,base Cc:
Category: BASE Effort: 0
Importance: Ticket_group:
Estimated Number of Hours: Add Hours to Ticket:
Billable?: Total Hours:
Analysis_owners: boyan Design_owners: boyan
Imp._owners: boyan Test_owners:
Analysis_reviewers: dido Changelog:
Design_reviewers: meddle, meddle Imp._reviewers: meddle, meddle
Test_reviewers: Analysis_score: 3.5
Design_score: 3 Imp._score: 3
Test_score: 0

Change History

comment:1 Changed 10 years ago by boyan

  • Status changed from new to s1a_analysis_started
  • Description modified (diff)
  • Analysis_owners set to boyan

starting analysis

comment:2 Changed 10 years ago by boyan

  • Status changed from s1a_analysis_started to s1b_analysis_finished

finished in a total of 3h

comment:3 Changed 10 years ago by dido

  • Status changed from s1b_analysis_finished to s1c_analysis_ok
  • Analysis_reviewers set to dido
  • Analysis_score changed from 0 to 3.5

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)

comment:4 Changed 10 years ago by boyan

  • Design_owners set to boyan
  • Status changed from s1c_analysis_ok to s2a_design_started

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

starting design

comment:5 Changed 10 years ago by boyan

  • Status changed from s2a_design_started to s2b_design_finished

finsihed in a total of 3h

comment:6 Changed 10 years ago by meddle

  • Status changed from s2b_design_finished to s1c_analysis_ok
  • Design_score changed from 0 to 2.5
  • Design_reviewers set to 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)

comment:7 Changed 10 years ago by boyan

  • Status changed from s1c_analysis_ok to s2a_design_started

starting redesign

comment:8 Changed 10 years ago by boyan

  • Status changed from s2a_design_started to s2b_design_finished

finished in 30m

comment:9 Changed 10 years ago by meddle

  • Status changed from s2b_design_finished to s2c_design_ok
  • Design_score changed from 2.5 to 3
  • Design_reviewers changed from meddle to meddle, 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)

comment:10 Changed 10 years ago by boyan

  • Status changed from s2c_design_ok to s3a_implementation_started
  • Imp._owners set to boyan

starting implementation

comment:11 Changed 10 years ago by boyan

  • Status changed from s3a_implementation_started to s3b_implementation_finished

finished implementation

comment:12 Changed 10 years ago by meddle

  • Status changed from s3b_implementation_finished to s2c_design_ok
  • Imp._score changed from 0 to 2
  • Imp._reviewers set to 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)

comment:13 Changed 10 years ago by boyan

  • Status changed from s2c_design_ok to s3a_implementation_started

restarting implementation

comment:14 Changed 10 years ago by boyan

  • Status changed from s3a_implementation_started to s3b_implementation_finished

reimplementation finished in about 4 hours

comment:15 Changed 10 years ago by meddle

  • Status changed from s3b_implementation_finished to s3c_implementation_ok
  • Imp._score changed from 2 to 3
  • Imp._reviewers changed from meddle to meddle, 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)

comment:16 Changed 10 years ago by deyan

  • Status changed from s3c_implementation_ok to closed
  • Resolution set to obsolete

Batch update from file query-obsoleted.csv

Note: See TracTickets for help on using tickets.