Yair Zaslavsky has posted comments on this change. Change subject: aaa: Fix Audit log and can do action msgs handling in ldap broker. ......................................................................
Patch Set 2: (5 comments) http://gerrit.ovirt.org/#/c/25529/2/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/PasswordAuthenticator.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/PasswordAuthenticator.java: Line 16: * the name of user being authenticated Line 17: * @param password Line 18: * @return true if authenticated Line 19: */ Line 20: public abstract boolean authenticate(String name, String password); > why do you need boolean? if exception it is a failure. I'll fix that. http://gerrit.ovirt.org/#/c/25529/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 202: } Line 203: Line 204: VdcBllMessages canDoActionMsg = vdcBllMessagesMap.get(ex.getError()); Line 205: Line 206: String passwordChangeMsg = passwordChangeMsgPerDomain.get(getParameters().getProfileName()); > this should be moved to provider as well Just to be sure - What should be? moved the whole password change msg/url? Ifo s, why should it be moved to the provider? we may encounter this issue from various AAA providers. Line 207: getReturnValue().setSucceeded(false); Line 208: if (canDoActionMsg == VdcBllMessages.USER_PASSWORD_EXPIRED && passwordChangeMsg != null) { Line 209: if (passwordChangeMsg.indexOf("http") == 0 || passwordChangeMsg.indexOf("https") == 0) { Line 210: addCanDoActionMessage(VdcBllMessages.USER_PASSWORD_EXPIRED_CHANGE_URL_PROVIDED); Line 205: Line 206: String passwordChangeMsg = passwordChangeMsgPerDomain.get(getParameters().getProfileName()); Line 207: getReturnValue().setSucceeded(false); Line 208: if (canDoActionMsg == VdcBllMessages.USER_PASSWORD_EXPIRED && passwordChangeMsg != null) { Line 209: if (passwordChangeMsg.indexOf("http") == 0 || passwordChangeMsg.indexOf("https") == 0) { > why do you check http or https? This code was moved here from ProvisionalAuthenticationResult that was removed in this patch (so besides moving the code the logic was not invented in this patch). The reason I check for http or https, is because there are two types of canDoAction messages - one for message containing URL, and one for plain message. Here is the relevant part from AppErrors.properties USER_PASSWORD_EXPIRED_CHANGE_URL_PROVIDED=Cannot Login. User Password has expired. Use the following URL to change the password: ${URL} USER_PASSWORD_EXPIRED_CHANGE_MSG_PROVIDED=Cannot login. User Password has expired. Detailed message: ${MSG} IMHO, If you think the behavior needs to be handled, I think we should solve this outside the scope of this Line 210: addCanDoActionMessage(VdcBllMessages.USER_PASSWORD_EXPIRED_CHANGE_URL_PROVIDED); Line 211: getReturnValue().getCanDoActionMessages().add(String.format("$URL %1$s", passwordChangeMsg)); Line 212: } else { Line 213: addCanDoActionMessage(VdcBllMessages.USER_PASSWORD_EXPIRED_CHANGE_MSG_PROVIDED); http://gerrit.ovirt.org/#/c/25529/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LdapBrokerCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LdapBrokerCommandBase.java: Line 57: new AAAExtensionException(ExtensionError.CREDENTIALS_EXPIRED, "")); Line 58: authResultToExceptionMap.put(AuthenticationResult.USER_ACCOUNT_DISABLED_OR_LOCKED, Line 59: new AAAExtensionException(ExtensionError.LOCKED_OR_DISABLED_ACCOUNT, "")); Line 60: authResultToExceptionMap.put(AuthenticationResult.WRONG_REALM, Line 61: new AAAExtensionException(ExtensionError.INCORRET_CREDENTIALS, "")); > why can't you just map the code? Meaning? introducing it to AuthenticationResult as another field? Line 62: } Line 63: Line 64: @Override Line 65: protected String getPROTOCOL() { http://gerrit.ovirt.org/#/c/25529/2/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/kerberos/AuthenticationResult.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/kerberos/AuthenticationResult.java: Line 1 > this should be moved to the built-in provider module, right? Indeed, in next patches/next patch to come it will be moved ot built-in-provider. -- To view, visit http://gerrit.ovirt.org/25529 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8b0b024d27a92f620bb60e4689264bc6b3c3eda1 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: Martin Peřina <[email protected]> Gerrit-Reviewer: Oved Ourfali <[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
