Fixed handlebars_cache_key to be app_type-aware, preventing cross-app_type cache poisoning - fixes #1279

[philayres]

Problem

Fixes #1279. analysis_plans_process_spec.rb and grant_aims_process_spec.rb both failed reproducibly at find('.common-templates--result-item h4', text: '<App> ...') — the master record's details-tabs nav-pills were missing the activity_log__project_assignments ("Analysis Plan"/"Grant Aims") tab entirely, even though the underlying record, page layout config, and access control were all correct.

Root cause

app/helpers/handlebars_precompiler_helper.rb#handlebars_cache_key was documented as "a cache key common to all users" and did not vary by current_user/app_type_id/access-control. But the master_tabs Handlebars partial it caches to disk (public/handlebars-<env>/partials/<id>-<hash>.js) genuinely does vary per app type (page_layout_panels filters Admin::PageLayout by current_user.app_type_id, and tab visibility depends on access control). Whichever request compiled the partial first permanently "poisoned" the shared on-disk file for every other app type/user, until an unrelated table's updated_at happened to change.

Verified via interactive debugging: deleting the stale compiled file and forcing a fresh compile for the same user correctly included the missing tab, proving the ERB/access-control logic was correct and the defect was purely in the cache-key scoping.

Fix

• handlebars_cache_key now embeds app_type_id directly in its digest (not just indirectly via role/access-control timestamps), so two different app_type contexts can never collide, even when neither has any Admin::UserRole/Admin::UserAccessControl rows yet.
• app_type_access_control_timestamps (shared by handlebars_cache_key and access_control_version) now scopes those queries to app_type_id: [app_type_id, nil], matching the established pattern used elsewhere (UserAndRoles#where_user_and_role, PageLayoutsHelper#page_layout_panels) — so changes to global/shared (app_type_id: nil) roles or access controls also correctly invalidate the cache key for every app type.
• Extracted current_user_or_admin_app_type_id and app_type_access_control_timestamps as shared private helpers, eliminating duplicated query logic between handlebars_cache_key and access_control_version.

Testing

• 6 new specs in spec/helpers/handlebars_precompiler_helper_spec.rb covering: cross-app_type cache-key/filename divergence, cross-context write poisoning, app_type_id collision safety when role/UAC timestamps are identical, and global (app_type_id: nil) role change invalidation.
• Full handlebars_precompiler_helper_spec.rb (52 examples), handlebars_precompiler_spec.rb, application_helper_spec.rb, memcached_clear_template_recovery_spec.rb, template_resilient_versions_spec.rb, global_template_historical_versions_spec.rb, versioned_template_config_spec.rb: all passing, no regressions.
• Both originally-failing system specs now pass: analysis_plans_process_spec.rb (1 example) and grant_aims_process_spec.rb (2 examples).
• rubocop clean on all changed files.

Files changed

• app/helpers/handlebars_precompiler_helper.rb
• spec/helpers/handlebars_precompiler_helper_spec.rb

[philayres]

Code Review — remaining cross-user/role cache poisoning

Summary

This PR correctly fixes the cross-app_type poisoning (the root cause of the failing specs). However, a review of the full compilation pipeline found that the same class of bug exists one level down: shared compiled files between users in the same app_type.

Root cause of the remaining issue

write_handlebars_template skips recompilation if the on-disk file already exists:

return relative_path if File.exist?(compiled_file)

The compiled filename is derived from handlebars_cache_key, which now includes app_type_id + the app-type-wide MAX updated_at of UserRole/UserAccessControl — identical for every user in that app_type.

But the content of master_tabs (and any other access-filtered partial) is rendered per-user, via:

• master_viewables → Admin::UserAccessControl.viewable_tables(current_user) — keyed on user.id
• current_user.has_access_to?(:access, resource_type, resource_name) — per-user role check

So two users in the same app_type (one privileged, one restricted) get the same compiled filename → whichever request compiles first poisons the file for the other.

The per-user multi-file bundle (requested-templates-<user_id>-...) does not rescue this, because it reads the shared individual compiled files via read_handlebars_template.

Non-issues confirmed

• Deletion-based invalidation: configs are retired with disabled: true, which bumps updated_at, so the existing MAX(updated_at) keys already capture retirement correctly. No additional handling needed.

Plan to address

In handlebars_cache_key, add the user/admin id to the digest:

# Before (this PR):
Digest::SHA256.hexdigest("#{ver}-#{items}-#{app_type_id}-#{userrole}-#{uac}")[0..12]

# After:
Digest::SHA256.hexdigest("#{ver}-#{items}-#{app_type_id}-#{user_id}-#{userrole}-#{uac}")[0..12]

This propagates automatically to handlebars_compiled_filename, the recompile-skip check in write_handlebars_template, access_control_version, and the multi-file name.

Refactor current_user_or_admin_app_type_id into a shared current_user_or_admin private helper so user_id and app_type_id are resolved from the same object (no double Warden call).

Do not add current_sign_in_at — it would bust the cache on every login; role/config changes are already caught by the updated_at MAX timestamps.

Disk growth: HandlebarsPrecompiler.cleanup_public_dir already wipes all compiled files on every server start/deploy and resets server_cache_version. Per-user individual files follow the same model as the already-per-user multi-file bundles.

Tests to add

1. Same app_type, different user id → different cache key and different compiled filename.
2. write_handlebars_template: two same-app_type users writing different content for the same template_id → separate compiled files (parallel of the existing cross-app_type poisoning test).
3. Same user id + same app_type + same timestamps → identical key (dedup still works per user).
4. Adjust the existing "app_type_id collision safety" test: add a variant with the same user id but different app_type, so both dimensions remain independently exercised now that user_id is also in the digest.

Suggested follow-up

Implement on branch handlebars-cache-key-user-scope-1279 off up-develop, either amending this PR or as a follow-up PR before merge.

[philayres]

Update — per-user scoping added, branch squashed

The branch has been squashed to a single commit (b798ea37e) and force-pushed, now covering both app_type and per-user cache-key scoping.

New changes since the app_type-only fix

• Per-user cache key: handlebars_cache_key now folds the resolved user/admin id into the digest. Two users in the same app_type no longer share a compiled master_tabs (or any access-filtered) partial — its content genuinely varies per user via master_viewables / current_user.has_access_to?, so app_type alone was insufficient to prevent cross-user cache poisoning.
• User/Admin id-collision safety: the resolved object's class name (User vs Admin) is also folded in, so a User and an Admin that happen to share the same id can never collide.
• Single resolution / consistency: added a current_user_or_admin private helper (rescues Devise::MissingWarden); handlebars_cache_key resolves the user/admin once and derives user_type, user_id, and app_type_id from it. write_multiple_handlebars_templates now uses the same helper instead of a raw current_user || current_admin.
• Deliberately excluded current_sign_in_at — it would bust the cache on every login; role/access-control and config changes are already captured via their respective updated_at timestamps.

Digest is now: "#{ver}-#{items}-#{app_type_id}-#{user_type}-#{user_id}-#{userrole}-#{uac}".

Non-issue confirmed

• Deletion-based invalidation: configs are retired with disabled: true, which bumps updated_at, so the existing MAX(updated_at) keys already capture retirement. No COUNT/deletion handling needed.

Tests added (all green — 59 examples, 0 failures)

• Per-user divergence: different users, same app_type → different cache key and compiled filename.
• Stability: same user → identical key across separate calls (dedup preserved).
• Cross-user write_handlebars_template: same app_type, different users, different content → separate compiled files (no early-return skip).
• User/Admin id-collision safety: User(id: 42) vs Admin(id: 42) → distinct keys.
• app_type isolation: same user id, different app_type → still distinct (guards against dropping app_type once user id was added).

Notes

• Disk growth stays bounded: HandlebarsPrecompiler.cleanup_public_dir wipes all compiled files on each start/deploy, and individual files now follow the same per-user model as the already-per-user multi-file bundles.
• rubocop clean on both changed files.