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

Unhandled Exception on CRUD::setAccessCondition() #590

Closed
MarioVillani opened this issue Jul 27, 2024 · 7 comments
Closed

Unhandled Exception on CRUD::setAccessCondition() #590

MarioVillani opened this issue Jul 27, 2024 · 7 comments
Assignees
Labels
bug Something isn't working Priority: SHOULD Size: XS 1 hour

Comments

@MarioVillani
Copy link
Contributor

MarioVillani commented Jul 27, 2024

class CompanyCrudController extends CrudController
{
    use \Backpack\CRUD\app\Http\Controllers\Operations\ListOperation;
    use \Backpack\CRUD\app\Http\Controllers\Operations\CreateOperation;
    use \Backpack\CRUD\app\Http\Controllers\Operations\UpdateOperation;
    use \Backpack\CRUD\app\Http\Controllers\Operations\DeleteOperation { destroy as traitDestroy;
    }
    use \Backpack\CRUD\app\Http\Controllers\Operations\ShowOperation;
    use \Backpack\ReviseOperation\ReviseOperation;

    protected $setupDetailsRowRoute = false;
    /**
     * Configure the CrudPanel object. Apply settings to all operations.
     * 
     * @return void
     */
    public function setup()
    {
        // Remove all permissions
        $this->crud->denyAccess(['list', 'create', 'update', 'delete', 'show', 'revise']);

        CRUD::setAccessCondition(['show', 'update'], function ($entry) {
            if(isset($entry)) {
                if (backpack_user()->hasRole('Reseller'))
                    return ($entry->created_by === backpack_user()->id) || ($entry->created_by_company === backpack_user()->id_company);
            }
            return false;
        });

If a reseller tries to open up a Company entity without having permissions, an exception is thrown and logged in laravel.log:

Accesso non autorizzato - non hai i permessi necessari per vedere questa pagina. {"userId":32,"exception":"[object] (Backpack\\CRUD\\app\\Exceptions\\AccessDeniedException(code: 0): Accesso non autorizzato - non hai i permessi necessari per vedere questa pagina. at /myapp/vendor/backpack/crud/src/app/Library/CrudPanel/Traits/Access.php:88)

Docs do not mention that exceptions are thrown by CRUD::setAccessCondition()(related section: https://backpackforlaravel.com/docs/6.x/crud-operations#handling-access-to-operations).
I think it would be better to show how to correctly handle that. Thank you in advance!

@MarioVillani MarioVillani changed the title Unhandled Exception Unhandled Exception on CRUD::setAccessCondition() Jul 27, 2024
@karandatwani92 karandatwani92 added Priority: SHOULD Size: XS 1 hour bug Something isn't working labels Jul 27, 2024
@karandatwani92
Copy link
Contributor

Hey @pxpm

An exception is being logged whenever the user doesn't have access. It is not only limited to setAccessCondition. Crud::denyAccess does the same.

I don't think that is intentional; even if it is, it should be a warning or info, not an error.

@pxpm
Copy link
Contributor

pxpm commented Jul 28, 2024

Hey @pxpm

An exception is being logged whenever the user doesn't have access. It is not only limited to setAccessCondition. Crud::denyAccess does the same.

I don't think that is intentional; even if it is, it should be a warning or info, not an error.

It throws a http unauthorized exception with forbidden (403) status: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403

Similar if you navigate to a non-existent page you get a 404.

If you think it's worth adding to the docs a description of what we talked here, go ahead and add a note in the docs about denyAccess throwing a 403 http error forbidden.

Cheers

@pxpm pxpm assigned karandatwani92 and unassigned pxpm Jul 29, 2024
@karandatwani92
Copy link
Contributor

Hey @pxpm
IDK if we are on the same page. I mean, this logs an error for each attempt, and we both assume it should not be saved in the log file:
Screenshot 2024-08-03 at 4 28 23 PM

@karandatwani92 karandatwani92 assigned pxpm and unassigned karandatwani92 Aug 3, 2024
@pxpm
Copy link
Contributor

pxpm commented Aug 5, 2024

@karandatwani92 that depends on your log level settings I guess.
In Backpack we are not doing Log::error() or something similar. Like I told you, you are trying to access a resource on server, say user/1/edit and you don't have access to that resource, we throw an http 403 exception. (abort(403)).

Maybe you are looking to don't report some exceptions? https://laravel.com/docs/11.x/errors#ignoring-exceptions-by-type

@munjaldevelopment
Copy link

munjaldevelopment commented Aug 5, 2024

Hey @pxpm IDK if we are on the same page. I mean, this logs an error for each attempt, and we both assume it should not be saved in the log file: Screenshot 2024-08-03 at 4 28 23 PM

Hi @karandatwani92, This is functionality of "Backpack" I guess. In log once any page is 403 forbidden for our reference,,

Please check this error on one of our LIVE project.
image

@pxpm
Copy link
Contributor

pxpm commented Aug 5, 2024

Hey @pxpm IDK if we are on the same page. I mean, this logs an error for each attempt, and we both assume it should not be saved in the log file: Screenshot 2024-08-03 at 4 28 23 PM

Hi @karandatwani92, This is functionality of "Backpack" I guess. In log once any page is 403 forbidden for our reference,,

Please check this error on one of our LIVE project. image

Hey @munjaldevelopment . We use abort(403). Cheers

@pxpm
Copy link
Contributor

pxpm commented Aug 30, 2024

I got back here with a fresh perspective, and I think I got what you guys where trying to say.

I did some digging and I found that Laravel itself already excludes some status codes from being reported like 419 for eg. In that regard I think we can make our exception extend one that Laravel already don't report on, so it will not be logged like before.

The @munjaldevelopment screenshot was very helpful, if it had some description of where I should look for it would have helped more from the start, but it is what it is.

Thanks for the report and for sticking with us 🙏

I will tag a new version later today that will have Laravel-Backpack/CRUD#5642 merged.

Cheers

@pxpm pxpm closed this as completed Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Priority: SHOULD Size: XS 1 hour
Projects
Status: Done
Development

No branches or pull requests

4 participants