tactilenews / 100eyes

Dialogtechnologie für den Pivot to People invented by tactile.news GmbH
MIT License
10 stars 1 forks source link

Fix redirect for users setting up otp #2041

Closed mattwr18 closed 1 month ago

mattwr18 commented 1 month ago

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:

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 from Clearance gem:

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 method:

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 method:

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:

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

def return_to_url
  session[:return_to]
end

which is called by where return_to is:

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 in our otp_setup_controller#create:

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