Alon Bar-Lev has posted comments on this change. Change subject: AAA: Introduce usage of Acct ......................................................................
Patch Set 3: (4 comments) > I have asked Juan and Vojtech, If you try to send a request to the server > with an expired session, the GUI will run its logout logic (i.e - present the > user that a logout occurred due to session experiation). do you think it's > enough? well, no... we need to report session expiration at server regardless of any code at the client. 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 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: > why not here after the for loop? don't i know in this case that i used all if non of the extension succeeded login you get to login filter with no credentials within session, then user will try to explicit login and event will be reported. look at it from another angle... if user did not specify credentials, why do we need to report he is to be blamed for invalid login attempt? 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); > why? see above... in addition... timeout, configuration errors and such are not the fault of the user. if we like we can add another category for reporting these... but I do not think it is required nor security need. 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: 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 359: if (reportReason == null) { Line 360: reportReason = Acct.ReportReason.PRINCIPAL_LOGIN_FAILED; Line 361: } Line 362: Line 363: AcctUtils.reportAuthRecord(reportReason, result); > what do you mean use the user of the input map? no... if you do not have principal you report the user as the end-user specified. the core cannot "invent"/"construct" anything without extension. 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 > you will have ConcurrentModificationException. I think you can. as then another thread that tries to use the map will fail.... no? so what concurrent in this? -- 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: 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
