Repository navigation
v4.0 fixes (auth, cache, snapshot, Proxmox, OAuth2 logout, TOTP, X2GO, PhysicalMachines) - #191
Merged
Merged
Conversation
AuthenticatorException was missing from the exception translation table, so any authenticator rejecting a login showed the generic unknown error page instead of the access denied one. (cherry picked from commit a47f053)
The mapping is resolved by exact type, so a subclass raised by a plugin falls through to the unknown error instead of inheriting the base entry. (cherry picked from commit 4fc9019)
Previously, get_attributes_regex_field returned the prefix as the attribute name to request from LDAP, and process_regex_field only used the prefix as a key — the value was never prefixed. For UCA's 'alumno:alumno' / 'pas:pas' / 'pdi:pdi' mappings this meant the wrong attribute was fetched and the role string was missing its prefix, so group matching silently failed. - get_attributes_regex_field: return the right-hand side of ':' as the attribute to look up. - process_regex_field: when 'prefix:attr' is used, look up 'attr' and prepend 'prefix' to each value (alumno + 'S' -> 'alumnoS'). Existing '+' concat and '**' prepend syntax are unchanged. Adds unit tests covering the colon prefix, the self-rename case, and the pre-existing syntaxes. (cherry picked from commit a470dba)
(cherry picked from commit 583d4b1)
…operations (cherry picked from commit 9c45862)
(cherry picked from commit b81d291)
…lease (cherry picked from commit cee0c13)
…ool stats (cherry picked from commit 5a42f89)
… it is ready
Backport of 6c22b8b74 from 5.0 ("Fixed back to cache timing with snapshot recovery").
forced_move_assigned_to_cache_l1() marked the user service USABLE and only then
called move_to_level(), which runs the move-to-cache queue and, for snapshot pools,
the snapshot recovery operation. For the few seconds that recovery took, the machine
was already listed as a valid cache entry and could be handed out to a user.
Leave it PREPARING and let move_to_level() set the final state from the outcome of
the operation, so the machine only becomes usable once it really is.
Applies on top of dev/v4.0-new-feature-back-to-cache-snapshot-recovery: plain v4.0
does not call move_to_level() there, so the window this closes does not exist yet.
(cherry picked from commit c4ca05d)
Some IdPs and copy-paste sources embed invisible code points in the username attribute (BOM U+FEFF, ZWSP, ZWNJ, ZWJ, soft hyphen, etc.). The result: visually identical strings reach the DB as different rows, creating duplicate user objects under the same authenticator. The chokepoint is Authenticator.get_or_create_user, which stored the username as-is via get_or_create(name=...). Strip invisible / control chars, NFKC-normalize, and trim there. SAML, OAuth2, LDAP, ticket auth, REST and web login all funnel through it. Includes a minimal test for the helper covering BOM in three positions, ZWSP / ZWNJ / ZWJ, soft hyphen, fullwidth -> ASCII via NFKC, NUL / tab, case preservation, and empty input. (cherry picked from commit bf03194)
…zation Follow-up on the username normalization, fixing three problems: * Existing rows were left polluted, so normalizing only on write meant the next login no longer matched them and created a new, empty user (losing groups, permissions and assigned user services). A data migration now normalizes those names and merges the duplicates into a single row, moving user services, permissions, group memberships, logs, properties and child users to the surviving one. * The normalization ran before the invisible chars were stripped. Removing a zero width joiner between a letter and its combining mark leaves a decomposed string, which no longer matches the precomposed form the next login sends. Strip first, compose after. * NFKC folded compatibility code points into ASCII, so two distinct directory identities collapsed into a single UDS user, and one inherited the other's groups and services. Use NFC. (cherry picked from commit 299b025)
The normalization added to Authenticator.get_or_create_user was not applied on the other paths that look users up (or create them) by name, so a username carrying invisible code points (BOM, zero-width, soft hyphen, ...) still failed to match the -- already clean -- stored row: * Authenticator.is_user_allowed: an existing user was reported as non existing. * Handler.get_user: the REST session username did not match its user row. * Users.save_item: a name pasted from the admin UI created a polluted row again (and, as a side effect of normalizing first, a name made *only* of invisible chars is now rejected by the existing "Username cannot be empty" check). Also reduces samples/uds_dedupe_users.sql to its diagnostic query. The apply part relied on FK CASCADE to clean up the merged rows, which silently dropped their assigned user services; the 0048_dedupe_user_names migration already does the merge properly on upgrade. (cherry picked from commit e606d7e)
Updates the import logic to accept a CSV-formatted string instead of a JSON array, improving compatibility and aligning with common data exchange formats. Enhances input handling by leveraging the CSV parser for more robust processing and easier integration. (cherry picked from commit 103cf8e)
(cherry picked from commit 7b90308)
(cherry picked from commit 7476a53)
…_logout Concurrent logout events (duplicate actor logouts, or the assigned_unused worker racing an interactive logout) could hit the same userservice. The second event saw a machine a prior logout had already returned to cache (cache_level != NONE) and called release() on it, destroying a valid L1 cache element and causing the observed "Creating cache" <-> "Removing" churn. - Serialize concurrent logouts by locking and re-reading the row (select_for_update inside transaction.atomic()); guard DoesNotExist for rows a prior event already released and cleaned. - When the machine was already handled (back in cache or no longer usable), early-return without releasing instead of destroying the cache element. - Save the cloned userservice before logging in forced_move_assigned_to_cache_l1: logging a not-yet-saved copy inserts an uds_log row with a NULL owner_id, silently swallowed before but poisoning the enclosing atomic transaction and breaking the following save(). Add regression tests covering no-cache release, already-in-cache skip, missing-row no-op, L1 overflow release, and L1 growth move-to-cache. Backport of 0cc1988 + 6a28eb9 from 5.0 (master-fix-logout-cache-race). (cherry picked from commit 33ff3bf)
The multiple IPs fixture included a 'localhost' entry that the test expected to be dropped because it does not resolve, and asserted a hostname obtained from a reverse lookup of 172.27.1.25-27. Both hold only on a machine whose resolver behaves like ours, so the test failed elsewhere. Same fix already applied on master. (cherry picked from commit 2db1b5c)
OAuth2Authenticator.logout() returned AuthenticationState.SUCCESS together with the logout URL, but the logout view only follows the URL when the state is REDIRECT. The configured provider logout URL was discarded, the provider session (e.g. Keycloak SSO cookie) stayed alive and the user was logged back in without credentials. (cherry picked from commit 6687598)
ProxmoxService.is_deleted() returned True on any exception raised while looking up the VM. A read timeout, an unavailable node or an auth error made the deferred deletion worker consider the VM gone, drop it from the queue and never retry, leaving the VM alive in Proxmox while UDS had already released it. This happens most under storage contention (e.g. Ceph with cfs-lock timeouts), which is also when destroy tasks fail. - is_deleted() now returns True only on ProxmoxNotFound; other errors reach the deferred deletion worker, which retries retryable ones. - ProxmoxClient.do_get() maps requests.Timeout to ProxmoxConnectionError so read timeouts are retryable instead of surfacing as raw requests exceptions. POST/DELETE are left unchanged: retrying them on timeout could duplicate a clone or a destroy. (cherry picked from commit f95e8bd)
…tops tracking a vm DynamicService.delete() sets a deleting_<vmid> marker and the owner (a publication, for instance) keeps reporting 'removing' while it exists. The only place that cleared it was the success path of process_deleting(). The worker also stopped tracking an item, without clearing the marker, when the vm was already gone (NotFoundError, both in add() and in _process_exception()) and when it gave up after MAX_FATAL_ERROR_RETRIES or MAX_RETRAYABLE_ERROR_RETRIES. In all of those the owner stayed in 'removing' for ever. Now every path that drops the item calls notify_deleted(). Giving up still logs the error with the vmid, so the leftover vm can be cleaned by hand. (cherry picked from commit 824c5d8)
…ll hang from it Proxmox answers the DELETE of a template with a task id and then fails that task with 'base volume ... is still in use by linked cloned' while a linked clone exists. Nothing reads the task result, so UDS kept asking again until the deferred deletion gave up. The core waits for the machines of a publication before unpublishing it, but it does not count the errored or canceled ones, whose disks are still there. ProxmoxPublication now holds the delete until no machine of the publication is left in a state other than REMOVED, and issues it from op_delete_checker. If the deferred deletion gave up and released its marker while the template still exists, the checker asks for the deletion again instead of finishing. (cherry picked from commit b1cd1dc)
(cherry picked from commit 5705cb3)
… VM as deleted Proxmox answers OK to the VM removal before its disks are gone, so is_deleted saw the VM missing and the publication template was removed while the clone disks were still in use. execute_delete now stores the VM disks, and is_deleted removes any that remain and reports the VM as not deleted until none are left. (cherry picked from commit 4d6db18)
…gs to execute_delete only recorded the disks when no list was stored yet, so a list left behind by a deletion the deferred worker stopped tracking (ProxmoxNotFound short-circuit or retries exhausted) blocked recording the disks of the next vm reusing that vmid: the stored volids never matched, the key was dropped and the new disks were reported as gone. The list is now rewritten on every delete and released in notify_deleted. Disk detection also skipped any cdrom entry, which drops the cloudinit volume, a real volume that would be left on the storage. Detection now looks at the volume name instead, so vm-owned volumes are picked up wherever they are declared and iso images are still ignored. (cherry picked from commit 9b96895)
…s vmid Filtering the storage content by vmid cannot work for a vm that is already gone: proxmox drops the owner of a volume along with the vm, so the listing came back empty and the disks were reported as deleted while still there. The volume ids recorded before the removal are now matched against the plain content listing of each storage. That listing is cached for 5 seconds so the deletions running in parallel share a single read, well below the 10 seconds between checks, and a volume that disappears between the listing and its removal no longer raises: the next check retries if it is still around. (cherry picked from commit 8e02028)
…re-delete inspection (cherry picked from commit 99398ff)
(cherry picked from commit 5b29bff)
The group membership check ran an unbounded search, so directories with more entries than their own page size (1000 by default on Active Directory) answered sizeLimitExceeded. python-ldap raises on that result and discards the partial entries, so the test reported a raw LDAP error dict while the authenticator itself worked fine. Bound that search to LDAP_RESULT_LIMIT and treat the size limit as a pass, as the other checks already do: reaching the limit proves matching groups exist. (cherry picked from commit 425b195)
A rejected TOTP code left no trace, so a clock drift between the client and the server could not be told apart from a wrong or a replayed code. Traces the result, the valid window and the server time; the code itself is never traced. (cherry picked from commit f5b66a2)
Logging every validation fills the log with one line per successful login, which on a deployment with many users is noise. Only the failures stay: an invalid code and an already used one, both at warning level, with the code length, the valid window and the server time. A code that passes leaves no trace, and the code itself is never logged. (cherry picked from commit 9922bd2)
Refreshes IP and MAC from the server model before triggering wake-on-LAN, ensuring accurate MAC usage for user-services migrated from older versions where the MAC might be missing. Also prevents WOL attempts with unknown MAC addresses to improve reliability. (cherry picked from commit 8438541)
Changes the lookup for the X2GO client executable to pass a list of paths, ensuring compatibility with the expected argument type and preventing runtime errors if the path is not found. Updates signature to reflect the code adjustment. (cherry picked from commit e0877c1)
…re moving to cache (cherry picked from commit 15eafbd)
PublicationManager.unpublish already refuses to unpublish while the publication has user services outside INFO_STATES, and destroy() is only reached through it (or before any user service exists), so the check in op_delete never changed the outcome. The deferred deletion marker release in the worker stays. (cherry picked from commit e8533e1)
Adolfo review (2026-10-19 16:36): no se puede añadir una migración que no existe en la versión anterior cuando ya tenemos la 5.0 como release con migraciones 0048..0054. Meter 0048 en 4.0 rompe el camino de migraciones al actualizar a 5.0. Por qué este commit: * Quita server/src/uds/migrations/0048_dedupe_user_names.py. * Quita server/tests/migrations/__init__.py y test_dedupe_user_names.py. * Quita server/samples/uds_dedupe_users.sql (era solo diagnóstico y referenciaba la migración borrada en el comentario; sin la migración queda mintiendo). Lo que se mantiene (commit d439584): * normalize_username() en uds/core/util/auth.py (NFC + strip invisibles). Se invoca desde REST/handlers.py al hacer login, así que cada usuario existente se renombra la próxima vez que entre. Sin migración Django, el rename es progresivo. Lo que NO entra en este PR (decisión de Adolfo): * El script que limpia filas duplicadas (la función _dedupe_user_names). Se entrega por separado (parche cliente o udspkg) cuando un cliente reporte el problema.
Contributor
Author
|
Removed migration Kept from d439584: Not in this PR: cleanup of rows that are already duplicated. If a customer reports it, it is shipped as a separate script, not as a Django migration. |
The migration resolves every configured server through DNS: IPs get their reverse name and names that do not resolve are dropped. The test relied on the host resolver, so it only passed on a network where 172.27.1.x had PTR records and "localhost" did not resolve. With systemd-resolved, which answers for "localhost", the group got four servers instead of three. Patch dns.resolver.resolve during the migration so PTR queries return a name and forward queries fail, which is the scenario the test describes.
dkmstr
approved these changes
Oct 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Branch
dev/v4.0-fixes-pr(35 commits on top oforigin/v4.0). Fixes and patches only.Main blocks
prefix:attrregex syntax, authenticator error mapping.259c52932 fix(cache)sits on top).sizeLimitExceeded.What is NOT included
0048_dedupe_user_names.py, its test andsamples/uds_dedupe_users.sqlwere removed (b3ace3bf2): 5.0 already ships 0048..0054, so a new 0048 in 4.0 would break the upgrade path. Existing polluted rows, if a customer reports them, are handled with a separate script, not a migration.fixes/v4.0-openshift,backport-openshift-v4.0).iam_instance_profile(feature).is_avaliable->is_available(non-local name in a production release).91f787830reverts the machine check in the Proxmox publication delete.Tests run on the final tree
tests/core/util/,tests/auths/oauth2/,tests/REST/auths/test_users_groups_users.py: passed.Stats