Fix: groups and permissions filter redirects

[sammyskills]

Steps to reproduce:

1. Create an admin group (if you don't already have one)
2. Register a new user and add to the group
3. Create two views that can only be accessible by this group, let's call them A and B
4. Filter these views (via the route or filter)
5. Login and visit page A.
6. Change the group of the logged in user, preferrably from the database directly
7. Try to visit page B.

What you'll notice is that the browser dies of too many redirects error. This happens because the filter is redirecting the user back to the previous page, which at this point, the user no longer has enough privileges to access it.

A real-life scenario:
A forum website with multiple groups and permissions, an admin might decide to remove a certain permission from a user or remove the user from a certain group WHILE that user might be logged in. If this user visits another page that requires the same permission or group that s/he has been stripped of, then it causes the browser to die due to too many redirects.

[datamweb]

Thanks for your report. I confirm this bug.

I think that if the user's group is deleted, we should completely logout the user from the system.
something like

if (auth()->loggedIn()) {
  if (!auth()->user()->inGroup(...$arguments)) {
      return redirect()->route('logout');
   }
}

I also think this is the case for filter PermissionFilter, which you should consider.

Edite:
The code below includes both filters (PermissionFilter and GroupFilter).

if (auth()->loggedIn()) {
  if (!$this->isAuthorized($arguments)) {
       return redirect()->route('logout');
  }
}

[sammyskills]

Yes, you are correct @datamweb, I had it in mind to state it as part of my observation, but it completely skipped my mind.

I'll push a commit as per you suggestion.

[sammyskills]

On a second thought, I think the remainder of the code covers for the removed or deleted group(s)/permission(s).

This covers for an authenticated user:

shield/src/Filters/AbstractAuthFilter.php

Lines 32 to 34 in 49887d9

	if (! auth()->loggedIn()) {
	return redirect()->route('login');
	}

This covers for both valid groups and permissions:

shield/src/Filters/AbstractAuthFilter.php

Lines 36 to 38 in 49887d9

	if ($this->isAuthorized($arguments)) {
	return;
	}

And this will present a descriptive information for the user without forcing logout:

shield/src/Filters/AbstractAuthFilter.php

Line 47 in 49887d9

return redirect()->to('/')->with('error', lang('Auth.notEnoughPrivilege'));

Please check again and let me know what you think.

[kenjis]

There were 2 failures:

1) Tests\Authentication\Filters\GroupFilterTest::testFilterIncorrectGroupWithPrevious
Redirect URL `https://example.com/index.php/open-route` does not match `https://example.com/index.php/`
Failed asserting that false is true.

/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Test/TestResponse.php:212
/home/runner/work/shield/shield/tests/Authentication/Filters/GroupFilterTest.php:79

2) Tests\Authentication\Filters\PermissionFilterTest::testFilterIncorrectGroupWithPrevious
Redirect URL `https://example.com/index.php/open-route` does not match `https://example.com/index.php/`
Failed asserting that false is true.

/home/runner/work/shield/shield/vendor/codeigniter4/framework/system/Test/TestResponse.php:212
/home/runner/work/shield/shield/tests/Authentication/Filters/PermissionFilterTest.php:79

https://github.com/codeigniter4/shield/actions/runs/3867406237/jobs/6592163575

[kenjis]

I am not sure if there is a normal use case of accessing a page without permission.
So I don't see a problem with redirecting to /.

[datamweb]

I'm just thinking out loud. Maybe this is not related to PR.

shield/src/Config/Auth.php

Lines 48 to 52 in 49887d9

	public array $redirects = [
	'register' => '/',
	'login' => '/',
	'logout' => 'login',
	];

Why don't we have these two?

    public array $redirects = [
        'register' => '/',
        'login'    => '/',
        'logout'   => 'login',
        'afterPermissionDenied' => '/',
        'afterGroupDenied' => '/',
    ];

[sammyskills]

Why don't we have these two?

Ah @datamweb , I think that will be great. Developers can be able to set a custom URL (path, named route or absolute URL) for each of them.
I also agree with you that it should be another PR. I'll do that when #577 is merged.

[datamweb]

@sammyskills It looks like you are using tag -S to sign each commit. This makes you forget to sign your commits in some cases.

To check further, run the following command in the terminal

git config --list

user.name=Samuel Asor
user.email=your emaillllllllll
user.signingkey=9079CC1BED930E60
commit.gpgsign=false
tag.forcesignannotated=true

If you see commit.gpgsign=false, please run the following command. From now on, all your commits will be signed automatically. And there is no need to use -S.

git config --global commit.gpgsign true

But In this PR, you have two commits that you must use gpg-signing-old-commits (all commit) given below to sign them.

[datamweb]

@kenjis, please clarify, is this implementation what you had in mind?

[kenjis]

I'm okay with this implementation.

[sammyskills]

Hi @datamweb ,

I've tried to follow the instruction on signing all commits, but each time I run the command, a new file opens up in my text editor and the terminal just stops. I've waited for hours, but it is still at the same spot, as if it is expecting an input.
I've checked online but didn't get any headway. Do you have a clearer documentation on the steps to take?

[datamweb]

I run the command

Which command?

a new file opens up in my text editor and the terminal just stops. I've waited for hours,

I saw that it took a while for you to answer. You wait for the terminal, the terminal waits for you 😊.
This is normal, because you have to save the changes in the editor, you can close the editor by pressing the q button and then look at the terminal.

Do you have a clearer documentation on the steps to take?

The easiest way is the following, considering your lack of familiarity with git rebase.

git fetch upstream
git switch develop
git merge upstream/develop
git switch fix/update-groups-permissions-redirects
git branch -m fix/update-groups-permissions-redirects fix/update-groups-permissions-redirects.bk1
git switch -c fix/update-groups-permissions-redirects develop
code .

After that, change src/Filters/AbstractAuthFilter.php and tests/Authentication/Filters/GroupFilterTest.php and tests/Authentication/Filters/PermissionFilterTest.php.

git add .
git commit -m "fix: groups and permissions filter redirects"
git push -f origin fix/update-groups-permissions-redirects

[sammyskills]

Thank you @datamweb.

[datamweb]

@sammyskills thanks!
merge!