[MAGNOLIA-4459] AuditLogging logs twice login attempt Created: 28/Jun/12  Updated: 15/Oct/14  Resolved: 13/Oct/14

Status: Closed
Project: Magnolia
Component/s: core
Affects Version/s: 4.5, 5.0
Fix Version/s: 4.5.23, 5.3.5

Type: Bug Priority: Neutral
Reporter: Milan Divilek Assignee: Christopher Zimmermann
Resolution: Fixed Votes: 0
Labels: support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
duplicate
is duplicated by MAGNOLIA-5829 Duplicate logging in audit trail Closed
relation
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Steps to reproduce:

  • Allow audit logging for login (Configuration /server/auditLogging/logConfigurations/login/ property active="true")
  • Logout
  • Login any user
  • Look in to magnolia-access.log file
    -> There is logged twice login attemp
     
    28.06.2012 13:09:16, login, superuser, 127.0.0.1, Success
    28.06.2012 13:09:16, login, superuser, 127.0.0.1, Success
    

This behaviour is happen only for "Login" log. "Logout, Activation, Move,.. " are logged correctly.



 Comments   
Comment by Christopher Zimmermann [ 07/Oct/14 ]

Removed redundant call to AuditLoggingUtil.log in LoginFilter.

Comment by Jan Haderka [ 07/Oct/14 ]

Excuse my ignorance, but how is change in AbstractI18nContentSupport relevant to this ticket?
And secondly, removing the log you removed means that login attempts over ntlm or other sso solution will not be logged at all and those login attempts that are denied due to memory issues will also not be logged at all. Shouldn't you rather remove the other log entries and keep the top most one?

Comment by Christopher Zimmermann [ 08/Oct/14 ]

In consultation with Jan - keeping the current approach as the login status can change, and adding additional calls to uditLoggingUtil#log in the clauses did not have them. Also logging the login failure in the isMemoryLimitReached clause.

Comment by Jan Haderka [ 08/Oct/14 ]

Looks good. Just one more case to cover:

if (subject == null) {
  90                     log.error("Invalid login result from handler [{}] returned STATUS_SUCCEEDED but no subject", handler.getClass().getName());
  91                     throw new ServletException("Invalid login result");
  92                 }

we log error and throw exception, but we should also log audit record about it as having successful login w/o subject is very suspicious.

Comment by Christopher Zimmermann [ 08/Oct/14 ]

Also write failure to access log if subject=null.

Comment by Aleksandr Pchelintcev [ 09/Oct/14 ]

Review:

  • un-set the MgnlContext in test tearDown() method.

Otherwise - looks good.

Generated at Mon Feb 12 03:55:59 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.