Alon Bar-Lev has posted comments on this change. Change subject: aaa: Stop using proxies ......................................................................
Patch Set 2: (9 comments) http://gerrit.ovirt.org/#/c/26602/2/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationFilter.java: Line 66: if (profile != null) { Line 67: if ((((AuthenticatorProxy) profile.getAuthenticator()).getExtension() Line 68: .getContext() Line 69: .<Long> get(Authn.ContextKeys.CAPABILITIES) Line 70: .longValue() & Authn.Capabilities.AUTHENTICATE_NEGOTIATE) == Authn.Capabilities.AUTHENTICATE_NEGOTIATE) { bitwise and please Line 71: profiles.add(0, profile); Line 72: } Line 73: Line 74: } http://gerrit.ovirt.org/#/c/26602/2/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticatorProxy.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticatorProxy.java: Line 7 Line 8 Line 9 Line 10 Line 11 why don't you make Autheticator inherit from extension proxy? http://gerrit.ovirt.org/#/c/26602/2/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 278: } Line 279: Line 280: private boolean isPasswordAuth() { Line 281: return (extension.getContext().<Long> get(Authn.ContextKeys.CAPABILITIES).longValue() & Line 282: Authn.Capabilities.AUTHENTICATE_PASSWORD) == Authn.Capabilities.AUTHENTICATE_PASSWORD; flags should be checked as: (xxx & flag) != 0 Line 283: } Line 284: Line 285: private boolean authenticate(String user, String password) { Line 286: boolean result = true; Line 301: "Can't login user \"{0}\" with authentication profile \"{1}\" because the authentication failed.", Line 302: user, Line 303: getParameters().getProfileName()); Line 304: Line 305: AuditLogType auditLogType = auditLogMap.get(authResult); if missing in map you should default to the FAILED entry. Line 306: // if found matching audit log type, and it's not general login failure audit log (which will be logged Line 307: // anyway due to CommandBase.log) Line 308: if (auditLogType != null && auditLogType != AuditLogType.USER_VDC_LOGIN_FAILED) { Line 309: logEventForUser(user, auditLogType); Line 304: Line 305: AuditLogType auditLogType = auditLogMap.get(authResult); Line 306: // if found matching audit log type, and it's not general login failure audit log (which will be logged Line 307: // anyway due to CommandBase.log) Line 308: if (auditLogType != null && auditLogType != AuditLogType.USER_VDC_LOGIN_FAILED) { please check result directly and not the conversion. Line 309: logEventForUser(user, auditLogType); Line 310: } Line 311: Line 312: if (authResult == Authn.AuthResult.CREDENTIALS_EXPIRED) { Line 308: if (auditLogType != null && auditLogType != AuditLogType.USER_VDC_LOGIN_FAILED) { Line 309: logEventForUser(user, auditLogType); Line 310: } Line 311: Line 312: if (authResult == Authn.AuthResult.CREDENTIALS_EXPIRED) { also, I suggest switch statement. switch(result) { default: // failure success: /// password-expired: /// } Line 313: boolean addedUserPasswordExpiredCDA = false; Line 314: if (outputMap.<String> get(Authn.InvokeKeys.CREDENTIALS_CHANGE_URL) != null) { Line 315: addCanDoActionMessage(VdcBllMessages.USER_PASSWORD_EXPIRED_CHANGE_URL_PROVIDED); Line 316: getReturnValue().getCanDoActionMessages().add(String.format("$URL %1$s", Line 309: logEventForUser(user, auditLogType); Line 310: } Line 311: Line 312: if (authResult == Authn.AuthResult.CREDENTIALS_EXPIRED) { Line 313: boolean addedUserPasswordExpiredCDA = false; remove^? Line 314: if (outputMap.<String> get(Authn.InvokeKeys.CREDENTIALS_CHANGE_URL) != null) { Line 315: addCanDoActionMessage(VdcBllMessages.USER_PASSWORD_EXPIRED_CHANGE_URL_PROVIDED); Line 316: getReturnValue().getCanDoActionMessages().add(String.format("$URL %1$s", Line 317: outputMap.<String> get(Authn.InvokeKeys.CREDENTIALS_CHANGE_URL))); Line 316: getReturnValue().getCanDoActionMessages().add(String.format("$URL %1$s", Line 317: outputMap.<String> get(Authn.InvokeKeys.CREDENTIALS_CHANGE_URL))); Line 318: addedUserPasswordExpiredCDA = true; Line 319: } Line 320: if (outputMap.<String> get(Authn.InvokeKeys.USER_MESSAGE) != null) { else if? Line 321: addCanDoActionMessage(VdcBllMessages.USER_PASSWORD_EXPIRED_CHANGE_MSG_PROVIDED); Line 322: getReturnValue().getCanDoActionMessages().add(String.format("$MSG %1$s", Line 323: outputMap.<String> get(Authn.InvokeKeys.USER_MESSAGE))); Line 324: addedUserPasswordExpiredCDA = true; Line 322: getReturnValue().getCanDoActionMessages().add(String.format("$MSG %1$s", Line 323: outputMap.<String> get(Authn.InvokeKeys.USER_MESSAGE))); Line 324: addedUserPasswordExpiredCDA = true; Line 325: } Line 326: if (!addedUserPasswordExpiredCDA) { else? Line 327: addCanDoActionMessage(VdcBllMessages.USER_PASSWORD_EXPIRED); Line 328: } Line 329: } else { Line 330: addCanDoActionMessage(vdcBllMessagesMap.get(authResult)); -- 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: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[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
