Yair Zaslavsky has posted comments on this change. Change subject: aaa: Introduce usage of Acct ......................................................................
Patch Set 4: (12 comments) 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 cann ok. i will consolidate to one method. 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 Done 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/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 165: // Forward the request to the next filter in the chain: Line 166: chain.doFilter(req, rsp); Line 167: } Line 168: Line 169: private void reportAcct(int reportReason, String userName) { moving this to AcctUtils.reportUser Line 170: AcctUtils.report(new ExtMap().mput( Line 171: Acct.InvokeKeys.REASON, Line 172: reportReason Line 173: ).mput( 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 wh as explained - this is Acct.Principal_RECORD.User (a string) - so it's ok. Line 175: new ExtMap().mput( Line 176: Acct.PrincipalRecord.USER, Line 177: userName Line 178: ) 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. yes, we will go for pre-destroy. done. 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, hmm... why did we fail it this point? the failure for MLA happens later on. here - we just passed authz. Maybe i should move this report (as it reports on credentials based login) after the successful authentication. 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. done. 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? which one, i use result, authResult , and repotReason several times in the code. 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. Done. Line 378: ) Line 379: ); Line 380: if (authResult != Authn.AuthResult.SUCCESS) { Line 381: log.infoFormat( Line 375: principal).mput( Line 376: Acct.PrincipalRecord.AUTH_RECORD, Line 377: result) Line 378: ) Line 379: ); > this should probably sync with other signature of functions in AcctUtils... Done Line 380: if (authResult != Authn.AuthResult.SUCCESS) { Line 381: log.infoFormat( Line 382: "Can't login user \"{0}\" with authentication profile \"{1}\" because the authentication failed.", Line 383: user, 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 Done 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. done. 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
