Fixes #39512 - Add audit logs for user login/logoff/failed attempts#11097
Fixes #39512 - Add audit logs for user login/logoff/failed attempts#11097sbernhard wants to merge 1 commit into
Conversation
12c84aa to
720b56f
Compare
sbernhard
changed the title
Add manual audit events for user authentication activity, covering successful login, logout, and failed login attempts. Re-use the method to manually create a Audit event for the user impersonate method which already created manually an Audit log. Show failed login attempts for admins without limiting to location / organization. Co-authored-by: OpenAI Codex <codex@openai.com>
720b56f to
eec4b8e
Compare
| log_bruteforce | ||
| log_authentication_event('failed_login', nil, "Blocked login attempt from #{request.remote_ip}") |
There was a problem hiding this comment.
I want to say that the ludit-Logging should be part of log_bruteforce(), assuming we want to audit any Brute-Force attempt. Downside: we cannot use log_authentication_event() in Foreman::Controller::BruteforceProtection.
Also it might not be tied to User's ID in other occurrences of log_bruteforce().
There was a problem hiding this comment.
log_bruteforce is a method with a single line - which logs the login attempt. So, I dont see the requirement to move the log_authentication_event() to this method.
There was a problem hiding this comment.
The idea is to add the audit-log also into log_bruteforce(), so all invocations of that method also create an audit-log. For instance the log_bruteforce() method is also used here: https://github.com/ATIX-AG/foreman/blob/dc651706cd492de2ead0b1db57adf34f17419811/app/controllers/api/base_controller.rb#L203
There was a problem hiding this comment.
this would then log user access attempts to the API. actually, the current implementation of this PR is not about API.
| previous_user = User.current | ||
| User.current = nil | ||
| create!(audit_attributes) | ||
| ensure | ||
| User.current = previous_user | ||
| end |
There was a problem hiding this comment.
Got curious and now I know (finally) that this has no side-effects on other Threads, because User.current() is explicitly written to only modify Thread-variables 👍
https://github.com/ATIX-AG/foreman/blob/develop/app/models/concerns/foreman/thread_session.rb
m-bucher
left a comment
m-bucher
left a comment
There was a problem hiding this comment.
For what it's worth:
- code looks good to me
- tested on my forklift and works as expected
| or(arel_table[:auditable_type].in(untaxable.map(&:to_s))). | ||
| or(arel_table.grouping(arel_taxed_only_by_organization)). | ||
| or(arel_table.grouping(arel_taxed_only_by_location)) | ||
| statement = statement.or(arel_table.grouping(arel_global_failed_login)) if User.current.admin? |
There was a problem hiding this comment.
I assume this is to deny a User that can read Audits but not the List of Users the ability to implicitly find out which User-Accounts exist/are disabled.
That is OK for me 😄
2 participants
Successful login:
Logout:
Failed login:
In the UI, it looks like this:
