Require client visibility to view enrollment in HMIS; update HMIS permission docs

[gigxz]

Merging this PR

• use the squash-merge strategy for PRs targeting main
• use a merge-commit or rebase strategy for PRs targeting staging and production

Description

Issue: https://github.com/open-path/Green-River/issues/9442

Enrollment visibility now requires can_view_clients in addition to project/enrollment-detail permissions, closing a gap where a user without client visibility could still query enrollment(id:) and resolve related records (assessments, services, CLS, SOGI). Limited enrollment summaries on the client dashboard get the same requirement.

Approach: The check is expressed declaratively in Hmis::Role.permissions_with_descriptions (requirements:), so policy objects inherit it for free via HmisPermissionLoader, plus explicit additions to the viewable_by scope chain.

Note on same-role semantics: Enrollment visibility is resolved with with_access(..., mode: :all), so can_view_enrollment_details, can_view_project, and can_view_clients must all be granted on the same role rather than unioned across a user's roles. This is intentional: a role granting enrollment details without client visibility is an incomplete grant, and satisfying it from an unrelated role would make a role's effective access depend on which other roles a user happens to hold. All production environments have been audited, and no existing role grants enrollment visibility in a way that loses access under this change.

Documentation: moves drivers/hmis/doc/PERMISSIONS.md to docs/features/hmis-permissions.md, updates it to mirror format of warehouse-permissions.md; add explanation of how "requirements" work; focus it on helping devs understand how best to check permissions (and what's legacy vs not)

Follow-up work needed

• Enrollment#viewable_by and HmisEnrollmentPolicy#can_view_details? still diverge. viewable_by uses Project.with_access(user, :can_view_enrollment_details, :can_view_project, :can_view_clients, mode: :all) under the hood, which requires that all three permissions exist on the same role. Meanwhile Policies use UserContext#project_permissions which allows all three permissions to exist on different roles, as long as they all apply to the project.
  • I don't like this, and have a proposal for how to align the scope to follow the same logic as the policies. This is definitely out of scope, though, and is a pre-existing issue unrelated to the lack of dependence on can_view_clients.
• Expose requirements in the HMIS Role interface; and prevent a user from saving a Role that grants a permission without it's requirements. This will make things clearer to admins.
• Memoize Role.permissions_with_descriptions and expose a helper required_permissions_for(perm) that can be used by HmisPermissionLoader#apply_permission_requirements. I left this out because it felt like scope creep, but I think it would be a beneficial refactor

Type of change

Bug fix

Checklist before requesting review

• I performed a self-review of my code
• I ran the OP review skill
• I ran the code that is being changed under ideal conditions, and it doesn't fail
• If adding a new endpoint / exposing data in a new way, I have:
  • ensured the API can't leak data from other data sources
  • ensured this does not introduce N+1s
  • ensured permissions and visibility checks are performed in the right places
• Any major architectural changes are supported by an approved ADR (Architectural Decision Record)
• I updated the documentation (or not applicable)
• I added spec tests (or not applicable)
• I provided testing instructions in this PR or the related issue (or not applicable)

[martha]

Looks good to me! Thanks especially for the updated documentation! ✨

[martha]

Should this be moved to docs/features/hmis/hmis-permissions.md?

[martha]

If I understand right, this isn't just true of viewability, and the general concept of direct vs. inherited permission on an entity feels like it might belong in a different section. Close to line 26 at the end of the Core Concepts section, maybe?

[martha]

Curious why this doc stays here instead of getting centralized in docs/features. Is it because this doc is more technical with code examples? If that's true, have we documented this desired documentation structure? :)

[martha]

any reason to not use HmisPermissionSets::ENROLLMENT_VISIBILITY here? I have the same question in a few places in the below specs too.

[martha]

Suggested change

	# as dependencies, so the user must have both at SOME projects (not necessarily the same project).
	# as dependencies, so the user must have all 3 at SOME projects (not necessarily the same project).

[martha]

Suggested change

	let(:enrollment_view_perms) { HmisPermissionSets::ENROLLMENT_VISIBILITY }

Prompted by AI review:

[warning] enrollment_access_spec.rb:239 — the files_viewable_by block was not updated. enrollment_view_perms is still [:can_view_enrollment_details, :can_view_project], but files_viewable_by routes through enrollment_details_viewable_by, which now requires can_view_clients on the same role. Two examples are affected. At :246, "is empty if the user has enrollment access but no file permissions" grants only enrollment_view_perms, so the user no longer has enrollment access either and the assertion holds for the wrong reason. At :252, the p2 access control ("no file perms") is likewise missing enrollment visibility. Both still pass, but neither tests file permissions anymore. Adding :can_view_clients to enrollment_view_perms — or using HmisPermissionSets::ENROLLMENT_VISIBILITY — restores the intended premise.

[martha]

Suggested change

	let!(:cruft_access_control) { create_access_control(user, p6, with_permission: HmisPermissionSets::ENROLLMENT_VISIBILITY) }

prompted by AI review:

[warning] enrollment_access_spec.rb:167 — the "cruft" access control is there specifically to defeat the early return in viewable_by (return none unless global_policy.can_view? || global_policy.can_view_limited?, enrollment.rb:176). It grants only can_view_enrollment_details, which is now stripped from global_permissions for lacking can_view_clients, so can_view? is false and the early return fires. The example passes without ever reaching the scope logic it was written to exercise. Add :can_view_clients to that grant.

[martha]

Here's a sort of amorphous thought that I want to post even though it's not in scope for this PR. Could we create a scope living between the with_access layer and the viewable_by scopes, that would handle permissions requirements consistently under the hood?

Currently,

• with_access - resolves raw permissions, ignoring requirements. So in a way, "with access" is kinda a misnomer, because it doesn't really tell you what the user has actual access to see/do, it just tells you what raw permissions they have from their roles.
• viewable_by- each one needs to remember to pass in all the requirements, even though the requirement information already lives on the role. I guess the point of the current design is that the viewable_by scopes are the nice api that I'm wishing for. But can we centralize it better? -- similar to how, when implementing a new policy, you don't have to restate every permission requirement, it's just handled already at a lower level.

I'm not quite sure, but maybe we are talking about the same thing here?:

Memoize Role.permissions_with_descriptions and expose a helper required_permissions_for(perm) that can be used by HmisPermissionLoader#apply_permission_requirements. I left this out because it felt like scope creep, but I think it would be a beneficial refactor

Here are some notes from AI trying to help me think through this idea and propose a concrete change, although I'm not sure I have totally wrapped my head around the second suggestion in particular. But let me know what you think!

It would be nice if requirement resolution were a property of the scope layer rather than a call-site convention.

The doc as written states the rule as something to remember — "Pass the flattened chain when a scope needs it" — and this PR has to add with_access(user, :can_view_enrollment_details, :can_view_project, :can_view_clients, mode: :all), which is a hand-maintained copy of a chain the system already knows (can_view_enrollment_details's requirements). When the answer to a design question is "remember to do X," it's worth asking whether X can be done once.

Right now the *_viewable_by scopes are the intended answer, and with_enrollment_details_viewable_by here is a good instance of it. But they're opt-in: with_access stays public and is the easiest thing to reach for, and the chain gets re-derived by hand at each call site. Existing callers show the drift that allows:

• Types::Forms::PickListOption (ENROLLABLE_PROJECTS) uses viewable_by(user).with_access(user, :can_enroll_clients), which matches only that one Role column. The policy gating the form (HmisProjectPolicy#can_create_enrollments? → project_permissions.include?(:can_edit_enrollments)) resolves the full chain down to can_view_clients. So the pick list can offer a project whose enrollment form submission is then denied.
• StaffAssignmentPolicy::Global#can_index? calls Project.with_access(user, :can_edit_enrollments) — a policy that skips the requirement resolution the policy layer otherwise promises.
• Client.files_viewable_by and Enrollment.files_viewable_by chain two with_access calls, so can_view_any_*_client_files' can_view_clients requirement is satisfied by whatever role the other call matched. Which is to say: the same-role invariant only holds within a single with_access call, so chaining already mixes the two semantics.

Two levels of fix, and the first seems small:

1. Flatten inside entities_with_permissions. With the Role.required_permissions_for(perm) helper you already listed as a follow-up, with_access(user, :can_view_enrollment_details, mode: :all) could mean what a reader assumes, and both with_enrollment_details_viewable_by and HmisPermissionSets would become unnecessary. The wrinkle is mode: :any: with_access(user, :a, :b, mode: :any) would need to become (a AND a's reqs) OR (b AND b's reqs), a disjunction of conjunctions that Role.with_any_permissions can't express today. But that is the semantics HmisPermissionLoader already implements in Ruby, so we'd mostly be making the SQL agree with it.
2. Have one evaluator. The deeper version of your first follow-up: there are two independent implementations of "what does this user have here" — Role.with_permissions matching columns in SQL, and HmisPermissionLoader building a Set in Ruby. Requirements are just where they first disagree; per-role versus union-across-roles is the second place. Since with_access already plucks ids into where(id: ...), we aren't getting much SQL composability from it, so a scope could instead ask UserContext for the project ids whose project_permissions include a permission, backed by a bulk load over the user's project universe. Most of the machinery exists (HmisProjectAccessGroupLoader, preload_project_dependencies, cached_viewable_project_ids). Then the scope and the policy can't drift, because only one thing resolves permissions.