Alon Bar-Lev has posted comments on this change. Change subject: AAA: Introduce usage of Acct ......................................................................
Patch Set 3: (13 comments) http://gerrit.ovirt.org/#/c/27070/3//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-04-25 11:21:03 +0300 Line 4: Commit: Yair Zaslavsky <yzasl...@redhat.com> Line 5: CommitDate: 2014-04-25 17:49:50 +0300 Line 6: Line 7: AAA: Introduce usage of Acct aaa: Line 8: Line 9: Introducing usage of accounting Line 10: Line 11: Topic: AAA http://gerrit.ovirt.org/#/c/27070/3/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 149 Line 150 Line 151 Line 152 Line 153 you should put the auth record within the session as well. Line 153: session.setAttribute(AUTHENTICATED_ATTR, true); Line 154: session.setAttribute(NAME_ATTR, name); Line 155: session.removeAttribute(STACK_ATTR); Line 156: req = new AuthenticatedRequestWrapper(req, name); Line 157: AcctUtils.reportAuthRecord(Acct.ReportReason.PRINCIPAL_LOGIN_NEGOTIATE, authRecord); indent? Line 158: chain.doFilter(req, rsp); Line 159: return; Line 160: Line 161: case Authn.AuthResult.NEGOTIATION_UNAUTHORIZED: Line 160: Line 161: case Authn.AuthResult.NEGOTIATION_UNAUTHORIZED: Line 162: moveToNextProfile(session, stack); Line 163: AcctUtils.reportAuthRecord(Acct.ReportReason.PRINCIPAL_LOGIN_FAILED, authRecord); Line 164: you should not report in this case as it is normal that extension returns unauthorized to go into the next extension. only when all extensions are used you should report. this should be in the login filter not in this one. Line 165: break; Line 166: Line 167: default: Line 168: moveToNextProfile(session, stack); Line 165: break; Line 166: Line 167: default: Line 168: moveToNextProfile(session, stack); Line 169: AcctUtils.reportAuthRecord(Acct.ReportReason.PRINCIPAL_LOGIN_FAILED, authRecord); this is not login failure... it should not go into accounting, it is application failure. Line 170: log.error(String.format("An error has occurred during negotiation. Error code is %1$s", Line 171: output.<Integer> get(Authn.InvokeKeys.RESULT))); Line 172: Line 173: Line 169: AcctUtils.reportAuthRecord(Acct.ReportReason.PRINCIPAL_LOGIN_FAILED, authRecord); Line 170: log.error(String.format("An error has occurred during negotiation. Error code is %1$s", Line 171: output.<Integer> get(Authn.InvokeKeys.RESULT))); Line 172: Line 173: remove? Line 174: } Line 175: } Line 176: Line 177: // If we are here then there are no more authenticators to try so we need to invalidate the session and reject http://gerrit.ovirt.org/#/c/27070/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java: Line 251: @Override Line 252: public void run() { Line 253: AcctUtils.reportReason(Acct.ReportReason.SHUTDOWN); Line 254: } Line 255: })); are you sure all classes are alive at this point? I thought of using something like ServletContextListener[1] [1] http://docs.oracle.com/cd/E13222_01/wls/docs61/webapp/app_events.html Line 256: Line 257: AcctUtils.reportReason(Acct.ReportReason.STARTUP); Line 258: } Line 259: http://gerrit.ovirt.org/#/c/27070/3/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 265: getActionType().getActionGroup(), Line 266: MultiLevelAdministrationHandler.BOTTOM_OBJECT_ID, Line 267: VdcObjectType.Bottom, Line 268: true)) { Line 269: AcctUtils.reportAuthRecord(Acct.ReportReason.PRINCIPAL_LOGIN_NO_PERMISSION, authRecord); here you should also pass principal record as you should have it. Line 270: addCanDoActionMessage(VdcBllMessages.USER_NOT_AUTHORIZED_TO_PERFORM_ACTION); Line 271: return false; Line 272: } Line 273: Line 359: if (reportReason == null) { Line 360: reportReason = Acct.ReportReason.PRINCIPAL_LOGIN_FAILED; Line 361: } Line 362: Line 363: AcctUtils.reportAuthRecord(reportReason, result); you should also get principal from output map, if missing use the user of input map to report the event. Line 364: if (authResult != Authn.AuthResult.SUCCESS) { Line 365: log.infoFormat( Line 366: "Can't login user \"{0}\" with authentication profile \"{1}\" because the authentication failed.", Line 367: user, http://gerrit.ovirt.org/#/c/27070/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/session/SessionDataContainer.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/session/SessionDataContainer.java: Line 147 Line 148 Line 149 Line 150 Line 151 so why not just use the removeSession(sessionid) method and avoid duplicate code? if ( (hardLimit != null && hardLimit.before(now)) || (softLimit != null && softLimit.beofre(now)) ) { removeSession(entry.getKey()); } Line 30: private static final String PASSWORD_PARAMETER_NAME = "password"; Line 31: private static final String HARD_LIMIT_PARAMETER_NAME = "hard_limit"; Line 32: private static final String SOFT_LIMIT_PARAMETER_NAME = "soft_limit"; Line 33: Line 34: private static final String AUTH_RECORD_PARAMETER_NAME = null; why null? why not as the above have static name? also please add principal record entry. Line 35: Line 36: private static SessionDataContainer dataProviderInstance = new SessionDataContainer(); Line 37: Line 38: private SessionDataContainer() { http://gerrit.ovirt.org/#/c/27070/3/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthn.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthn.java: Line 41: output.put(Authn.InvokeKeys.RESULT, Authn.AuthResult.CREDENTIALS_INVALID); Line 42: } else { Line 43: output.put(Authn.InvokeKeys.RESULT, Authn.AuthResult.SUCCESS); Line 44: } Line 45: output.put(Authn.InvokeKeys.AUTH_RECORD, auth record should be set only if success. principal should be set also if password is bad. Line 46: new ExtMap().mput( Line 47: Authn.AuthRecord.PRINCIPAL, Line 48: adminUser Line 49: ) http://gerrit.ovirt.org/#/c/27070/3/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapAuthenticateUserCommand.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapAuthenticateUserCommand.java: Line 32: new ExtMap().mput( Line 33: Authn.AuthRecord.PRINCIPAL, Line 34: loginName Line 35: )); Line 36: auth record should be set only if success. principal should be set also if password is bad. Line 37: if (!loginName.contains("@")) { Line 38: loginName = loginName + "@" + getDomain(); Line 39: } Line 40: -- 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: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
