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

[Bug]: Shield Laravel Policy broken #63

Closed
ghsgabriel opened this issue May 29, 2024 · 8 comments
Closed

[Bug]: Shield Laravel Policy broken #63

ghsgabriel opened this issue May 29, 2024 · 8 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@ghsgabriel
Copy link

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!

@ghsgabriel ghsgabriel added the bug Something isn't working label May 29, 2024
@marcogermani87 marcogermani87 self-assigned this May 29, 2024
@marcogermani87
Copy link
Collaborator

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

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

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

malle-pietje commented Jun 6, 2024

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

@marcogermani87 marcogermani87 added the enhancement New feature or request label Jun 6, 2024
@RickDBCN
Copy link
Owner

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

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

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

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?

@RickDBCN RickDBCN closed this as completed Jul 4, 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 enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants