Alon Bar-Lev has posted comments on this change. Change subject: aaa: Introduce usage of Acct ......................................................................
Patch Set 4: (15 comments) > Do you still think we need an interceptor here? again... if we do not relay on the j2ee session container, and only relay on our expiration mechanism in both application and restapi then what you implemented here is enough. what I want you is to be sure that: 1. we always get login acct message (obvious) 2. we *ALWAYS* get either logout or session expired message for each message of (1). http://gerrit.ovirt.org/#/c/27070/4/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AcctUtils.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AcctUtils.java: Line 28: ); Line 29: report(input); Line 30: } Line 31: Line 32: public static void reportRecords(int reportReason, ExtMap authRecord, ExtMap principalRecord) { user and principal should be also here, as if login fail and extension cannot map it into principal or if you have principal but fail login and not have auth record, we should log the user. Line 33: ExtMap input = new ExtMap(); Line 34: input.put(Acct.InvokeKeys.REASON, reportReason); Line 35: if (authRecord != null) { Line 36: input.put(Acct.InvokeKeys.PRINCIPAL_RECORD, Line 39: authRecord Line 40: )); Line 41: } Line 42: if (principalRecord != null) { Line 43: input.put(Acct.InvokeKeys.PRINCIPAL_RECORD, you can just put these into the map, null is as if you put nothing, so you can drop the conditionals. Line 44: new ExtMap().mput( Line 45: Acct.PrincipalRecord.PRINCIPAL_RECORD, Line 46: authRecord Line 47: )); http://gerrit.ovirt.org/#/c/27070/4/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 51: * In order to support several alternative authenticators we store their names in a stack inside the HTTP session, Line 52: * this is the key for that stack. Line 53: */ Line 54: private static final String STACK_ATTR = AuthenticationFilter.class.getName() + ".stack"; Line 55: - Line 56: private static final String AUTH_RECORD_ATTR = "auth_record"; Line 57: Line 58: @Override Line 59: public void init(FilterConfig filterConfig) throws ServletException { Line 156: session.setAttribute(NAME_ATTR, name); Line 157: session.setAttribute(AUTH_RECORD_ATTR, authRecord); Line 158: session.removeAttribute(STACK_ATTR); Line 159: req = new AuthenticatedRequestWrapper(req, name); Line 160: AcctUtils.reportRecords(Acct.ReportReason.PRINCIPAL_LOGIN_NEGOTIATE, authRecord, null); hmmm... this is strange... ok, we succeeded in authn, but have not yet authz, so we cannot report it here. we should report only after we have all information... where do we do authz in this case? Line 161: chain.doFilter(req, rsp); Line 162: return; Line 163: Line 164: case Authn.AuthResult.NEGOTIATION_UNAUTHORIZED: Line 166: break; Line 167: Line 168: default: Line 169: moveToNextProfile(session, stack); Line 170: AcctUtils.reportRecords(Acct.ReportReason.PRINCIPAL_LOGIN_FAILED, authRecord, null); you should not report failure at negotiation. Line 171: log.error(String.format("An error has occurred during negotiation. Error code is %1$s", Line 172: output.<Integer> get(Authn.InvokeKeys.RESULT))); Line 173: Line 174: } http://gerrit.ovirt.org/#/c/27070/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AutomaticLoginFilter.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AutomaticLoginFilter.java: Line 170: AcctUtils.report(new ExtMap().mput( Line 171: Acct.InvokeKeys.REASON, Line 172: reportReason Line 173: ).mput( Line 174: Acct.InvokeKeys.PRINCIPAL_RECORD, we discussed that, you should never construct your own records, only use what you have from authn/authz. Line 175: new ExtMap().mput( Line 176: Acct.PrincipalRecord.USER, Line 177: userName Line 178: ) Line 172: reportReason Line 173: ).mput( Line 174: Acct.InvokeKeys.PRINCIPAL_RECORD, Line 175: new ExtMap().mput( Line 176: Acct.PrincipalRecord.USER, - Line 177: userName Line 178: ) Line 179: ) Line 180: ); Line 177: userName Line 178: ) Line 179: ) Line 180: ); Line 181: - Line 182: } http://gerrit.ovirt.org/#/c/27070/4/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 245: 1, quotaCacheIntervalInMinutes, TimeUnit.MINUTES); Line 246: //initializes attestation Line 247: initAttestation(); Line 248: Line 249: Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() { I thought we are going to context pre-destroy. Line 250: Line 251: @Override Line 252: public void run() { Line 253: AcctUtils.reportReason(Acct.ReportReason.SHUTDOWN); http://gerrit.ovirt.org/#/c/27070/4/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 240: return false; Line 241: } Line 242: Line 243: DirectoryUser directoryUser = AuthzUtils.mapPrincipalRecord(profile.getAuthz(), principalRecord); Line 244: AcctUtils.reportRecords(Acct.ReportReason.PRINCIPAL_LOGIN_CREDENTIALS, authRecord, null); hmmm.... we succeeded authn but failed authz, this means credentials is ok, but PRINCIPAL_LOGIN_NO_PERMISSION. Line 245: Line 246: Line 247: // Check that the user exists in the database, if it doesn't exist then we need to add it now: Line 248: dbUser = Line 355: )); Line 356: Line 357: String principal = Line 358: outputMap.<String> get(Authn.InvokeKeys.PRINCIPAL) != null ? outputMap.<String> get(Authn.InvokeKeys.PRINCIPAL) Line 359: : getParameters().getLoginName(); please use the defaults... we are not using the primitive java map. String principal = outputMap.<String> get(Authn.InvokeKeys.PRINCIPAL, getParameters().getLoginName()) however, I do not like you use the term of principal for the login name... the principal is only what returned by extension, the user specify 'user'. Line 360: SessionDataContainer.getInstance().setPrincipal(outputMap.<String> get(Authn.InvokeKeys.PRINCIPAL)); Line 361: ExtMap result = outputMap.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); Line 362: int authResult = outputMap.<Integer>get(Authn.InvokeKeys.RESULT); Line 363: Integer reportReason = authResultToReportReasonMap.get(authResult); Line 357: String principal = Line 358: outputMap.<String> get(Authn.InvokeKeys.PRINCIPAL) != null ? outputMap.<String> get(Authn.InvokeKeys.PRINCIPAL) Line 359: : getParameters().getLoginName(); Line 360: SessionDataContainer.getInstance().setPrincipal(outputMap.<String> get(Authn.InvokeKeys.PRINCIPAL)); Line 361: ExtMap result = outputMap.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); temp var? Line 362: int authResult = outputMap.<Integer>get(Authn.InvokeKeys.RESULT); Line 363: Integer reportReason = authResultToReportReasonMap.get(authResult); Line 364: if (reportReason == null) { Line 365: reportReason = Acct.ReportReason.PRINCIPAL_LOGIN_FAILED; Line 373: new ExtMap().mput( Line 374: Acct.PrincipalRecord.PRINCIPAL, Line 375: principal).mput( Line 376: Acct.PrincipalRecord.AUTH_RECORD, Line 377: result) put also user per what user specified. Line 378: ) Line 379: ); Line 380: if (authResult != Authn.AuthResult.SUCCESS) { Line 381: log.infoFormat( http://gerrit.ovirt.org/#/c/27070/4/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 151: ConcurrentMap<String, Object> sessionMap = entry.getValue().contentOfSession; Line 152: Date hardLimit = (Date) sessionMap.get(HARD_LIMIT_PARAMETER_NAME); Line 153: Date softLimit = (Date) sessionMap.get(SOFT_LIMIT_PARAMETER_NAME); Line 154: if ((hardLimit != null && hardLimit.before(now)) || (softLimit != null && softLimit.before(now))) { Line 155: removeSessionImpl(entry.getKey(), Acct.ReportReason.PRINCIPAL_SESSION_EXPIRED); most of this hank can go into the VALID_TO patch, it is not related to this one. Line 156: } Line 157: } Line 158: } Line 159: Line 279: } Line 280: } Line 281: Line 282: private void removeSessionImpl(String sessionId, int reason) { Line 283: AcctUtils.reportRecords(reason, please also put principal. Line 284: (ExtMap) getData(sessionId, AUTH_RECORD_PARAMETER_NAME, false), Line 285: (ExtMap) getData(sessionId, PRINCIPAL_RECORD_PARAMETER_NAME, false) Line 286: ); Line 287: sessionInfoMap.remove(sessionId); -- 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: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
