shield icon indicating copy to clipboard operation
shield copied to clipboard

feat: add filter permission and group

Open jlopes90 opened this issue 3 years ago • 17 comments

  • [x] add filter group
  • [x] add filter permission
  • [x] update languages
  • [ ] add tests (I don't have testing experience)
  • [ ] add docs

jlopes90 avatar Jul 02 '22 12:07 jlopes90

@jlopes90 Like you, I am a beginner in test writing. However, I have tried hard to learn it. Using tests over time will help you identify errors and problems easily. If you are interested, writing a test is not difficult. Here is some useful information about learning phpunittest. https://forum.codeigniter.com/showthread.php?tid=81830&pid=396218#pid396218

datamweb avatar Jul 02 '22 19:07 datamweb

A good start, and some good feedback from the team! Thank you for taking this on @jlopes90 - let us know how we can help.

MGatner avatar Jul 04 '22 10:07 MGatner

<?php

namespace CodeIgniter\Shield\Authorization;

use Exception;

class AuthorizationException extends Exception
{
    protected $code = 401;

    public static function forUnauthorizedGroup(): self
    {
        return new self(lang('Auth.unauthorizedGroup'));
    }

    public static function forUnknownGroup(string $group): self
    {
        return new self(lang('Auth.unknownGroup', [$group]));
    }

    public static function forUnauthorizedPermission(): self
    {
        return new self(lang('Auth.unauthorizedPermission'));
    }

    public static function forUnknownPermission(string $permission): self
    {
        return new self(lang('Auth.unknownPermission', [$permission]));
    }
}

'unauthorizedGroup'      => 'You do not have sufficient groups to access that page.',
'unauthorizedPermission' => 'You do not have sufficient permissions to access that page.',

What about?

jlopes90 avatar Jul 04 '22 12:07 jlopes90

I'm not good at documentation or explanation in english.

jlopes90 avatar Jul 04 '22 12:07 jlopes90

This is from Myth:Auth.

    'notEnoughPrivilege'        => 'You do not have sufficient permissions to access that page.',

Do we need to change the error message for different filters?

kenjis avatar Jul 11 '22 08:07 kenjis

Personal opinion, You do not have the necessary permission to perform the desired operation.

datamweb avatar Jul 11 '22 09:07 datamweb

I always prefer a very generic authorization error message; it can be used more globally and it doesn't reveal any information to probe attacks.

MGatner avatar Jul 11 '22 11:07 MGatner

<?php

namespace CodeIgniter\Shield\Authorization;

use Exception;

class AuthorizationException extends Exception
{
    protected $code = 401;

    public static function forUnknownGroup(string $group): self
    {
        return new self(lang('Auth.unknownGroup', [$group]));
    }

    public static function forUnknownPermission(string $permission): self
    {
        return new self(lang('Auth.unknownPermission', [$permission]));
    }

    public static function forUnauthorized(): self
    {
        return new self(lang('Auth.notEnoughPrivilege'));
    }
}

'notEnoughPrivilege' => 'You do not have sufficient permissions to access that page.',

What about?

jlopes90 avatar Aug 07 '22 18:08 jlopes90

Please go with @datamweb's suggestion, then this phrase can be reused for any authorization fault without belying the actual attempt:

You do not have the necessary permission to perform the desired operation.

MGatner avatar Aug 08 '22 10:08 MGatner

Please go with @datamweb's suggestion, then this phrase can be reused for any authorization fault without belying the actual attempt:

You do not have the necessary permission to perform the desired operation.

'notEnoughPrivilege' => 'You do not have the necessary permission to perform the desired operation.'

jlopes90 avatar Aug 08 '22 11:08 jlopes90

@jlopes90 I know how you feel, things are a bit complicated, let's continue together, we will call our friends whenever necessary.

  1. Proceed as follows in file src/Config/Registrar.php :
use CodeIgniter\Shield\Filters\GroupFilter ;
use CodeIgniter\Shield\Filters\PermissionFilter;

    /**
     * Registers the Shield filters.
     */
    public static function Filters(): array
    {
        return [
            'aliases' => [
                'session'    => SessionAuth::class,
                   ...
                'group-filter'    => GroupFilter::class,
                'permission-filter'    => PermissionFilter::class,
            ],
        ];
    }

datamweb avatar Aug 09 '22 08:08 datamweb

src/Config/Registrar.php

I didn't even notice about "registar", it was very helpful. Thanks.


The group-filter or better just group?

$routes->get('/', 'Home::index', ['filter' => 'group-filter:home.view');

or

$routes->get('/', 'Home::index', ['filter' => 'group:home.view');

jlopes90 avatar Aug 09 '22 10:08 jlopes90

The group-filter or better just group?

I consider group a common expression and I don't like it. However, yes group-filter is not interesting either.

Allow other members to decide on this matter.

datamweb avatar Aug 09 '22 11:08 datamweb

We need docs for this. At the very least, install.md needs updated to include the new filters. Having instructions added to authorization.md in the appropriate section under Authorizing User is needed also.

I'm not good at documentation or explanation in english.

Step 2: Writing documents. @jlopes90 , But I understood your comment well.

In fact, since you are not an English speaker, this is a bonus. Because you can express things in simple terms. Don't forget that you are the better person to write the documentation because you know what happened in these PR.

Well, first try to write down everything you need to say in your own language somewhere. After the content is finalized, start translating in any way you are comfortable with. After that I will check here whether it is understandable or not.

Finally, it is reviewed by the members.

So just do it. I am waiting.

datamweb avatar Aug 09 '22 11:08 datamweb

We need docs for this. At the very least, install.md needs updated to include the new filters.

https://github.com/codeigniter4/shield/blob/bc85d79599ec7150f9bcad45d3622179919afe74/docs/install.md?plain=1#L157-L176


Controller Filters

The Controller Filters you can use to protect your routes the shield provides are:

Filters Description
session and tokens The Session and AccessTokens authenticators, respectively.
chained The filter will check both authenticators in sequence to see if the user is logged in through either of authenticators, allowing a single API endpoint to work for both an SPA using session auth, and a mobile app using access tokens.
auth-rates Provides a good basis for rate limiting of auth-related routes.
group-filter ??
permission-filter ??

These filters are already loaded for you by the registrar class located at src/Config/Registrar.php.

public $aliases = [
    // ...
    'session'           => \CodeIgniter\Shield\Filters\SessionAuth::class,
    'tokens'            => \CodeIgniter\Shield\Filters\TokenAuth::class,
    'chain'             => \CodeIgniter\Shield\Filters\ChainAuth::class,
    'auth-rates'        => \CodeIgniter\Shield\Filters\AuthRates::class,
    'group-filter'      => \CodeIgniter\Shield\Filters\GroupFilter::class,
    'permission-filter' => \CodeIgniter\Shield\Filters\PermissionFilter::class,
];

jlopes90 avatar Aug 09 '22 15:08 jlopes90

I consider group a common expression and I don't like it. However, yes group-filter is not interesting either.

These are aliases for filters. All of them are filters. So -filter is not needed. Shorter is preferable.

kenjis avatar Aug 13 '22 08:08 kenjis

@jlopes90 The docs seems good. Why don't you proceed?

kenjis avatar Aug 13 '22 08:08 kenjis

I have no idea how to resolve the PHPCSFixer error.

jlopes90 avatar Aug 17 '22 14:08 jlopes90

I have no idea how to resolve the PHPCSFixer error.

@jlopes90 We finally decided to use declare_strict_types, please see PR #379 .

And see https://github.com/codeigniter4/shield/runs/7880373539?check_suite_focus=true

datamweb avatar Aug 17 '22 15:08 datamweb

@jlopes90 Very easy. Just run composer cs-fix. See also https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#php-style

kenjis avatar Aug 17 '22 21:08 kenjis

LoginTest is not part of here, so I commit?

jlopes90 avatar Aug 17 '22 21:08 jlopes90

LoginTest is not part of here, so I commit?

I sent a PR #390

kenjis avatar Aug 19 '22 04:08 kenjis

LoginTest was fixed.

Please rebase. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

kenjis avatar Aug 19 '22 09:08 kenjis

I did git rebase and then git pull and it was like this?

jlopes90 avatar Aug 19 '22 11:08 jlopes90

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch We never use git pull. It is harmful.

kenjis avatar Aug 19 '22 11:08 kenjis

and now? make new PR?

jlopes90 avatar Aug 19 '22 11:08 jlopes90

Cherry pick the commits that are needed and create a new PR.

kenjis avatar Aug 19 '22 11:08 kenjis

Sorry, I can't close pull request

jlopes90 avatar Aug 19 '22 12:08 jlopes90

I did wrong again, I'm going to recover and do new PR

jlopes90 avatar Aug 19 '22 12:08 jlopes90