Yair Zaslavsky has posted comments on this change. Change subject: aaa: Introducing usage of Acct ......................................................................
Patch Set 17: (7 comments) http://gerrit.ovirt.org/#/c/27070/17/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 167: authnExtension = profile.getAuthn(); Line 168: authRecord = (ExtMap) getParameters().getAuthRecord(); Line 169: int reportReason = Acct.ReportReason.PRINCIPAL_LOGIN_CREDENTIALS; Line 170: if (getParameters().getAuthType() != null) { Line 171: if (AuthType.NEGOTIATION == getParameters().getAuthType()) { > shouldn't this be equals instead of ==? no. if you happen to encounter more places where we perform equals on enum, please let me know (or submit patches :) ) i thought like you in the past it should be equals, but not, it should be == - as enum is a fixed set of literals. == is considered to be (a bit) faster. Line 172: reportReason = Acct.ReportReason.PRINCIPAL_LOGIN_NEGOTIATE; Line 173: } Line 174: } Line 175: String loginName = null; Line 238: Acct.ReportReason.PRINCIPAL_INVALID, Line 239: loginName, Line 240: authRecord, Line 241: null, Line 242: "Principal record is invalid" > please add principal name to message well, this is an invalid principal record. I will the user name (loginName) - see your comment after. Line 243: ); Line 244: Line 245: return false; Line 246: } Line 275: loginName, Line 276: authRecord, Line 277: principalRecord, Line 278: Line 279: "The user %1$s is not authorized to perform login", > this is principal not user, if you want add user which is loginName thanks. i will change to loginName at the last argument. Line 280: principalRecord.<String> get(Authz.PrincipalRecord.NAME) Line 281: ); Line 282: addCanDoActionMessage(VdcBllMessages.USER_NOT_AUTHORIZED_TO_PERFORM_ACTION); Line 283: return false; Line 294: reportReason, Line 295: loginName, Line 296: authRecord, Line 297: principalRecord, Line 298: "The user %1$s has performed successful authentication", > "User ... logged in" is enough :))) Done Line 299: principalRecord.<String> get(Authz.PrincipalRecord.NAME) Line 300: ); Line 301: return true; Line 302: } Line 295: loginName, Line 296: authRecord, Line 297: principalRecord, Line 298: "The user %1$s has performed successful authentication", Line 299: principalRecord.<String> get(Authz.PrincipalRecord.NAME) > same ok. Line 300: ); Line 301: return true; Line 302: } Line 303: return false; http://gerrit.ovirt.org/#/c/27070/17/backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/aaa/Acct.java File backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/aaa/Acct.java: Line 64: public static final int STARTUP = 0; Line 65: /** Application shutdown. */ Line 66: public static final int SHUTDOWN = 1; Line 67: /** Line 68: * User is invalid. > please add: I have removed user_invalid from the API. Line 69: */ Line 70: public static final int USER_INVALID = 2; Line 71: /** Line 72: * Principal record was not found. Line 68: * User is invalid. Line 69: */ Line 70: public static final int USER_INVALID = 2; Line 71: /** Line 72: * Principal record was not found. > please add: Hmm, the only place I use it in code is to mark is to report i could not find a principal record from fetchPrincipalRecord. Maybe change to PRINCIPAL_WAS_NOT_FOUND? Line 73: */ Line 74: public static final int PRINCIPAL_INVALID = 3; Line 75: /** Line 76: * Login failed by any reason but locked. -- To view, visit http://gerrit.ovirt.org/27070 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ief13d233d11b7ab32b328735b4f58ec7cffff567 Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Ravi Nori <[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
