Authenticated non-member users can no longer find closed groups [#3279787]

Background

When testing our upgrade to Open Social 11, we noticed that authenticated non-member users can no longer find closed groups, which breaks the intent described in the documentation:

A non-group member is only allowed to read the about page, the group manager, and the members of the group

This is no longer possible as logged-in users can't see closed group results in Group-based views.

Analysis

In tracking this down, I stumbled across a Social Group function that alters Views query results, which was recently added as part of work done for #3260572: 'All group' overview shows groups I don't have access to.. The entire change can be seen in Commit a35157d9.

After taking a closer look at everything, it's clear that there was a mismatch between the original intent of that issue and the way it was fixed. The issue author was complaining that anonymous users could see closed groups, but the developer made changes so that logged-in users couldn't see closed groups. So that means:

Anonymous users can *still* see closed groups (if that was actually a confirmed problem), and now
Logged in users cannot, when they should be able to.

So the wrong problem was fixed.

Proposed solutions

Revert the commit: This is the simplest solution as it doesn't appear to do anything else (other than fix formatting), but it could still be that there's a bug in the original code allowing anonymous users to see things they shouldn't. I haven't tested this, and don't intent to, because my site doesn't let anonymous users see anything anyway (so no a problem for my use case).
Keep the code as it is, but reverse the condition:

if ($account->isAuthenticated()) {

if (!$account->isAuthenticated()) {

This should work fine as long as there's no code in there that requires users to be logged in.

Implementation

I'll leave the implementation to the maintainers as I'm not sure which option is the best, other than posting a revert patch here for Composer builds to pick up. (Sorry to do it here, but I have no idea if `composer-patches` works with GitHub, and don't want to spend time dealing with multiple upstreams when I may be on the wrong track. I'm on a tight budget here, and just need something quick for now to get this working until y'all find a more permanent solution. I'll open PRs when it makes more sense.)

Comment	File	Size	Author
#2	social-auth_users_cannot_see_closed_groups-3279787-2.patch	1.11 KB	colan

Comments

Comment #1

10 May 2022 at 20:38

colan created an issue. See original summary.

Comment #2

colan

he/him

English

Toronto 🇨🇦

commented 10 May 2022 at 21:03

Issue summary:

View changes

Status	File	Size
new	social-auth_users_cannot_see_closed_groups-3279787-2.patch	1.11 KB

git revert didn't work for me as there were too many other changes conflicting with the formatting fixes, but reversing the condition did the trick.

Note: This was not tested anonymously.

Comment #3

colan

he/him

English

Toronto 🇨🇦

commented 10 May 2022 at 21:05

Issue summary:

View changes

Comment #4

colan

he/him

English

Toronto 🇨🇦

commented 10 May 2022 at 21:06

Issue summary:

View changes

Comment #5

colan

he/him

English

Toronto 🇨🇦

commented 17 May 2022 at 16:16

Status:

Active

» Needs review

Updating state so that this issue gets some eyeballs.

Comment #6

colan

he/him

English

Toronto 🇨🇦

commented 17 May 2022 at 16:31

PR created at https://github.com/goalgorilla/open_social/pull/2932 .

Comment #7

tbsiqueira

He/him

commented 19 May 2022 at 06:47

Hi @colan, thank you very much for your PR, great catch!

Indeed checking for if ($account->isAuthenticated()) creates a bug. After some investigation I think that we can change a bit the approach on social_group.module to something like this:

  $account = \Drupal::currentUser()->getAccount();

  if ($account->isAnonymous()) {
    // Get all hidden groups.
    $hidden_groups = \Drupal::entityTypeManager()
      ->getStorage('group')
      ->loadByProperties([
        'type' => 'closed_group',
      ]);

    // Remove all closed groups from /all-groups page.
    $ids = array_keys($hidden_groups);
    if ($ids) {
      /** @var \Drupal\views\Plugin\views\query\Sql $query */
      $query->addWhere('group_membership', 'groups_field_data.id', $ids, 'NOT IN');
    }
  }

We still need to check and remove closed_groups from anonymous users on that view ;) and reverting the entire commit will revert part of a bigger fix.

This should do the trick.

Can you please adjust your PR and let us know if this works as intended?

Kind regards,