Skip to content

Commit

Permalink
Fix redirect for users setting up otp (#2041)
Browse files Browse the repository at this point in the history
### 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
mattwr18 authored Sep 26, 2024
1 parent 6c9e43f commit 975a4a9
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 6 deletions.
6 changes: 6 additions & 0 deletions app/controllers/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 15,10 @@ def create
sign_in(user)
end
end

private

def url_after_create
redirect_path
end
end
2 changes: 1 addition & 1 deletion config/initializers/clearance.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 8,7 @@
config.mailer_sender = ENV['EMAIL_FROM_ADDRESS']
config.rotate_csrf_on_sign_in = true
config.same_site = :lax
config.redirect_url = '/organizations'
config.redirect_url = '/sign_in'
config.routes = false
config.sign_in_on_password_reset = false

Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,7 1,7 @@
# frozen_string_literal: true

Rails.application.routes.draw do
root to: redirect('/organizations')
root to: 'sessions#new'

concern :paginatable do
get '(page/:page)', action: :index, on: :collection, as: ''
Expand Down
32 changes: 28 additions & 4 deletions spec/requests/sessions_request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 8,36 @@
let(:otp_enabled) { false }

describe 'GET /sign_in' do
before { get sign_in_path(as: user) }
subject { response }
subject { -> { get sign_in_path(as: user) } }

# This redirect comes from Clearance.configuration.redirect_url and is hard-coded in clearance initializer.
context 'if user is already signed-in' do
it { should redirect_to(organizations_path) }
context 'and belongs to one organization' do
it "redirects to the organization's dashboard" do
subject.call
expect(response).to redirect_to(organization_dashboard_path(organization))
end
end

context 'and belongs to multiple organizations' do
before do
user.organizations << create(:organization)
user.save!
end

it 'redirects to the organizations path to choose the organization' do
subject.call
expect(response).to redirect_to(organizations_path)
end
end

context 'user is an admin' do
let(:user) { create(:user, admin: true) }

it 'redirects to the organizations path to choose the organization' do
subject.call
expect(response).to redirect_to(organizations_path)
end
end
end
end

Expand Down

0 comments on commit 975a4a9

Please sign in to comment.