Yair Zaslavsky has posted comments on this change.

Change subject: aaa: Fix sync
......................................................................


Patch Set 5:

(6 comments)

http://gerrit.ovirt.org/#/c/29706/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SyncUsers.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SyncUsers.java:

Line 35:                 authzToNamespaceToUserIds.put(dbUser.getDomain(), new 
HashMap<String, Set<String>>());
Line 36:             }
Line 37:             MultiValueMapUtils.addToMapOfSets(dbUser.getNamespace(), 
dbUser.getExternalId(), authzToNamespaceToUserIds.get(dbUser.getDomain()));
Line 38:             dbUsersMap.put(new DirectoryEntryKey(dbUser.getDomain(), 
dbUser.getExternalId()), dbUser);
Line 39:         }
> please split into another private function to ease reading:
not just authz, i have to provide namespace as well.
Line 40: 
Line 41:         for (String authz : authzToNamespaceToUserIds.keySet()) {
Line 42:             try {
Line 43:                 ExtensionProxy authzExtension = 
EngineExtensionsManager.getInstance().getExtensionByName(authz);


Line 43:                 ExtensionProxy authzExtension = 
EngineExtensionsManager.getInstance().getExtensionByName(authz);
Line 44:                 for (Entry<String, Set<String>> userIdsPerNamespace : 
authzToNamespaceToUserIds.get(authz).entrySet()) {
Line 45:                     Set<String> userIds = 
userIdsPerNamespace.getValue();
Line 46:                     for (DirectoryUser directoryUser : 
DirectoryUtils.fetchUsersByIdsRecursively(authzExtension, 
userIdsPerNamespace.getKey(), new ArrayList<String>(userIds))) {
Line 47:                         DirectoryUtils.flatGroups(directoryUser);
> why do you flat groups here? flat groups should be the last thing you do, a
flat groups should be done before setting the group ids.
Line 48:                         DbUser dbUser = new DbUser(directoryUser);
Line 49:                         
dbUser.setGroupIds(DirectoryUtils.getGroupIdsFromUser(directoryUser));
Line 50:                         activeUsers.put(new 
DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), 
dbUser);
Line 51:                     }


Line 44:                 for (Entry<String, Set<String>> userIdsPerNamespace : 
authzToNamespaceToUserIds.get(authz).entrySet()) {
Line 45:                     Set<String> userIds = 
userIdsPerNamespace.getValue();
Line 46:                     for (DirectoryUser directoryUser : 
DirectoryUtils.fetchUsersByIdsRecursively(authzExtension, 
userIdsPerNamespace.getKey(), new ArrayList<String>(userIds))) {
Line 47:                         DirectoryUtils.flatGroups(directoryUser);
Line 48:                         DbUser dbUser = new DbUser(directoryUser);
> why do you convert to dbUser here? can't we gather the DirectoryUser and pe
what's the 2nd function for?
Line 49:                         
dbUser.setGroupIds(DirectoryUtils.getGroupIdsFromUser(directoryUser));
Line 50:                         activeUsers.put(new 
DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), 
dbUser);
Line 51:                     }
Line 52:                 }


Line 50:                         activeUsers.put(new 
DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), 
dbUser);
Line 51:                     }
Line 52:                 }
Line 53:             } catch (ConfigurationException ex) {
Line 54:                 log.error(String.format("The authz extension for %1%s 
has not been found. Users belonging to this extension will be marked as 
inactive",
> please print the ex.getMessage() as well, as the error can be different tha
Done
Line 55:                         authz));
Line 56:             }
Line 57:         }
Line 58: 


Line 74:                 updatedUser.setId(dbUser.getId());
Line 75:                 if (!dbUser.isActive()) {
Line 76:                     updated = true;
Line 77:                     dbUser.setActive(true);
Line 78:                     log.info(String.format("The inactive user %1$s 
from authz extension %2$s was found, and will be marked as active",
> how can it be? we do not search for inactive at all
what if extension was removed and re-added ? (for example, the jboss module was 
removed and then re-placed).
when the extension was removed - all the relevant users will become inactive.
when returned - all relevant users will become active, no?
Line 79:                             dbUser.getLoginName(),
Line 80:                             dbUser.getDomain()));
Line 81:                 }
Line 82:                 if (!dbUser.equals(activeUser)) {


http://gerrit.ovirt.org/#/c/29706/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbUser.java
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbUser.java:

Line 250: 
Line 251:     @Override
Line 252:     public int hashCode() {
Line 253:         final int prime = 31;
Line 254:         int result = 1;
This proved to be wrong due to the following reasons:
a. in the code I'm not using any HashSet or HashMap that has DbUser as a key.
b. i should compare all fields when checking equality of db user at sync.
Line 255:         result = prime * result + ((externalId == null) ? 0 : 
externalId.hashCode());
Line 256:         result = prime * result + ((domain == null) ? 0 : 
domain.hashCode());
Line 257:         return result;
Line 258:     }


-- 
To view, visit http://gerrit.ovirt.org/29706
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a66a7c235f16ff26cac3a779de688c73d67f4a2
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to