Hello Vyacheslav, It’s an excellent effort. Good attention to project coding and testing conventions. Good understanding of the underlying framework. As a post-mortem I’d like feedback from you wrt to how we can improve but there’s time for that later.
> On Sep 23, 2016, at 9:46 AM, Vyacheslav Vakhlyuev <[email protected]> > wrote: > > I added a new commit to the forked copy of Fortress core. > It adds tests for new GroupMgr methods, modified SSD and DSD checks for > Group-type entities and some minor changes (HttpIds, renamed/overloaded > AccessMgr.createSession( Group ) method etc.) > Here's a link to the commit: https://github.com/vvakhlyuev-work/directory- > fortress-core/commit/252e6116933c7d37d53159c304fdb1e309a97aa1 > > Also, there's a minor change to EnMasse related to method rename in Core > project: https://github.com/vvakhlyuev-work/directory- > fortress-enmasse/commit/3eb9754290ced641b9a303b356cd6031165a02cc > Not sure but suspect the contextId (tenant) isn’t being set before the call to deassign here. This is a tricky area of the SDK. For now let’s flag it with a ‘todo’ and I’ll do some multitenancy testing later to make sure it’s correct. public void deleteRole( Role role ) throws SecurityException { … Role outRole = roleP.read( role ); outRole.setContextId( role.getContextId() ); // deassign all groups assigned to this role first (because of schema's configGroup class constraints) List<Group> groups = groupP.roleGroups( outRole ); for ( Group group : groups ) { groupP.deassign( group, outRole.getDn() ); } I should have brought this one up sooner. For each role assigned to group, we have to perform a read, to pull back its constraints. This works but not efficient. It’s OK for now and not a showstopper. Eventually we’ll want to store these constraints on the group object itself like done with the user object. That way we can perform the method will one read. Session createSession( Group group ) throws SecurityException { // Create the impl session without authentication of password. Session session = createSessionTrusted( group ); // Did the caller pass in a set of roles for selective activation? if ( CollectionUtils.isNotEmpty( group.getMembers() ) ) { // Process selective activation of user's RBAC roles into session: List<String> availableRoles = session.getGroup().getMembers(); availableRoles.retainAll( group.getMembers() ); } // Fill aux field 'roles' with Role entities fillRoles( session.getGroup() ); // Check role temporal constraints + activate roles: VUtil.getInstance().validateConstraints( session, VUtil.ConstraintType.ROLE, true ); return session; } > > On Sep 23, 2016, at 9:46 AM, Vyacheslav Vakhlyuev <[email protected]> > wrote: > > Overall, I think these commits finalize the changes I need to do for > FC-144. > Could you please review the changes located in > "FC-144/assign-roles-for-groups" branches in my forked copies? If you find > everything is OK, I will open a pull-request to merge these changes. > The code looks really good. The core's unit tests pass. I haven’t run through enmasse / multitenancy yet. In the meantime - go for it. > > On Sep 23, 2016, at 9:46 AM, Vyacheslav Vakhlyuev <[email protected]> > wrote: > > One more thing regarding Realm project. I modified versions of Core and > EnMasse to "1.0.2-SNAPSHOT" in their pom.xml files. I also modified EnMasse > pom to require Realm "1.0.2-SNAPSHOT". > However, I didn't commit any changes to Realm project, so an additional > change will be needed there. OK Shawn
