Make username access configurable and not implicit allowed

Regarding the Disclosure of usernames and user IDs is not considered a weakness policy, it would be worth discussing changing it in a new "[policy] ..."-titled issue and re-title this one to the task at hand, because this one as titled looks like a policy discussion at a glance.

@mxr576 Regarding comment #5 above, are you going to open a policy change issue to discuss?

If Drupal took the opposite stance on user enumeration, we would work through those issues in public (like this issue), and in some release consider it “done”, then thereafter any reports would be security issues that would get Security Advisories. If we don't change the policy, then future unexpected user enumeration bugs would be public.

I'm taking a shot at this. I hid the old MR because it was 4 years old and I didn't feel like updating it.

Made a new MR (https://git.drupalcode.org/project/drupal/-/merge_requests/13617):

• adds a new permission: View usernames
• copies logic from emails #3070293: New permission to view user email (https://www.drupal.org/node/3070293)
• adds tests (had help from Claude for the tests)

In my manual tests this works nicely for JSON:API, hiding usernames unless the new permission is granted.

Note:
Permission to View user information to anons allows them access to example.site/user/2 which could easily expose the username in the theme. This is not addressed in the MR and is not necessarily a core security issue.
Not much I can do about it, themers must check for permissions before rendering the usernames in that case. The work in the MR does help in that direction but doesn't solve that case, it can't.

Please have a look

Just some thoughts in order to make sure we have taken everything API-related into account:

1. The entity API operation to check for labels is "view label". This was previously returning AccessResut::allowed() and using MR!13617 we add our own logic in there.
2. For view label to not fallback to view the entity class handler must set viewLabelOperation to TRUE; something already done by the core user module.
3. The view label permission is used at least by JSON:API module, the EntityAutocomplete element API and the entity_reference_label field formatter.

Though I agree with the rest of the code introduced into the MR, I don't really like the following code:

// Allow for the special anonymous user.
// Normally this user profile can't be seen, but it is exposed in JSON:API
// by default.
if ($entity->isAnonymous()) {
  return AccessResult::allowed();
}

The main issue of this module is that we don't have the username access as a configurable option. By transferring this issue to the anonymous users only, I think we are actually hiding the issue in a subset of affected roles. I would prefer to not have this code and instead only check that the current role has the required permissions.
(EDIT: As @bserem explained to me in Slack, this check is there to make sure that User #0 has the "Anonymous" username listed, so my point above is invalid)

As for the rest of the code:

// Require both 'access user profiles' AND 'view usernames' permissions.
$access_result = AccessResult::allowedIfHasPermission($account, 'access user profiles')
  ->andIf(AccessResult::allowedIfHasPermission($account, 'view usernames'));

Can you explain why we need an AND? Wouldn't OR be more suitable? That way we could even just check if ($account->id() == $entity->id()) {...} and immediately after check for either of the following permissions: administer users, access user profiles or view usernames.

As a last step, just to make sure that nothing breaks in current installations - that's the reason I turn this into Needs work - we could make sure to have a post_update hook to assign the new view usernames permission to all roles.
Besides, using only permission-based checks we can make sure that anyone willing to change the system using the new Access Policy API is able to do so.

What do you think?

Thanks for the feedback @vensires! Much appreciated.

I'm assigning the issue to me for now, but it might be a couple of days before I can jump to it again.
If anyone is willing to get this further by all means, be my guest.

@vensires from a quick overview I believe you got valid points, let me check them in detail and I'll get back to you with improvements.

Improved logic and order of checks, for better code readability. Also fixed CS issue added previously and adapted previous changes for new user registrations.

@vensires the idea behing AND is to do things the same as with emails:
- right now you are not allowed to view the email unless you have BOTH `view user email` AND `access user profiles`

I do not object to the idea of an update hook to give `view user names` to all people, however since this is actually an open security-related issue where usernames are exposed in JSON:API I would prefer not to do so, not yet. Many of the sites that have JSON:API enabled might not know that usernames are exposed. So it would be better for us to apply the fix, rather than apply the fix and at same time undo it by giving anonymous users the permission to see usernames.

Open to discussion of course, as always.

I'll bump this to needs review, mostly because I'd love a fresh set of eyes to check it before I continue working on it again.

Thanks

Thanks for reviving this old issue, I agree this would be important to fix.

If you are using the email_registration module, jsonapi leaks the e-mail address of every user.
And even without email_registration, we noticed a lot of users tend to use their email address or their first name + last name as their username.
This is a GDPR violation and we are getting complaints from clients.

Have not fully reviewed but appears to have pipeline issues.

@mxr576 thank you very much for coming back here and sharing your work on this as a contrib module!!!
It sure is a great place to start or to further decide what we expect core to have as features so that we can see this issue as "fixed".

Your module's code takes into account fields, JSON:API, bubble-ups and even emails.
Maybe we could focus on JSON:API and fields for this core issue?

I made an attempt and fixed the failing PHPUnit test. There are 2 more now but they seem related to gitlab artifacts and not to the changes in code.
I'm trying to understand what this is about.

@bserem thanks for the update on the last day of the year :)
I would also like to know your opinion on @mxr576's work and which of his achieved goals we would like to have in core too.

PS: Happy new year everyone! Best wishes to all of you!

I'm failing to fix this properly, mainly because many tests need to be updated for the new permissions, but that generates a very important question:

Do we update the tests to assume the new permissions are in place or do we update the tests to assume they don't have access to usernames?

I'm gonna stop here and wait for some feedback.

@bserem in order to keep the scope of this issue somewhat manageable, I think we should keep the existing behavior (so I guess grant the permission to everyone at install and in a postupdate hook?).
Being able to remove the permission will already be an improvement for site admins.
And then we can open a followup to discuss changing the default.