Skip to content

fix(logs): prefer username over numeric user id in log messages - #3672

Open
IshanA2007 wants to merge 1 commit into
DefGuard:stable/2.xfrom
IshanA2007:fix/issue-719-username-in-logs
Open

IshanA2007 wants to merge 1 commit into
DefGuard:stable/2.xfrom
IshanA2007:fix/issue-719-username-in-logs

Conversation

@IshanA2007

Copy link
Copy Markdown

📝 New contributors

  • I have read, understand, and agree to the Contributor Agreement. By checking this box, I confirm I have the right to contribute this work and I grant Defguard sp. z o.o. the necessary rights to use my contribution as outlined in the full agreement.: https://github.com/DefGuard/defguard/blob/stable/2.x/CLA.md

📖 Description

Part of #719. Not using a closing keyword, see the note at the end.

Several debug!/info!/warn!/error! lines printed a user's numeric database id
even though the corresponding User object was already bound in scope. Switched
those to username, matching the convention already used elsewhere in the codebase
("User {} listing ACL aliases", session.user.username and similar):

  • crates/defguard_core/src/handlers/yubikey.rs: delete/rename handlers
  • crates/defguard_core/src/handlers/forward_auth.rs: session validation
  • crates/defguard_core/src/handlers/openid_flow.rs: authorize flow
  • crates/defguard_core/src/enterprise/allowed_ips/mod.rs: ACL-derived AllowedIPs
  • crates/defguard_core/src/grpc/proxy/client_mfa.rs: posture check, inactive user
  • crates/defguard_setup/src/handlers/initial_wizard.rs: admin creation, setup guard

Where an id still carries information the username doesn't, it is kept alongside
rather than replaced, e.g. the initial-admin id (which is stored in settings) and
the finish_setup mismatch guard, where seeing only a name and an id would make the
error impossible to interpret.

Left unchanged: log lines with only a raw id and no User object, and lines that
fire precisely because the lookup failed. Also unchanged are the hot paths
(enterprise/db/models/acl.rs rule evaluation, enterprise/firewall/mod.rs SNAT
binding generation) and the external directory-sync modules, whose "user id" is
JumpCloud's/Entra's identifier rather than ours.

That leaves the other half of the issue, adding a minimal SQL query to resolve the
username where no User object is in scope, undone, which is why this isn't marked
as closing #719. In the ACL and firewall paths a query per log line would run
per-rule/per-binding, so I'd rather check with you on the approach before doing it.
Happy to follow up either way.

🛠️ Dev Branch Merge Checklist

Documentation

  • Not applicable: no environment or deployment changes.
  • I have commented on my code, particularly in hard-to-understand areas: no new
    logic, log message content only.

Testing

  • I have prepared end-to-end tests for all new functionalities: not applicable,
    no new functionality.
  • I have performed end-to-end tests manually: not performed for the yubikey and
    posture-check handlers, which have no existing automated coverage.
  • New and existing unit tests pass locally with my changes
    (cargo test -p defguard_core --lib enterprise::allowed_ips: 20 passed;
    cargo test -p defguard_setup --test integration -- initial_setup: 17 passed).
    cargo +nightly fmt --all --check and cargo clippy --all-targets --all-features -- -D warnings are clean across the workspace.

Deployment

  • Not applicable.

Prepared with AI assistance (Claude Code).

Several log lines printed a user's numeric database id even though the
corresponding User object (with its human-readable username) was
already available in scope, making the logs harder to read/correlate
for admins. Switch those log lines to the username. Log lines that
only have a raw user id (e.g. lookup failures, or values that would
require an extra DB query to resolve) are left unchanged.

Also covers five lines in forward_auth.rs and openid_flow.rs that were
missed in an earlier pass, and corrects three yubikey.rs handler lines
that were switched to the wrong User (the target user resolved via
user_for_admin_or_self, instead of the acting session.user) when the
log line is describing who performed the action.

The other half of the issue, adding a minimal SQL query to resolve a
username where no User object is in scope, is intentionally not done
here.

Part of DefGuard#719
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant