Add expiring user support to web panel - #101
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional user expiration with web/API controls, persistent metadata, and automated lifecycle cleanup.
Changes:
- Adds expiration fields, validation, UI, and API support.
- Adds migration, cleanup scheduling, concurrency controls, and retry-safe removal.
- Extends tests and architecture documentation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/web/mod.rs |
Expiration API, UI, validation, and tests |
src/poller/mod.rs |
Periodic expired-user cleanup |
src/db/peers.rs |
Expiration persistence and queries |
src/admin/mod.rs |
Lifecycle orchestration and cleanup |
src/admin/client_manager.rs |
Resumable identity-safe removal |
migrations/0008_add_peer_expiration.sql |
Expiration schema |
docs/ARCHITECTURE.md |
Architecture updates |
Suppressed comments (1)
amneziawg-web/src/web/mod.rs:2553
- The HTML edit path has the same clear/remap race as the API path: an existing migrated managed user can temporarily have
has_config = 0while config discovery rebuilds mappings, causing a valid expiration edit to fail as unmanaged. Hold the config-mapping lock across the peer read, eligibility check, and expiration write, or make mapping persist the stable managed identity.
let managed_client_name = managed_client_name_for_expiration(&existing);
if expiration_update.as_ref().is_some_and(Option::is_some)
&& managed_client_name.is_none()
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amneziawg-web/src/admin/mod.rs:246
- The lifecycle lock held here does not serialize creation persistence with
execute_update_peer_expiration. The newly synced peer can be inserted/mapped by the poller whilepersist_created_peerwaits forCONFIG_MAPPING_LOCK; a concurrent PATCH can then successfully set a deadline, after whichupsert_created_peerunconditionally overwritesexpires_atwith the create request's older value (includingNULL). AcquireEXPIRATION_STATE_LOCKbefore the lifecycle lock and retain it through persistence so the lock order remains consistent with cleanup and successful expiration edits cannot be lost.
// Keep a single lifecycle lock held across native creation and expiration
// persistence. If the database write fails, rollback runs under the same
// lock, so no concurrent add/remove can strand or replace this client.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amneziawg-web/src/db/peers.rs:701
- The stale-peer pass can discard the retry state created by
remove_client_resumable. For example, a manual removal of a permanent or future-expiring user can remove the server block/live peer and then fail while deleting the client config;execute_remove_user_innerdeliberately keeps the row so the operation can retry, but the next poll sees the key absent and this predicate allows the row to be deleted as stale. The client config is then orphaned and no later lifecycle retry can find the peer. Persist and exclude an explicit removal-in-progress state (covering manual and expiration removals), rather than inferring retry eligibility only from whether the deadline is currently due.
AND (expires_at IS NULL
OR julianday(expires_at) IS NULL
OR julianday(expires_at) > julianday(?))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
amneziawg-web/src/web/mod.rs:2585
- On an expiration conflict, this returns an error page after
update_peer_metadatahas already committed the submitted name/comment. Thus a rejected form submission silently applies part of the edit and skips its audit record. Persist metadata and expiration in one guarded transaction so the conflict rolls back the full form update.
if let Some(ref expires_at) = expiration_update {
if crate::admin::execute_update_peer_expiration(
&state.db,
id,
expires_at.as_deref(),
expires_at.as_deref().and(managed_client_name),
)
amneziawg-web/src/web/mod.rs:2185
- If removal has already set
removal_pending, this call returnsNoneand the handler responds 409, but the precedingupdate_peer_metadatahas already committed any requested display-name/comment changes. The failed PATCH is therefore partially applied without an audit event. Apply metadata and expiration atomically under the expiration/removal guard so a conflict leaves every requested field unchanged.
This issue also appears on line 2579 of the same file.
if let Some(ref expires_at) = expiration_update {
if crate::admin::execute_update_peer_expiration(
&state.db,
id,
expires_at.as_deref(),
expires_at.as_deref().and(managed_client_name),
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
amneziawg-web/src/admin/mod.rs:523
- Opening the config directory as the lock now fails before native removal when
AWG_CONFIG_DIRis absent. The binary explicitly permits a missing directory at startup (main.rs:147-149), andremove_client_innertreats it as “no config files to remove,” but this call makes that branch unreachable. If the directory is deleted while the service is stopped, startup expiration cleanup can never revoke the still-active server peer and every periodic retry fails withENOENT. Establish a securely validated lock directory before this point, or use a stable lock inode that does not depend on the client directory existing.
let lock_result = acquire_lifecycle_lock(config_dir).map_err(map_lock_error);
amneziawg-install.sh:5799
- When the panel's configured directory is missing, this recreates it as root with mode 0700. The web installer deliberately makes
AWG_CONFIG_DIRservice-owned (amneziawg-web/scripts/amneziawg-web-install.sh:1081-1104), so after a root CLI add/remove recreates a missing directory, the running panel cannot open the shared lock or manage configs. Preserve the configured service ownership/access when recreating the directory, or avoid creating the panel directory solely to obtain the lock.
mkdir -p "${lock_dir}" || {
umask "${old_umask}"
echo "ERROR: could not create client lifecycle directory '${lock_dir}'" >&2
return 1
}
|
Also addressed both suppressed findings from the latest review in be8f94e:
Shell coverage verifies custom database-path contention, missing client-directory behavior, and refusal to recreate a missing panel state directory. Both shell files pass bash -n, git diff --check passes, component versions validate, and 423 Rust tests pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
amneziawg-install.sh:1741
- If the installed unit uses its supported inline
Environment=AWG_WEB_DB=...settings and has no EnvironmentFile, this fallback locks the client-config directory. The shippedpackaging/amneziawg-web.service:29-32explicitly supports that setup, while the web process always locks the database parent, so installer and web mutations are no longer serialized. Resolve inline unit settings too, or fail instead of selecting a different lock inode.
fi
resolveWebPanelConfigDir
amneziawg-install.sh:1734
- This rejects a documented database configuration:
AWG_WEB_DB=sqlite:///var/lib/amneziawg-web/awg-web.dbis accepted by the web process (db/mod.rs) but does not begin with/, so installer--add-client/--remove-clientnow fail before acquiring the shared lock. Normalize supported SQLite URLs (and relative paths using the service working directory) to the same filesystem directory asmain.rs.
This issue also appears on line 1739 of the same file.
if [[ -n "${database_path}" ]]; then
if [[ "${database_path}" != /* || "${database_path}" =~ [[:space:][:cntrl:]] ]]; then
echo "ERROR: refusing unsafe AWG_WEB_DB '${database_path}'" >&2
return 1
fi
dirname -- "${database_path}"
amneziawg-web/docs/ARCHITECTURE.md:85
- This architecture description is now incorrect:
main.rsderiveslifecycle_lock_dirfrom the database parent, and the installer resolver does the same. Saying the client-config directory is locked obscures the persistence invariant and conflicts with the implementation.
The web panel holds an advisory lock on the open client-config directory across
the complete managed-client lifecycle, including database persistence and
client-config cleanup. Supported installer `--add-client` and `--remove-client`
operations lock the same directory descriptor, preventing an out-of-band
same-name replacement from appearing inside a web lifecycle operation without
introducing a mutable lock pathname in the service-writable directory.
amneziawg-web/docs/MVP.md:30
- The implementation now shares the database state directory descriptor, not the client-config directory. Update this summary so it matches
main.rs::lifecycle_lock_dirandresolveClientLifecycleLockDir.
|24 | Lifecycle locking + validation | Cross-process add/remove lock on the client-config directory shared with installer CLI operations, plus installer-name validation for managed user actions |
|
Addressed all four suppressed findings from the latest review in 7c25e66:
Verification: 424 Rust tests pass; the changed shell files pass bash -n; the shipped inline-only service fixture resolves to /var/lib/amneziawg-web; git diff --check and component-version validation pass; Clippy has no new warnings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amneziawg-web/src/admin/client_manager.rs:1123
create_clientnow tries to openconfig_diras the lock directory beforeprepare_client_config_dirruns. For the documented case where the client directory does not exist yet, this returnsENOENTand never recreates the directory, regressing the previous behavior. Prepare/validate the directory before using it as the standalone lock inode; the production path can still acquire its independent persistent lock first.
script_bridge::validate_client_name(name)?;
let _lock_file = acquire_creation_lifecycle_lock(config_dir, config_dir)?;
create_client_with_lifecycle_lock(config_dir, name, disabled_keys, ip_override)
|
Also addressed the latest suppressed finding in 3f101f0: standalone |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
amneziawg-install.sh:1846
- The binary's public
--config-diroption takes precedence overAWG_CONFIG_DIR, but this resolver only readsEnvironment=/EnvironmentFile=. If an operator supplies that option in the effectiveExecStart, installer add/remove operations still copy or delete configs in the environment/default directory rather than the directory the panel scans. Account for effective command-line overrides, or configure one authoritative path that both processes consume.
configured_dir="$(readWebPanelSetting AWG_CONFIG_DIR)" || return 1
|
Also addressed the latest suppressed finding in f3c8bed: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
amneziawg-install.sh:1917
- This rejects valid zero-arity CLI options.
Configdefinesauth_enabledandauth_secure_cookieas boolean Clap flags, so an effective command such asamneziawg-web --auth-enabled --database-url=...is valid, but this branch sees the following--...token and returns status 2. Both lifecycle-lock and config-directory resolution then fail, disabling installer add/remove whenever those supported flags are configured inExecStart. Skip known boolean flags (or parse option arity from the application's CLI contract) while continuing to reject actual positional arguments.
else
option_name="${token}"
index=$((index + 1))
if [[ "${index}" -ge "${#exec_args[@]}" || "${exec_args[index]}" == --* ]]; then
echo "ERROR: missing value for '${option_name}' in web panel ExecStart" >&2
return 2
amneziawg-install.sh:1718
systemctl show --value EnvironmentFilesserializes thea(sb)list on one property line, with entries such as/etc/first.env (ignore_errors=no) /etc/last.env (ignore_errors=no). This loop treats that whole line as one entry; the greedy match makesconfigured_envcontain both files, andvalidateWebPanelEnvFilerejects it for whitespace. A normal unit with multiple effectiveEnvironmentFile=directives therefore makes installer add/remove fail instead of resolving the ordered settings. Parse and iterate eachpath (ignore_errors=...)tuple from the property value (while still rejecting unsupported escaping).
while IFS= read -r configured_env; do
[[ -n "${configured_env}" ]] || continue
if [[ ! "${configured_env}" =~ ^(.*)[[:space:]]+\(ignore_errors=(yes|no)\)$ ]]; then
echo "ERROR: unsupported systemd EnvironmentFiles value '${configured_env}'" >&2
return 1
fi
configured_env="${BASH_REMATCH[1]}"
|
Addressed both suppressed findings from the latest review in d205f45:
All PR checks are green, including shellcheck, syntax, every distro integration, the privileged Ubuntu smoke job, MSRV, and the full unit suite. |
Adds optional user lifetimes while keeping existing and explicitly permanent users non-expiring.
Stores expiration metadata, displays and edits expiration status in the web panel, and performs startup/periodic cleanup through the existing user-deletion path so VPN configuration, keys, and related state are removed consistently.
Includes concurrency safeguards, retry-safe cleanup, migration coverage, and architecture documentation updates.
Verification: cargo test --all-targets (414 passed); cargo check --all-targets; git diff --check.