Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix redirect for users setting up otp (#2041)
### What changed in this PR and Why This took a bit of time to track down. I'll document it here. This starts because we have a: ```rb root to: redirect('/organizations') ``` so when someone visits `/`, they are redirected to `/organizations` and `oranizations#index`. This is a protected route and hits the before_filter [require_login](https://github.com/thoughtbot/clearance/blob/v2.6.1/lib/clearance/authorization.rb#L20-L24) from Clearance gem: ```rb def require_login unless signed_in? deny_access(I18n.t("flashes.failure_when_not_signed_in")) end end ``` since the user is not yet `signed_in?`, it hits the [deny_access](https://github.com/thoughtbot/clearance/blob/v2.6.1/lib/clearance/authorization.rb#L43-L48) method: ```rb def deny_access(flash_message = nil) respond_to do |format| format.any(:js, :json, :xml) { head :unauthorized } format.any { redirect_request(flash_message) } end end ``` this then hits [redirect_request](https://github.com/thoughtbot/clearance/blob/v2.6.1/lib/clearance/authorization.rb#L53-L65) method: ```rb def redirect_request(flash_message) store_location if flash_message flash[:alert] = flash_message end if signed_in? redirect_to url_after_denied_access_when_signed_in else redirect_to url_after_denied_access_when_signed_out end end ``` this hits the store_location method: ```rb def store_location if request.get? session[:return_to] = request.original_fullpath end end ``` this stores the request's original fullpath to a session variable, which is then used by [return_to_url](https://github.com/thoughtbot/clearance/blob/v2.6.1/lib/clearance/authorization.rb#L100-L102) ```rb def return_to_url session[:return_to] end ``` which is called by where [return_to](https://github.com/thoughtbot/clearance/blob/v2.6.1/lib/clearance/authorization.rb#L86-L92) is: ```rb def return_to if return_to_url uri = URI.parse(return_to_url) path = path_without_leading_slashes(uri) "#{path}?#{uri.query}".chomp("?") "##{uri.fragment}".chomp("#") end end ``` which is called by [redirect_back_or](https://github.com/thoughtbot/clearance/blob/v2.6.1/lib/clearance/authorization.rb#L80-L83) in our [otp_setup_controller#create](https://github.com/tactilenews/100eyes/blob/main/app/controllers/otp_setup_controller.rb#L17): ```rb def redirect_back_or(default, **options) redirect_to(return_to || default, **options) clear_return_to end ``` We could simply change the `redirect_back_or` to `redirect_to`, we would lose the feature of redirecting back to a route the user might have been trying to view. Also, I think we should avoid having a protected route as the root of the application. It might be a good user experience to root to the organization's dashboard, but having these dynamic application level root is not simple. I've updated the `Clearance.configuration.redirect_url` to also not point to a protected route. It is used in the following locations: ``` - `passwords#url_after_update` - `sessions#url_after_create` - `sessions#url_for_signed_in_users` - `users#url_after_create` - `application#url_after_denied_access_when_signed_in` ``` this attempts to redirect a user resetting their password to the organizations route, which then redirects to sign_in since it is protected. it causes another bug when someone visits `/sign_in` who is already logged in, they hit the `/organizations` path even if they would otherwise never see this page. we do not currently allow new users to sign up, as it's not a public site. the last one is not so easy to understand. Looking at the usage of the redirect_url in clearance, I think it is mostly that we want the user to be redirected to the sign_in path. The lone exception, I have overridden the redirect_url to our own implementation for where a signed in user should be redirected. Fixes #2040
- Loading branch information