Yair Zaslavsky has posted comments on this change. Change subject: aaa: Stop using proxies ......................................................................
Patch Set 2: (8 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 Done 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? next phaseof this patch I will deal with that. I still want AuthenticationProfile to be able to work with Authenticator and Directory. 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: Done 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. Well, see public AuditLogType getAuditLogTypeValue() { ) in case of a failure, we are audit logging at least the defaut failure event, as retrieved by the above method. In addition, if we have more info on why the failure occured, we will audit log another 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. not sure i understood, this has to do with audit log, what is wrong with that? Line 309: logEventForUser(user, auditLogType); Line 310: } Line 311: Line 312: if (authResult == Authn.AuthResult.CREDENTIALS_EXPIRED) { Line 309: logEventForUser(user, auditLogType); Line 310: } Line 311: Line 312: if (authResult == Authn.AuthResult.CREDENTIALS_EXPIRED) { Line 313: boolean addedUserPasswordExpiredCDA = false; > remove^? see one comment under. 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? I don't think so - can't we get both USER_MESSAGE and a URL? 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? see one comment above :) 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: 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
