search

RickDBCN / filament-email

Log emails in your Filament project
https://filament-email-demo.marcogermani.it/
MIT License
77 stars 23 forks source link

[Bug]: Shield Laravel Policy broken #63

Closed ghsgabriel closed 2 months ago

ghsgabriel commented 3 months ago

What happened?

Hi! It appears that the canAccess function in the resource breaks the integration with Policy. (I don't know if just with Shield or without shield as well).

If I go directly to the vendor and remove this function, everything works fine. Without her, no.

Am I doing something wrong or is it really a bug?

How to reproduce the bug

If I remove the canAccess function, everything works. With it, any user can access it.

Package Version

1.4.5

PHP Version

8.3

Laravel Version

11

Which operating systems does with happen with?

Linux

Notes

Thanks!

marcogermani87 commented 3 months ago

Hi @ghsgabriel, you can provide more detail about the error received? I've tested Filament Shield plugin with our v1.4.5 version and all works.

ghsgabriel commented 3 months ago

Hi @ghsgabriel, you can provide more detail about the error received? I've tested Filament Shield plugin with our v1.4.5 version and all works.

Hi @marcogermani87 , In your test, if you leave a user without any permission to access, removing any permission via shield, can they access?

In my case, I want only users of some specific roles to be able to see these logs, but anyone can see them even if they are limited via shield.

From my tests, it appears that the canAccess function has more priority. When removing this function from the vendor, everything works as expected. People with roles where they have permission assigned in the shield can access and people without permission cannot access.

If you can just confirm in your test that it works this way, then we will be sure that I am doing something wrong.

Anyway, I forked this wonderful project and removed canAccess, so my case is now resolved. I didn't send a pull request because I believe I'm probably doing something wrong, and the canAccess function was implemented for some reason, but if you see that it really is a bug and want my help resolving it, I can try to investigate further.

Thank you for this package, it's very good

marcogermani87 commented 3 months ago

Hi @ghsgabriel , replicate this behavior is simple:

In my case, I want only users of some specific roles to be able to see these logs, but anyone can see them even if they are limited via shield.

In my tests i've installated Filament Shield, create a super_admin role and i've assigned to admin user.

Into config/filament-email.php i've configured the can_access options with this:

    'can_access' => [
        'role' => ['super_admin'],
    ],

After this all working well and only users with "super_admin" role are able to view the e-mail log section.

malle-pietje commented 3 months ago

First of all; love the plugin and we use in almost all of our Filament projects!

If I may add; the permissions based on roles work fine. It would be even nicer if we can fully leverage the capabilities associated with using a policy with Filament Shield: image

Details are explained here: https://github.com/bezhanSalleh/filament-shield?tab=readme-ov-file#policies

With policies we could even restrict a role from resending emails (which could be a good thing).

RickDBCN commented 3 months ago

Hi @malle-pietje, it should already be possible to create a policy for this, as we just create another Filament resource in your application. Could you try to generate a policy and see if it works?

malle-pietje commented 3 months ago

Thanks @RickDBCN , tried it again but I am unable to get it to work with a Policy. The role-based authorisation works fine which is good enough for my applications.

ghsgabriel commented 2 months ago

Hi @marcogermani87

Thanks for the tip.

This package is very good, thank you for maintaining it.

In my case, I want to add and remove the option to see these email logs for various roles in the panel, without having to specify it in the code (just like I do with any other resource).

To adapt the package to my needs, I simply removed the canAccess function and everything started working the way I was imagining (which is being able to manage role permissions via the panel)

As this is a specific case for me, I believe we can close the issue. I'm going to leave it open because I saw that label enhancement was added, so I don't know if you intend to do anything. But for me the issue can be closed.

Thanks

RickDBCN commented 2 months ago

Thank you for the clarification @ghsgabriel.

@marcogermani87, I feel like we should remove the canAccess config option altogether, so users can create their own policy. Any input on this?