From 77430f090e913a2af5f6a502d35e56731ff8ce9e Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Thu, 22 Jun 2017 13:37:50 +0200 Subject: [PATCH 1/8] Login via recorder token, first blood --- lib/asciinema/auth.ex | 6 +++ web/controllers/session_controller.ex | 73 +++++++++++++++++++++++++++ web/router.ex | 6 +++ 3 files changed, 85 insertions(+) create mode 100644 web/controllers/session_controller.ex diff --git a/lib/asciinema/auth.ex b/lib/asciinema/auth.ex index 00d2c92..05e0e9b 100644 --- a/lib/asciinema/auth.ex +++ b/lib/asciinema/auth.ex @@ -14,6 +14,12 @@ defmodule Asciinema.Auth do assign(conn, :current_user, user) end + def login(conn, %User{id: id} = user) do + conn + |> put_session(@user_key, id) + |> assign(:current_user, user) + end + def get_basic_auth(conn) do with ["Basic " <> auth] <- get_req_header(conn, "authorization"), auth = String.replace(auth, ~r/^%/, ""), # workaround for 1.3.0-1.4.0 client bug diff --git a/web/controllers/session_controller.ex b/web/controllers/session_controller.ex new file mode 100644 index 0000000..242d507 --- /dev/null +++ b/web/controllers/session_controller.ex @@ -0,0 +1,73 @@ +defmodule Asciinema.SessionController do + use Asciinema.Web, :controller + import Asciinema.UserView, only: [profile_path: 1] + alias Asciinema.{Auth, Users, User} + + def create(conn, %{"api_token" => api_token}) do + case Users.authenticate(api_token) do + {:ok, user} -> + login(conn, user) + {:error, :token_not_found} -> + conn + |> put_rails_flash(:alert, "Invalid token. Make sure you pasted the URL correctly.") + |> redirect(to: "/") + {:error, :token_revoked} -> + conn + |> put_rails_flash(:alert, "This token has been revoked.") + |> redirect(to: "/") + end + end + + defp login(conn, logging_user) do + case {conn.assigns.current_user, logging_user} do + {nil, %User{username: nil}} -> + conn + |> Auth.login(logging_user) + |> put_rails_flash(:notice, "Welcome! Setting username and email will help you with logging in later.") + |> redirect_to_edit_profile + {nil, %User{}} -> + conn + |> Auth.login(logging_user) + |> put_rails_flash(:notice, "Welcome back!") + |> redirect_to_profile + {%User{id: id, username: nil}, %User{id: id}} -> + conn + |> put_rails_flash(:notice, "Setting username and email will help you with logging in later.") + |> redirect_to_edit_profile + {%User{username: nil}, %User{username: nil}} -> + conn + |> put_rails_flash(:notice, "WATCHA GONNA DO") # TODO + |> redirect(to: "/") + {%User{username: nil}, %User{}} -> + conn + |> put_rails_flash(:notice, "WATCHA GONNA DO") # TODO + |> redirect(to: "/") + {%User{}, %User{username: nil}} -> + conn + |> put_rails_flash(:notice, "Recorder token has been added to your account.") + |> redirect_to_profile + # TODO auto-merge users + {%User{id: id}, %User{id: id}} -> + conn + |> put_rails_flash(:notice, "You're already logged in.") + |> redirect_to_profile + {%User{}, %User{}} -> + conn + |> put_rails_flash(:alert, "You're already logged in as a different user.") + |> redirect_to_profile + # TODO offer merging + end + end + + defp put_rails_flash(conn, key, value) do + put_session(conn, :flash, %{discard: [], flashes: %{key => value}}) + end + + defp redirect_to_profile(conn) do + redirect(conn, to: profile_path(conn.assigns.current_user)) + end + + defp redirect_to_edit_profile(conn) do + redirect(conn, to: user_path(conn, :edit)) + end +end diff --git a/web/router.ex b/web/router.ex index f248394..40ce858 100644 --- a/web/router.ex +++ b/web/router.ex @@ -50,6 +50,8 @@ defmodule Asciinema.Router do get "/docs", DocController, :index get "/docs/:topic", DocController, :show + + get "/connect/:api_token", SessionController, :create end scope "/api", Asciinema.Api, as: :api do @@ -65,6 +67,10 @@ end defmodule Asciinema.Router.Helpers.Extra do alias Asciinema.Router.Helpers, as: H + def user_path(_conn, :edit) do + "/user/edit" + end + def asciicast_file_download_path(conn, asciicast) do H.asciicast_file_path(conn, :show, asciicast) |> String.replace_suffix("/json", ".json") From 7cc07795aef89aa0328114c097d8d666468c577e Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Thu, 22 Jun 2017 13:38:19 +0200 Subject: [PATCH 2/8] Route to new login-via-recorder-token impl --- docker/nginx/asciinema.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/nginx/asciinema.conf b/docker/nginx/asciinema.conf index 1a8b655..c333e8c 100644 --- a/docker/nginx/asciinema.conf +++ b/docker/nginx/asciinema.conf @@ -24,7 +24,7 @@ server { client_max_body_size 16m; - location ~ ^/(phoenix/|css/|js/|images/|fonts/|docs/?|api/asciicasts) { + location ~ ^/(phoenix/|css/|js/|images/|fonts/|docs/?|api/asciicasts|/connect/) { try_files /maintenance.html $uri/index.html $uri.html $uri @phoenix; } From ff3aa008eb75458ba797d8ef9cd3941788e59bf8 Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Fri, 23 Jun 2017 09:26:30 +0200 Subject: [PATCH 3/8] Check for email, not username, when detecting incomplete user accounts --- web/controllers/session_controller.ex | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/web/controllers/session_controller.ex b/web/controllers/session_controller.ex index 242d507..bc52896 100644 --- a/web/controllers/session_controller.ex +++ b/web/controllers/session_controller.ex @@ -20,7 +20,7 @@ defmodule Asciinema.SessionController do defp login(conn, logging_user) do case {conn.assigns.current_user, logging_user} do - {nil, %User{username: nil}} -> + {nil, %User{email: nil}} -> conn |> Auth.login(logging_user) |> put_rails_flash(:notice, "Welcome! Setting username and email will help you with logging in later.") @@ -30,19 +30,19 @@ defmodule Asciinema.SessionController do |> Auth.login(logging_user) |> put_rails_flash(:notice, "Welcome back!") |> redirect_to_profile - {%User{id: id, username: nil}, %User{id: id}} -> + {%User{id: id, email: nil}, %User{id: id}} -> conn |> put_rails_flash(:notice, "Setting username and email will help you with logging in later.") |> redirect_to_edit_profile - {%User{username: nil}, %User{username: nil}} -> + {%User{email: nil}, %User{email: nil}} -> conn |> put_rails_flash(:notice, "WATCHA GONNA DO") # TODO |> redirect(to: "/") - {%User{username: nil}, %User{}} -> + {%User{email: nil}, %User{}} -> conn |> put_rails_flash(:notice, "WATCHA GONNA DO") # TODO |> redirect(to: "/") - {%User{}, %User{username: nil}} -> + {%User{}, %User{email: nil}} -> conn |> put_rails_flash(:notice, "Recorder token has been added to your account.") |> redirect_to_profile From c64bb379aaf9c54c5918fc8383d8b305cc68b265 Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Fri, 23 Jun 2017 09:35:56 +0200 Subject: [PATCH 4/8] Display tmp username for current user when no username nor email set --- app/decorators/current_user_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/decorators/current_user_decorator.rb b/app/decorators/current_user_decorator.rb index 962f22b..4f8917f 100644 --- a/app/decorators/current_user_decorator.rb +++ b/app/decorators/current_user_decorator.rb @@ -1,7 +1,7 @@ class CurrentUserDecorator < UserDecorator def display_name - model.username || model.email + model.username || model.email || model.temporary_username || "Me" end end From 4b3e81c813bd3e7fd81cbe2266a215c43c3fc718 Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Fri, 23 Jun 2017 10:21:20 +0200 Subject: [PATCH 5/8] Auto-merging of tmp user into current user --- lib/asciinema/users.ex | 14 ++++++++++++++ web/controllers/session_controller.ex | 4 ++-- web/models/expiring_token.ex | 7 +++++++ web/models/user.ex | 2 ++ 4 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 web/models/expiring_token.ex diff --git a/lib/asciinema/users.ex b/lib/asciinema/users.ex index bfaab45..fc689a9 100644 --- a/lib/asciinema/users.ex +++ b/lib/asciinema/users.ex @@ -1,5 +1,6 @@ defmodule Asciinema.Users do import Ecto.Query, warn: false + import Ecto, only: [assoc: 2] alias Asciinema.{Repo, User, ApiToken} def authenticate(api_token) do @@ -55,4 +56,17 @@ defmodule Asciinema.Users do |> ApiToken.revoke_changeset |> Repo.update! end + + def merge!(dst_user, src_user) do + Repo.transaction(fn -> + asciicasts_q = from(assoc(src_user, :asciicasts)) + Repo.update_all(asciicasts_q, set: [user_id: dst_user.id, updated_at: Timex.now]) + api_tokens_q = from(assoc(src_user, :api_tokens)) + Repo.update_all(api_tokens_q, set: [user_id: dst_user.id, updated_at: Timex.now]) + expiring_tokens_q = from(assoc(src_user, :expiring_tokens)) + Repo.delete_all(expiring_tokens_q) + Repo.delete!(src_user) + dst_user + end) + end end diff --git a/web/controllers/session_controller.ex b/web/controllers/session_controller.ex index bc52896..fdb6acf 100644 --- a/web/controllers/session_controller.ex +++ b/web/controllers/session_controller.ex @@ -42,11 +42,11 @@ defmodule Asciinema.SessionController do conn |> put_rails_flash(:notice, "WATCHA GONNA DO") # TODO |> redirect(to: "/") - {%User{}, %User{email: nil}} -> + {%User{} = u1, %User{email: nil} = u2} -> + Users.merge!(u1, u2) conn |> put_rails_flash(:notice, "Recorder token has been added to your account.") |> redirect_to_profile - # TODO auto-merge users {%User{id: id}, %User{id: id}} -> conn |> put_rails_flash(:notice, "You're already logged in.") diff --git a/web/models/expiring_token.ex b/web/models/expiring_token.ex new file mode 100644 index 0000000..0038f65 --- /dev/null +++ b/web/models/expiring_token.ex @@ -0,0 +1,7 @@ +defmodule Asciinema.ExpiringToken do + use Asciinema.Web, :model + + schema "expiring_tokens" do + belongs_to :user, Asciinema.User + end +end diff --git a/web/models/user.ex b/web/models/user.ex index 270af52..e87d6ff 100644 --- a/web/models/user.ex +++ b/web/models/user.ex @@ -14,6 +14,8 @@ defmodule Asciinema.User do timestamps(inserted_at: :created_at) has_many :asciicasts, Asciinema.Asciicast + has_many :api_tokens, Asciinema.ApiToken + has_many :expiring_tokens, Asciinema.ExpiringToken end @doc """ From bce66edf0033fcd83a3cc775a1b183ac2a9a00b3 Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Fri, 23 Jun 2017 10:54:50 +0200 Subject: [PATCH 6/8] Update flash message --- web/controllers/session_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/controllers/session_controller.ex b/web/controllers/session_controller.ex index fdb6acf..df68e83 100644 --- a/web/controllers/session_controller.ex +++ b/web/controllers/session_controller.ex @@ -53,7 +53,7 @@ defmodule Asciinema.SessionController do |> redirect_to_profile {%User{}, %User{}} -> conn - |> put_rails_flash(:alert, "You're already logged in as a different user.") + |> put_rails_flash(:alert, "This recorder token belongs to a different user.") |> redirect_to_profile # TODO offer merging end From 2042760abdcb03435d93a44ac34c6a87f485077a Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Fri, 23 Jun 2017 11:12:20 +0200 Subject: [PATCH 7/8] Handle cases where there is current user which is tmp user --- web/controllers/session_controller.ex | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/web/controllers/session_controller.ex b/web/controllers/session_controller.ex index df68e83..8965a6c 100644 --- a/web/controllers/session_controller.ex +++ b/web/controllers/session_controller.ex @@ -19,7 +19,9 @@ defmodule Asciinema.SessionController do end defp login(conn, logging_user) do - case {conn.assigns.current_user, logging_user} do + current_user = conn.assigns.current_user + + case {current_user, logging_user} do {nil, %User{email: nil}} -> conn |> Auth.login(logging_user) @@ -35,15 +37,18 @@ defmodule Asciinema.SessionController do |> put_rails_flash(:notice, "Setting username and email will help you with logging in later.") |> redirect_to_edit_profile {%User{email: nil}, %User{email: nil}} -> + Users.merge!(current_user, logging_user) conn - |> put_rails_flash(:notice, "WATCHA GONNA DO") # TODO - |> redirect(to: "/") + |> put_rails_flash(:notice, "Setting username and email will help you with logging in later.") + |> redirect_to_edit_profile {%User{email: nil}, %User{}} -> + Users.merge!(logging_user, current_user) conn - |> put_rails_flash(:notice, "WATCHA GONNA DO") # TODO - |> redirect(to: "/") - {%User{} = u1, %User{email: nil} = u2} -> - Users.merge!(u1, u2) + |> Auth.login(logging_user) + |> put_rails_flash(:notice, "Recorder token has been added to your account.") + |> redirect_to_profile + {%User{}, %User{email: nil}} -> + Users.merge!(current_user, logging_user) conn |> put_rails_flash(:notice, "Recorder token has been added to your account.") |> redirect_to_profile From 6f801bf445d8df8550fd710c71b563b174738292 Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Fri, 23 Jun 2017 12:55:15 +0200 Subject: [PATCH 8/8] Test new session controller --- config/test.exs | 1 + lib/asciinema/auth.ex | 3 + test/controllers/session_controller_test.exs | 104 +++++++++++++++++++ test/support/fixtures.ex | 7 +- 4 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 test/controllers/session_controller_test.exs diff --git a/config/test.exs b/config/test.exs index c0717b8..5a1e3cc 100644 --- a/config/test.exs +++ b/config/test.exs @@ -4,6 +4,7 @@ use Mix.Config # you can enable the server option below. config :asciinema, Asciinema.Endpoint, http: [port: 4001], + secret_key_base: "ssecretkeybasesecretkeybasesecretkeybasesecretkeybaseecretkeybase", server: false # Print only warnings and errors during test diff --git a/lib/asciinema/auth.ex b/lib/asciinema/auth.ex index 05e0e9b..7661f48 100644 --- a/lib/asciinema/auth.ex +++ b/lib/asciinema/auth.ex @@ -8,6 +8,9 @@ defmodule Asciinema.Auth do opts end + def call(%Plug.Conn{assigns: %{current_user: %User{}}} = conn, _opts) do + conn + end def call(conn, _opts) do user_id = get_session(conn, @user_key) user = user_id && Repo.get(User, user_id) diff --git a/test/controllers/session_controller_test.exs b/test/controllers/session_controller_test.exs new file mode 100644 index 0000000..10256cb --- /dev/null +++ b/test/controllers/session_controller_test.exs @@ -0,0 +1,104 @@ +defmodule Asciinema.SessionControllerTest do + use Asciinema.ConnCase + alias Asciinema.{Users, User, ApiToken} + + @revoked_token "eb927b31-9ca3-4a6a-8a0c-dfba318e2e84" + @regular_user_token "c4ecd96a-9a16-464d-be6a-bc1f3c50c4ae" + @other_regular_user_token "b26c2fe0-603b-4b10-b0fa-f6ec85628831" + @tmp_user_token "863f6ae5-3f32-4ffc-8d47-284222d6225f" + @other_tmp_user_token "2eafaa20-80c8-47fc-b014-74072027edae" + + setup %{conn: conn} do + %User{} = Users.get_user_with_api_token("revoked", @revoked_token) + @revoked_token |> Users.get_api_token! |> Users.revoke_api_token! + + regular_user = fixture(:user) + ApiToken.create_changeset(regular_user, @regular_user_token) |> Repo.insert! + + other_regular_user = fixture(:user, %{username: "other", email: "other@example.com"}) + ApiToken.create_changeset(other_regular_user, @other_regular_user_token) |> Repo.insert! + + %User{} = tmp_user = Users.get_user_with_api_token("tmp", @tmp_user_token) + + %User{} = Users.get_user_with_api_token("other_tmp", @other_tmp_user_token) + + {:ok, conn: conn, regular_user: regular_user, tmp_user: tmp_user} + end + + test "invalid token", %{conn: conn} do + conn = get conn, "/connect/nopenope" + assert redirected_to(conn, 302) == "/" + assert get_rails_flash(conn, :alert) =~ ~r/invalid token/i + end + + test "revoked token", %{conn: conn} do + conn = get conn, "/connect/#{@revoked_token}" + assert redirected_to(conn, 302) == "/" + assert get_rails_flash(conn, :alert) =~ ~r/been revoked/i + end + + test "guest with tmp user token", %{conn: conn} do + conn = get conn, "/connect/#{@tmp_user_token}" + assert redirected_to(conn, 302) == "/user/edit" + assert get_rails_flash(conn, :notice) =~ ~r/welcome.+username.+email/i + end + + test "guest with regular user token", %{conn: conn} do + conn = get conn, "/connect/#{@regular_user_token}" + assert redirected_to(conn, 302) == "/~test" + assert get_rails_flash(conn, :notice) =~ ~r/welcome back/i + end + + test "tmp user with his own token", %{conn: conn, tmp_user: user} do + conn = login_as(conn, user) + conn = get conn, "/connect/#{@tmp_user_token}" + assert redirected_to(conn, 302) == "/user/edit" + assert get_rails_flash(conn, :notice) + end + + test "tmp user with other tmp user token", %{conn: conn, tmp_user: user} do + conn = login_as(conn, user) + conn = get conn, "/connect/#{@other_tmp_user_token}" + assert redirected_to(conn, 302) == "/user/edit" + assert get_rails_flash(conn, :notice) + end + + test "tmp user with other regular user token", %{conn: conn, tmp_user: user} do + conn = login_as(conn, user) + conn = get conn, "/connect/#{@regular_user_token}" + assert redirected_to(conn, 302) == "/~test" + assert get_rails_flash(conn, :notice) + end + + test "regular user with other tmp user token", %{conn: conn, regular_user: user} do + conn = login_as(conn, user) + conn = get conn, "/connect/#{@tmp_user_token}" + assert redirected_to(conn, 302) == "/~test" + assert get_rails_flash(conn, :notice) + end + + test "regular user with his own token", %{conn: conn, regular_user: user} do + conn = login_as(conn, user) + conn = get conn, "/connect/#{@regular_user_token}" + assert redirected_to(conn, 302) == "/~test" + assert get_rails_flash(conn, :notice) + end + + test "regular user with other regular user token", %{conn: conn, regular_user: user} do + conn = login_as(conn, user) + conn = get conn, "/connect/#{@other_regular_user_token}" + assert redirected_to(conn, 302) == "/~test" + assert get_rails_flash(conn, :alert) + end + + defp get_rails_flash(conn, key) do + conn + |> get_session(:flash) + |> get_in([:flashes, key]) + end + + defp login_as(conn, user) do + assign(conn, :current_user, user) + end + +end diff --git a/test/support/fixtures.ex b/test/support/fixtures.ex index 88c9d43..5146a5f 100644 --- a/test/support/fixtures.ex +++ b/test/support/fixtures.ex @@ -11,9 +11,10 @@ defmodule Asciinema.Fixtures do content_type: "application/json"} end - def fixture(:user, _attrs) do - attrs = %{username: "test", - auth_token: "authy-auth-auth"} + def fixture(:user, attrs) do + attrs = Map.merge(%{username: "test", + email: "test@example.com", + auth_token: "authy-auth-auth"}, attrs) Repo.insert!(User.changeset(%User{}, attrs)) end