`Hyrax::Ability` treats `::User`s as in groups not in `user.groups`

Descriptive summary

Hyrax::Ability (through Blacklight::AccessControls::Ability) treats users as in groups they aren't otherwise in. specifically, it adds group membership from Ability#default_user_groups to all users, and adds the group "registered" whenever User#new_record? is falsey.

in both cases, these group name strings are hard coded ("public" and "registered"), though they are configurable in Hyrax in general. in the case of "registered", the logic is repeated in a common gem: https://github.com/samvera/hydra-role-management/blob/master/app/models/concerns/hydra/role_management/user_roles.rb#L15

i'm proposing that we change ::User#groups to provide a complete listing of the current groups for the user. in default Hyrax, this means providing a new RoleMapper implementation as ::User.group_service. we can then deprecate the existing Ability behavior, making Ability#user_groups an alias to #User#groups by default.

the details of the proposal are as follows:

now:
1. override Blacklight::AccessControls::Ability#default_user_groups at Hyrax::Ability to use configured "public" group name;
2. implement a new RoleMapper that injects "public" and "restricted"; a. this will mean ::User.groups returns the full group set for users in Hyrax's default behavior;
3. begin logging warnings in Hyrax::Ability#user_groups if that method adds a group; a. something like "treating #{user} as a member of the group '#{added_group_name}' because #{reason}". beginning in Hyrax 4.0.0, Hyrax::Ability#user_groups won't add groups when checking user abilities. applications should ensure User#groups returns the full set of groups for each user.";
4. patch https://github.com/samvera/hydra-role-management/blob/master/app/models/concerns/hydra/role_management/user_roles.rb to support configurable group names for "restricted" and "public";
5. patch https://github.com/samvera/hydra-role-management/blob/master/app/models/concerns/hydra/role_management/user_roles.rb to add "public" to all users by default (and release a major version); or add support for a configurable set of "default roles" for all users (much like Blacklight::AccessControls::Ability#default_user_groups)
in Hyrax 4.0.0:
1. make the default Hyrax::Ability#user_groups implementation current_user.groups; begin ignoring #default_user_groups

Rationale

currently, any code that wants to know what groups a user is a member of needs to know about specific interactions between ::Ability and ::User. this leads to cases of complex, divergent behavior depending on whether we ask a User or an Ability about group membership; e.g. https://github.com/samvera/hyrax/blob/master/app/services/hyrax/collection_types/permissions_service.rb#L214-L219 interactions with gems like hydra-role-management are likewise complicated (as shown by the duplicated injection of "restricted").

moving all the logic of group membership to User#groups will mean there's only one place to check whether a user is in a group. keeping ::Ability#user_groups still provides a hook for Ability specific group logic, if an application needs that.

a variation is to implement the new RoleMapper, continue to use the existing RoleMapper by default until Hyrax 4.0.0, but recommend that applications manually inject the new role mapper and update their code for compatibility.

Related Work

this is coming in part out of work on https://github.com/samvera/hyrax/pull/4685

In your mind is “registered” a group or a role? The distinction we are making in Hyku, based on our understanding of work done by the permissions interest group, is thus:

groups - a set of users with one or more roles role - a set of permissions belonging to one or more users or groups.

“Registered” and “public” are called roles primarily in the code, but accessed as groups in several of the places you mention above. That mixing of language I think really increases confusion and complexity.

In your mind is “registered” a group or a role?

i don't have a specific set of ideas about where we should be going with this in the longer run. at the moment, "registered" and "public" are groups. this view of it is baked in at the Blacklight::AccessControls::Ability level, so a few dependencies down from Hyrax.

The distinction we are making in Hyku, based on our understanding of work done by the permissions interest group

what's the best place for me to engage with this work?

“Registered” and “public” are called roles primarily in the code

can you say more about this? i agree that it would be helpful to clarify this, but this isn't the conclusion i came to in the analysis that lead to this issue. i see "role" terminology in two places that seem relevant for Hyrax's existing feature set:

workflow (sipity) roles (depositing, approving, etc...)
- these determine what workflow actions are available to a user, but don't relate to object ACLs or controller actions at the collection or object level.
Hydra::RoleManagement uses "roles" in all its class, module and db table names, but actually controls user group membership.

what's the best place for me to engage with this work?

I'm hoping @bkiahstroud can provide some links for us. I know he, Bess and Lea Ann would be happy to meet with you about their current status. There are also the docs from the working group found here - https://samvera.atlassian.net/wiki/spaces/samvera/pages/494242070/Hyrax+Permissions+Working+Group+HPWG

can you say more about this? i agree that it would be helpful to clarify this, but this isn't the conclusion i came to in the analysis that lead to this issue. i see "role" terminology in two places that seem relevant for Hyrax's existing feature set

the two locations you mention are primarily what I mean. If we accept the definitions the working group came up with, then then I feel like that the user_groups are actually roles with the wrong name tags on. It seems clear that either Hydra::RoleManagement is misnamed or the user_groups are and I'm using the broader argument to say which one I think should be renamed. This is just my opinion at this point.

a thing to keep in mind is that, at the object level, we have a strictly ACL-based authorization model. these grant :read, :edit and :discover access modes to users and groups. in theory, there's a community commitment to respect these ACL as existing below the application layer as part of an abstract repository model.

when i say that "registered" and "public" are (currently) groups, what i mean is that each existing Hyrax (or blacklight-access-controls) application has existing ACLs that list them as the agent (with the agentClass of "group"). in practice, we do a really good job of respecting these ACL based permission grants where they apply. it's a bit more Wild West when we get to application actions that don't have a clear mapping to the three ACL access modes, or that don't apply to a specific repository (PCDM/Hydra Works) entity. this is where a role-based approach seems useful to me (see also https://github.com/samvera/hyrax/pull/4685/files#diff-3657a135a00c0fc552a054fb17dfaa852b182b90c7b76b5a3926fb11f256a2ffR12-R14). i'd hope that any approach to RBAC would either be carefully layered on the existing ACL structure or include a comprehensive migration of existing ACLs.

with this in mind, it seems to me like the best approach is to regard "role" in Hydra::RoleManagement and RoleMapper as imprecise, and normalize on "group" to refer to the relationship between ::User and ACL access.

samvera / hyrax