Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add filter permission and group #535

Merged

Conversation

lonnieezell
Copy link
Member

Supersedes #393

Provides new Groups/Permission filters.

Most of the original work was done by @jlopes90, I just tweaked the filters and added tests.

@kenjis kenjis added enhancement New feature or request stale Pull requests with conflicts labels Nov 21, 2022
@kenjis
Copy link
Member

kenjis commented Nov 23, 2022

@lonnieezell lonnieezell force-pushed the add-filter-permission-and-group branch from c14dcb3 to 714bdd1 Compare November 30, 2022 05:25
docs/authorization.md Outdated Show resolved Hide resolved
@lonnieezell
Copy link
Member Author

I rebased. Though it's giving me Rector fails now. If I fix the Rector issues then it becomes PHPStan issues since it doesn't know what type of variable it is, so I don't know how to please both, here.

public function testFilterIncorrectGroupWithPrevious()
     {
-        /** @var User */
         $user = fake(UserModel::class);
         $user->addGroup('beta');
    ----------- end diff -----------

Applied rules:
 * RemoveNonExistingVarAnnotationRector (https://github.com/phpstan/phpstan/commit/d17e459fd9b45129c5deafe12bca56f30ea5ee99#diff-9f3541876405623b0d1[86](https://github.com/codeigniter4/shield/actions/runs/3580591390/jobs/6022847424#step:7:87)31259763dc1)

vs:

------ ------------------------------------------------------- 
  Line   tests/Authentication/Filters/PermissionFilterTest.php  
 ------ ------------------------------------------------------- 
  :34    Cannot call method addPermission() on array|object.    
  :50    Cannot call method addPermission() on array|object.    
  :65    Cannot call method addPermission() on array|object.    
 ------ ------------------------------------------------------- 

@kenjis kenjis removed the stale Pull requests with conflicts label Nov 30, 2022
@kenjis
Copy link
Member

kenjis commented Nov 30, 2022

Lang items are missing.

1) Tests\Language\TurkishTranslationTest::testAllConfiguredLanguageKeysAreIncluded with data set "tr" ('tr')
Failed asserting that the language key "Auth.notEnoughPrivilege" in the main repository is included for translation in "tr" locale.
Failed asserting that an array is empty.

/home/runner/work/shield/shield/tests/Language/AbstractTranslationTestCase.php:174

2) Tests\Language\ItalianTranslationTest::testAllConfiguredLanguageKeysAreIncluded with data set "it" ('it')
Failed asserting that the language key "Auth.notEnoughPrivilege" in the main repository is included for translation in "it" locale.
Failed asserting that an array is empty.

/home/runner/work/shield/shield/tests/Language/AbstractTranslationTestCase.php:174

https://github.com/codeigniter4/shield/actions/runs/3580771788/jobs/6023181104

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since really all that is different is the actually Auth check (inGroup() versus can()) can we make an abstract filter class that handles all the rest and just processes the arguments in a separate method?

abstract protected function isAuthorized(array $arguments);

This would make a convenient starting point for developers making their own authorization filters as well.

@lonnieezell
Copy link
Member Author

@MGatner Good call. Done.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for me! Maybe @kenjis or @datamweb could take one last look?

// If the previous_url is from this site, then
// we can redirect back to it.
if (strpos(previous_url(), site_url()) === 0) {
return redirect()->back()->with('error', lang('Auth.notEnoughPrivilege'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this PR in a practical way. I did not see a problem.

What bothers me is displaying the error message to the user.
Bonfire2 uses Tatter/Alerts to display errors, but the following code should be used to display errors on the shield:

    <?php if (session('error') !== null) : ?>
        <div class="alert alert-danger" role="alert"><?= session('error') ?></div>
    <?php elseif (session('errors') !== null) : ?>
        <div class="alert alert-danger" role="alert">
            <?php if (is_array(session('errors'))) : ?>
                <?php foreach (session('errors') as $error) : ?>
                    <?= $error ?>
                    <br>
                <?php endforeach ?>
            <?php else : ?>
                <?= session('errors') ?>
            <?php endif ?>
        </div>
    <?php endif ?>

In fact, I'm not sure how Shield users should know what to do to display error message (Auth.notEnoughPrivilege). Perhaps the need to add an explanation in the documentation or something else.

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all.

@lonnieezell lonnieezell reopened this Dec 13, 2022
@lonnieezell lonnieezell merged commit eeebfc8 into codeigniter4:develop Dec 13, 2022
@lonnieezell lonnieezell deleted the add-filter-permission-and-group branch December 13, 2022 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants