Alon Bar-Lev has posted comments on this change. Change subject: aaa: Stopping to use proxies ......................................................................
Patch Set 4: (9 comments) http://gerrit.ovirt.org/#/c/26602/4/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthzUtils.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthzUtils.java: Line 33: ) Line 34: )).<ExtMap> get(Authz.InvokeKeys.PRINCIPAL_RECORD)); Line 35: } Line 36: Line 37: public static DirectoryUser findUserById(final ExtensionProxy extension, final String id) { I think that almost every case you have more than one id... it is used for sync, no? Line 38: List<DirectoryUser> users = findUsers(extension, Arrays.asList(id)); Line 39: if (users.isEmpty()) { Line 40: return null; Line 41: } Line 51: ids Line 52: )); Line 53: } Line 54: Line 55: public static DirectoryGroup findGroupById(final ExtensionProxy extension, final String id) { same Line 56: return populateGroups( Line 57: extension, Line 58: Authz.InvokeCommands.QUERY_GROUPS_BY_IDS_OPEN, Line 59: new ExtMap().mput( http://gerrit.ovirt.org/#/c/26602/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java: Line 48: if (dbGroup != null) { Line 49: DirectoryProxy directory = Line 50: AuthenticationProfileRepository.getInstance().getDirectory(dbGroup.getDomain()); Line 51: if (directory != null) { Line 52: mGroup = AuthzUtils.findGroupById(directory.getExtension(), dbGroup.getExternalId()); why do we need this? why can't we fetch it out of user that is logged or user that was searched? Line 53: } Line 54: } Line 55: } Line 56: return mGroup; http://gerrit.ovirt.org/#/c/26602/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java: Line 279: AuditLogDirector.log(logable, AuditLogType.USER_VDC_LOGIN_FAILED); Line 280: } Line 281: Line 282: private boolean isPasswordAuth() { Line 283: return (authnExtension.getContext().<Long> get(Authn.ContextKeys.CAPABILITIES).longValue() & are you sure you cannot do <long> ? Line 284: Authn.Capabilities.AUTHENTICATE_PASSWORD) != 0; Line 285: } Line 286: Line 287: private boolean authenticate(String user, String password) { Line 284: Authn.Capabilities.AUTHENTICATE_PASSWORD) != 0; Line 285: } Line 286: Line 287: private boolean authenticate(String user, String password) { Line 288: boolean result = true; you must assume fail and set true only when you are sure there is success. Line 289: ExtMap outputMap = authnExtension.invoke(new ExtMap().mput( Line 290: Base.InvokeKeys.COMMAND, Line 291: Authn.InvokeCommands.AUTHENTICATE_CREDENTIALS Line 292: ).mput( Line 303: "Can't login user \"{0}\" with authentication profile \"{1}\" because the authentication failed.", Line 304: user, Line 305: getParameters().getProfileName()); Line 306: Line 307: AuditLogType auditLogType = auditLogMap.get(authResult); again, if that returns null it means that a new code was added and we should present failure to the user. Line 308: // if found matching audit log type, and it's not general login failure audit log (which will be logged Line 309: // anyway due to CommandBase.log) Line 310: if (auditLogType != null && auditLogType != AuditLogType.USER_VDC_LOGIN_FAILED) { Line 311: logEventForUser(user, auditLogType); Line 306: Line 307: AuditLogType auditLogType = auditLogMap.get(authResult); Line 308: // if found matching audit log type, and it's not general login failure audit log (which will be logged Line 309: // anyway due to CommandBase.log) Line 310: if (auditLogType != null && auditLogType != AuditLogType.USER_VDC_LOGIN_FAILED) { once again... checking for AditLogType and not authResult is very strange... I am unsure why this is correct... and what is USER_VDC_** but I leave it for you now... we will revisit all invoke executions once we finish a full pass. Line 311: logEventForUser(user, auditLogType); Line 312: } Line 313: Line 314: if (authResult == Authn.AuthResult.CREDENTIALS_EXPIRED) { http://gerrit.ovirt.org/#/c/26602/4/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthenticator.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthenticator.java: Line 98: passwordChangeUrlPerDomain.put(pairParts[0], decodedMsgOrUrl); Line 99: } else { Line 100: passwordChangeMsgPerDomain.put(pairParts[0], decodedMsgOrUrl); Line 101: } Line 102: } catch (UnsupportedEncodingException e) { it should not throw this, Charset.forName() already throws runtime exception. Line 103: throw new RuntimeException(e); Line 104: } Line 105: } Line 106: } http://gerrit.ovirt.org/#/c/26602/4/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapBrokerCommandBase.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapBrokerCommandBase.java: Line 47: Authn.AuthResult.CREDENTIALS_EXPIRED); Line 48: resultsMap.put(AuthenticationResult.USER_ACCOUNT_DISABLED_OR_LOCKED, Line 49: Authn.AuthResult.ACCOUNT_LOCKED); Line 50: resultsMap.put(AuthenticationResult.WRONG_REALM, Line 51: Authn.AuthResult.CREDENTIALS_INCORRECT); you can really drop the AuthenticationResult Line 52: } Line 53: Line 54: private Map<String, LdapGroup> globalGroupsDict = new HashMap<>(); Line 55: -- To view, visit http://gerrit.ovirt.org/26602 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I916012eab61a96bdb0f366d9dc8462325d7f726f Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
