Skip to content

Fixes #39857 - Store LDAP avatars with Active Storage - #11334

Open
jakduch wants to merge 1 commit into
theforeman:developfrom
jakduch:feature/active-storage-avatars-v2
Open

jakduch wants to merge 1 commit into
theforeman:developfrom
jakduch:feature/active-storage-avatars-v2

Conversation

@jakduch

@jakduch jakduch commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Why

LDAP photos are currently written into public/images/avatars. That requires a writable application directory, exposes static image URLs without the user controller's authorization, and requires shared filesystem storage when web processes run on different machines.

Store new photos as private Active Storage attachments instead. The default disk service still works for standalone installations; a shared service allows multiple web instances to read the same photo. A storage failure must not prevent LDAP authentication.

What changes

  • Decode LDAP photos without writing files or logging binary attributes.
  • Attach supported JPEG/PNG images, reuse unchanged photos, and verify uploaded bytes before replacing the current attachment.
  • Serve photos through an authenticated, user-scoped endpoint. Users can read their own photo; reading another user's photo requires view_users. Keep the generic Rails download routes disabled.
  • Preserve access to unmigrated legacy photos through the same endpoint.
  • Add foreman:avatars:migrate: preview by default, explicit APPLY=true, batched/resumable copy, content verification, and failure reporting. Existing attachments and original files are preserved.

Migration and rollback

Run the task on a machine that can read the old avatar directory, with the same storage configuration as the web processes. The old filename is not always a checksum of the decoded image, so verification compares actual source/uploaded bytes rather than assuming that filename is a content checksum.

Legacy public URLs remain accessible until an administrator retires the old directory after verification and retention. Back up both the database and file storage. No automatic deletion or destructive database migration is included; older Foreman versions cannot read photos created only as Active Storage attachments. See developer_docs/ldap_avatar_storage.asciidoc for commands and rollback limits.

Depends only on the already merged #11314. Selecting S3 is optional and handled separately.

Testing

Added tests for LDAP binary/Base64 decoding, login behavior, unchanged photos, own/other/hidden-user access, legacy downloads, migration preview/rerun/missing or corrupt files, historical Base64-derived filenames, path traversal, symbolic links, and storage failure.

At 6e79ec69d096ba4108147b6c4a7d8fa93bdd16b4, the five changed test files passed locally on Ruby 3.3.12, Rails 7.0.10 and PostgreSQL 13: 360 tests / 869 assertions, with no failures, errors, or skips. The focused migration file separately passed 11 tests / 31 assertions. Targeted RuboCop (four edited test files), Ruby syntax checks, and git diff --check passed.

The local test used existing native ARM images and a cached bundle, with only small missing standard-library gems added to a disposable container. Console/development/journald tools and optional cloud providers were excluded; ExecJS was disabled. This is not the full CI environment or a complete unit-suite pass. No production migration was performed.

The avatar tests now use ActiveJob::TestHelper: otherwise attaching a photo from the nontransactional helper test sends ActiveStorage::AnalyzeJob into the real Dynflow adapter and initializes its persistence during the test. The helper regression asserts that analysis is queued in the test adapter and dynflow.world is never called. The full CI matrix remains unchanged and is running again.

A previous targeted CI run at 0ca3aa81195c2d88ebe497ab88ba26d020968c7a passed 235 tests / 555 assertions on both Ruby 3.0 and 3.3. Temporary test filtering/reporting is not part of this diff.

Fixes https://projects.theforeman.org/issues/39857

AI assistance: Codex. Project signature: GPT_5.6 Sol.

@github-actions github-actions Bot added the Docs label Oct 15, 2026
@jakduch
jakduch force-pushed the feature/active-storage-avatars-v2 branch 6 times, most recently from 099bcfd to 39ef27a Compare October 20, 2026 14:38
@jakduch
jakduch force-pushed the feature/active-storage-avatars-v2 branch from 39ef27a to bc61256 Compare October 27, 2026 23:01
@jakduch

jakduch commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased on current develop and fixed test-job isolation in 6e79ec6.

The new JavaScript jobs hit the existing DateTimePicker test bug: at 23:04 UTC its "one hour in the future" value crossed midnight but kept the original date. The separate fix is #11280, which I also rebased. This PR does not change that component or its tests.

The unit log did not expose its cause. Locally, attaching an avatar in ApplicationHelperTest sent ActiveStorage::AnalyzeJob through the real Dynflow adapter, which attempted to initialize its persistence during the test and hit a database error. The avatar test classes now use ActiveJob::TestHelper. The helper regression verifies that the job goes to the test queue and never calls dynflow.world.

All five changed test files passed locally: 360 tests / 869 assertions, on Ruby 3.3.12 and PostgreSQL 13 using existing cached dependencies. Targeted RuboCop also passed.

The complete Foreman workflow has now passed on this revision, as has the container build. The remaining red check is the shared DateTimePicker JavaScript failure described above; #11280 fixes it. That fix passed its own Foreman tests but hit an independent Katello VCR cassette mismatch, so I retriggered its pipeline once. This avatar PR is waiting for the shared test fix before another full retry.

Use authorized private attachments instead of new public filesystem photos. Keep legacy downloads available and provide a non-destructive, resumable copy task with verification and access-control regression tests.

AI assistance: Codex. Project signature: GPT_5.6 Sol.
@jakduch
jakduch force-pushed the feature/active-storage-avatars-v2 branch from bc61256 to 6e79ec6 Compare October 27, 2026 23:22

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant