jeremyevans / rodauth

Ruby's Most Advanced Authentication Framework
http://rodauth.jeremyevans.net
MIT License
1.67k stars 95 forks source link

Skip displaying links for disabled routes #269

Closed janko closed 1 year ago

janko commented 1 year ago

I recently added an admin section to the demo Rails app, and I wanted to show the option of enabling the create_account feature, but allowing account creation only through internal requests, by disabling the HTTP route via create_account_route nil. This worked, but I noticed that the login page still displayed the registration link, even though the route was disabled. Since there is no information about the route name anyway, I thought it would make sense to skip displaying it.

I then applied this idea to all other places where links are being displayed. I also updated *_path and *_url methods to return nil for disabled routes.

jeremyevans commented 1 year ago

Thanks for the patch! I agree with the feature in general. However, in terms of the implementation, I think it would be better to filter the links in a single place to remove nil paths, instead of adding/updating conditionals all over the place. What are your thoughts on that?

janko commented 1 year ago

@jeremyevans Makes sense, updated 👍🏻. I wanted to do the filtering in the Rodauth methods itself, rather than in the views, because the links for two_factor_base are used in business logic as well. The only way I saw how to do it was dropping down from auth_cached_method to auth_private_methods.

I saw a benefit in sorting there as well, to get consistent order in /multifactor-{auth,manage,disable} routes, regardless of the order in which features were loaded. However, given that this can technically break backwards compatibility for applications that are currently relying on that order, I can undo that change if needed.

jeremyevans commented 1 year ago

Implementation looks good, thanks! I'll test and merge later today.

jeremyevans commented 1 year ago

Squash merged at 067d83eeffc1f27d2f1c0520da0a8de6c99e49b6