Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use UUIDs as indentifiers for sessions in public #176

Merged
merged 4 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,21 @@

# Unreleased

### Changed

Sessions are now referenced publicly by a random UUID instead of their primary key.

This needs a manual database migration like so:

```ruby
class AddIndentifierToPasswordlessSessions < ActiveRecord::Migration[7.1]
def change
add_column(:passwordless_sessions, :identifier, :string)
add_index(:passwordless_sessions, :identifier, unique: true)
end
end
```

### Added

- Add default flash notice for sign out (#178)
Expand Down
14 changes: 5 additions & 9 deletions app/controllers/passwordless/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def create
end

redirect_to(
url_for(id: @session.id, action: "show"),
url_for(id: @session.identifier, action: "show"),
flash: {notice: I18n.t("passwordless.sessions.create.email_sent")}
)
else
Expand All @@ -54,7 +54,7 @@ def create
# Shows the form for confirming a Session record.
# renders sessions/show.html.erb.
def show
@session = find_session
@session = passwordless_session
end

# patch "/:resource/sign_in/:id"
Expand All @@ -66,7 +66,7 @@ def show
# @see ControllerHelpers#sign_in
# @see ControllerHelpers#save_passwordless_redirect_location!
def update
@session = find_session
@session = passwordless_session

artificially_slow_down_brute_force_attacks(passwordless_session_params[:token])

Expand All @@ -86,7 +86,7 @@ def confirm
# safe. We don't want to sign in the user in that case.
return head(:ok) if request.head?

@session = find_session
@session = passwordless_session

artificially_slow_down_brute_force_attacks(params[:token])

Expand Down Expand Up @@ -166,10 +166,6 @@ def authenticatable_class
authenticatable_type.constantize
end

def find_session
Session.find_by!(id: params[:id], authenticatable_type: authenticatable_type)
end

def find_authenticatable
email = passwordless_session_params[email_field].downcase.strip

Expand Down Expand Up @@ -201,7 +197,7 @@ def redirect_to_options

def passwordless_session
@passwordless_session ||= Session.find_by!(
id: params[:id],
identifier: params[:id],
authenticatable_type: authenticatable_type
)
end
Expand Down
2 changes: 1 addition & 1 deletion app/mailers/passwordless/mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def sign_in(session, token = nil)
{
controller: "passwordless/sessions",
action: "confirm",
id: session.id,
id: session.identifier,
token: token,
authenticatable: "user",
resource: "users"
Expand Down
5 changes: 5 additions & 0 deletions app/models/passwordless/session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,18 @@ def available?
!expired?
end

def to_param
identifier
end

private

def token_digest_available?(token_digest)
Session.available.where(token_digest: token_digest).none?
end

def set_defaults
self.identifier = SecureRandom.uuid
self.expires_at ||= Passwordless.config.expires_at.call
self.timeout_at ||= Passwordless.config.timeout_at.call

Expand Down
1 change: 1 addition & 0 deletions db/migrate/20171104221735_create_passwordless_sessions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ def change
t.datetime(:expires_at, null: false)
t.datetime(:claimed_at)
t.string(:token_digest, null: false)
t.string(:identifier, null: false, index: {unique: true}, length: 36)

t.timestamps
end
Expand Down
2 changes: 1 addition & 1 deletion docs/upgrading_to_1_0.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ $ bin/rails g migration UpgradePassswordless
```

```ruby
class UpgradePassswordless < ActiveRecord::Migration[7.0]
class UpgradePasswordless < ActiveRecord::Migration[7.0]
def change
# Encrypted tokens
add_column(:passwordless_sessions, :token_digest, :string)
Expand Down
16 changes: 8 additions & 8 deletions test/controllers/passwordless/sessions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def create_pwless_session(attrs = {})
assert_equal 1, ActionMailer::Base.deliveries.size

follow_redirect!
assert_equal "/users/sign_in/#{Session.last!.id}", path
assert_equal "/users/sign_in/#{Session.last!.identifier}", path
end

test("POST /:passwordless_for/sign_in -> SUCCESS / malformed email") do
Expand All @@ -40,7 +40,7 @@ def create_pwless_session(attrs = {})
assert_equal 1, ActionMailer::Base.deliveries.size

follow_redirect!
assert_equal "/users/sign_in/#{Session.last!.id}", path
assert_equal "/users/sign_in/#{Session.last!.identifier}", path
end

test("POST /:passwordless_for/sign_in -> SUCCESS / custom delivery method") do
Expand Down Expand Up @@ -111,7 +111,7 @@ class << User
test("GET /:passwordless_for/sign_in/:id") do
passwordless_session = create_pwless_session

get("/users/sign_in/#{passwordless_session.id}")
get("/users/sign_in/#{passwordless_session.identifier}")

assert_equal 200, status
assert_equal "/users/sign_in/#{passwordless_session.to_param}", path
Expand All @@ -121,7 +121,7 @@ class << User
test("PATCH /:passwordless_for/sign_in/:id -> SUCCESS") do
passwordless_session = create_pwless_session(token: "hi")

patch("/users/sign_in/#{passwordless_session.id}", params: {passwordless: {token: "hi"}})
patch("/users/sign_in/#{passwordless_session.identifier}", params: {passwordless: {token: "hi"}})

assert_equal 303, status

Expand All @@ -135,7 +135,7 @@ class << User
test("PATCH /:passwordless_for/sign_in/:id -> ERROR") do
passwordless_session = create_pwless_session(token: "hi")

patch("/users/sign_in/#{passwordless_session.id}", params: {passwordless: {token: "no"}})
patch("/users/sign_in/#{passwordless_session.identifier}", params: {passwordless: {token: "no"}})

assert_equal 403, status
assert_equal "/users/sign_in/#{passwordless_session.to_param}", path
Expand All @@ -150,7 +150,7 @@ class << User

with_config(restrict_token_reuse: true) do
patch(
"/users/sign_in/#{passwordless_session.id}",
"/users/sign_in/#{passwordless_session.identifier}",
params: {passwordless: {token: "hi"}}
)
end
Expand All @@ -170,7 +170,7 @@ class << User
passwordless_session.update!(timeout_at: Time.current - 1.day)

patch(
"/users/sign_in/#{passwordless_session.id}",
"/users/sign_in/#{passwordless_session.identifier}",
params: {passwordless: {token: "hi"}}
)

Expand All @@ -188,7 +188,7 @@ class << User
user = User.create(email: "a@a")
passwordless_session = create_pwless_session(authenticatable: user, token: "hi")

get "/users/sign_in/#{passwordless_session.id}/#{passwordless_session.token}"
get "/users/sign_in/#{passwordless_session.identifier}/#{passwordless_session.token}"
assert_not_nil pwless_session(User)

get "/users/sign_out"
Expand Down
2 changes: 1 addition & 1 deletion test/dummy/config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
config.cache_store = :null_store

# Raise exceptions instead of rendering exception templates.
config.action_dispatch.show_exceptions = false
config.action_dispatch.show_exceptions = :internal

# Disable request forgery protection in test environment.
config.action_controller.allow_forgery_protection = false
Expand Down
2 changes: 2 additions & 0 deletions test/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
t.datetime "expires_at", precision: nil, null: false
t.datetime "claimed_at", precision: nil
t.string "token_digest", null: false
t.string "identifier", null: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, with null: false it might be worth considering column "identifier" of relation "passwordless_sessions" contains null values

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go with null: false as the new default in new installations but null: true in the upgrade instructions? That way it'll handle legacy tables and Rails can still validates :presence

t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.index ["authenticatable_type", "authenticatable_id"], name: "authenticatable"
t.index ["identifier"], name: "index_passwordless_sessions_on_identifier", unique: true
end

create_table "users", force: :cascade do |t|
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/passwordless/sessions.yml
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
# Read about fixtures at http:https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html

one:
identifier: 381a283e-4e64-4989-8b10-d5279bb7f338
timeout_at: 2017-11-04 23:17:35
expires_at: 2017-11-04 23:17:35
token_digest: MyString

two:
identifier: cb756ce8-62a0-4ac1-9cb2-7abce7eddf80
timeout_at: 2017-11-04 23:17:35
expires_at: 2017-11-04 23:17:35
token_digest: MyString

john:
identifier: 3d14ed2c-4eaa-4894-82fb-e2e6f92223e6
timeout_at: 2017-11-04 23:17:35
expires_at: 2017-11-04 23:17:35
token_digest: 3veDCdFw4VedgZtZZkCJ7um7rsc2bFNCZGB51Iih024
Expand Down
4 changes: 2 additions & 2 deletions test/integration/authentication_flow_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class AuthenticationFlowTest < ActionDispatch::SystemTestCase

email = ActionMailer::Base.deliveries.last
assert_equal alice.email, email.to.first
token = email.body.match(%r{/sign_in/\d+/([\w\-_]+)})[1]
token = email.body.match(%r{http:https://.*/sign_in/[A-Za-z0-9\-]+/(\w+)})[1]

fill_in "passwordless[token]", with: token
click_button "Confirm"
Expand All @@ -52,7 +52,7 @@ class AuthenticationFlowTest < ActionDispatch::SystemTestCase

email = ActionMailer::Base.deliveries.last
assert_equal alice.email, email.to.first
magic_link = email.body.to_s.scan(%r{http:https://.*/sign_in/\d+/\w+}).at(0)
magic_link = email.body.to_s.scan(%r{http:https://.*/sign_in/[a-z0-9\-]+/\w+}).at(0)

visit magic_link

Expand Down
2 changes: 1 addition & 1 deletion test/mailers/passwordless/mailer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ class Passwordless::MailerTest < ActionMailer::TestCase

assert_match "Signing in ✨", email.subject
assert_match /sign in: hello\n/, email.body.to_s
assert_match %r{/sign_in/#{session.id}/hello}, email.body.to_s
assert_match %r{/sign_in/#{session.identifier}/hello}, email.body.to_s
end
end