Skip to content

Conversation

@jlopes90
Copy link
Contributor

@jlopes90 jlopes90 commented Jul 2, 2022

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

@datamweb
Copy link
Collaborator

datamweb commented Jul 2, 2022

@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

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Thanks for this! It was definitely something I overlooked when porting it from Myth:Auth. We do need a few things to get it ready, though.

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.

They also need to be added to src/Config/Registrar.php so they are auto-registered.

Please give a try and getting some tests in place for these. You can look at tests/Authentication/Filters files for examples that should give you a good start on how to do them.

@MGatner
Copy link
Member

MGatner commented Jul 4, 2022

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

@jlopes90
Copy link
Contributor Author

jlopes90 commented Jul 4, 2022

<?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
Copy link
Contributor Author

jlopes90 commented Jul 4, 2022

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

@kenjis
Copy link
Member

kenjis commented Jul 11, 2022

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?

@datamweb
Copy link
Collaborator

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

@MGatner
Copy link
Member

MGatner commented Jul 11, 2022

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.

@kenjis kenjis added the enhancement New feature or request label Jul 24, 2022
@jlopes90
Copy link
Contributor Author

jlopes90 commented Aug 7, 2022

<?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?

@MGatner
Copy link
Member

MGatner commented Aug 8, 2022

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.

@jlopes90
Copy link
Contributor Author

jlopes90 commented Aug 8, 2022

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.'

@datamweb
Copy link
Collaborator

datamweb commented Aug 9, 2022

@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,
            ],
        ];
    }

@jlopes90
Copy link
Contributor Author

jlopes90 commented Aug 9, 2022

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');

@datamweb
Copy link
Collaborator

datamweb commented Aug 9, 2022

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
Copy link
Collaborator

datamweb commented Aug 9, 2022

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.

@jlopes90
Copy link
Contributor Author

jlopes90 commented Aug 9, 2022

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

shield/docs/install.md

Lines 157 to 176 in bc85d79

## Controller Filters
Shield provides 4 [Controller Filters](https://codeigniter.com/user_guide/incoming/filters.html) you can
use to protect your routes, `session`, `tokens`, and `chained`. The first two cover the `Session` and
`AccessTokens` authenticators, respectively. The `chained` 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. The fourth, `auth-rates`,
provides a good basis for rate limiting of auth-related routes.
These filters are already loaded for you by the registrar class located at `src/Config/Registrar.php`.
```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,
];
```


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,
];

Comment on lines 18 to 25
* Do whatever processing this filter needs to do.
* By default it should not return anything during
* normal execution. However, when an abnormal state
* is found, it should return an instance of
* CodeIgniter\HTTP\Response. If it does, script
* execution will end and that Response will be
* sent back to the client, allowing for error pages,
* redirects, etc.
Copy link
Member

Choose a reason for hiding this comment

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

Please update the doc comment.

@kenjis
Copy link
Member

kenjis commented Aug 13, 2022

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
Copy link
Member

kenjis commented Aug 13, 2022

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

@kenjis kenjis added the tests needed Pull requests that need tests label Aug 17, 2022
@jlopes90
Copy link
Contributor Author

I have no idea how to resolve the PHPCSFixer error.

@datamweb
Copy link
Collaborator

datamweb commented Aug 17, 2022

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

@kenjis
Copy link
Member

kenjis commented Aug 17, 2022

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

@jlopes90
Copy link
Contributor Author

LoginTest is not part of here, so I commit?

@kenjis
Copy link
Member

kenjis commented Aug 19, 2022

LoginTest is not part of here, so I commit?

I sent a PR #390

@kenjis
Copy link
Member

kenjis commented Aug 19, 2022

@jlopes90
Copy link
Contributor Author

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

@kenjis
Copy link
Member

kenjis commented Aug 19, 2022

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

@jlopes90
Copy link
Contributor Author

and now? make new PR?

@kenjis
Copy link
Member

kenjis commented Aug 19, 2022

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

@jlopes90 jlopes90 merged commit 3dd82a1 into codeigniter4:develop Aug 19, 2022
@jlopes90
Copy link
Contributor Author

Sorry, I can't close pull request

@jlopes90
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request tests needed Pull requests that need tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants