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

[coleshirley]

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

• Code style has been fixed by running the composer cs command.
• Changes have been tested to not break existing functionality.
• Documentation is up-to-date.

[danharrin]

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]

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]

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

[danharrin]

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

[danharrin]

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

[coleshirley]

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]

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?