search

filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
19.4k stars 2.97k forks source link

Add warning to documentation about checking for authenticated users in `shouldRegisterNavigation` and `canAccess` methods #14609

Open coleshirley opened 1 month ago

coleshirley commented 1 month ago

Description

See readme for https://github.com/coleshirley/filament-logout-bug and this filament discord report: https://discord.com/channels/883083792112300104/1272884452254810163

Visual changes

none

Functional changes

danharrin commented 4 weeks ago

Instead, is it feasible to avoid calling the shouldRegisterNavigation() (which I guess is the method calling canAccess()) if there isn't a user? What is the reason it is currently called?

coleshirley commented 4 weeks ago

It's currently called in by Filament::getUrl() in the LogoutResponse::toResponse() method. I don't know if there is a good way to avoid calling either method because getUrl has to build the whole navigation in order to determine what the "first" page of the panel is to redirect the user to on logout

Since there is always a chance that the page that defines canAccess is utilized on a guest accessible panel I don't think there is a way to avoid calling the methods without introducing breaking changes

danharrin commented 4 weeks ago

Maybe its best to always redirect to the login page instead of the panel URL? Wdyt should be the default behaviour on logout?

danharrin commented 4 weeks ago

Ah, I see we only actually call getUrl() if the panel doesn't have a login page

danharrin commented 4 weeks ago

I have pushed up some changes to the logic, what do you think? If you like them, we can revert the docs changes

coleshirley commented 4 weeks ago

That definitely solves the issue with the LogoutResponse. The only reason the warning might still be useful is for Guest accessible panels. Since you can still define canAccess on pages that are used in guest panels it might be worth keeping the warning.

The only other note is the redirect logic here is a minor breaking change in situations where someone has defined a "first" page that is different from the base path of the panel. (i.e. the first page in the navigation is /app/other-page and the second page is /app. Before this logic change if you logged out you would be redirected to /app/other-page instead of /app. I updated the demo repo with this situation

danharrin commented 4 weeks ago

For guest access, would visiting /app, if the panel didn't have the Authenticate middleware, hit the RedirectToHomeController, which would then redirect to the first nav item anyway?