From 96c9ec262a793dbd2c9abaf215b62ecc70348c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sat, 8 Jun 2024 18:38:00 +0100 Subject: [PATCH 01/27] Basic setup for applications Setup schema, a query on primary key and a test. No UI or write ops yet. --- lib/teiserver/o_auth.ex | 21 +++++++ .../o_auth/queries/application_query.ex | 24 ++++++++ lib/teiserver/o_auth/schemas/application.ex | 55 +++++++++++++++++++ .../migrations/20240608144340_oauth_setup.exs | 17 ++++++ test/teiserver/o_auth/application_test.exs | 51 +++++++++++++++++ 5 files changed, 168 insertions(+) create mode 100644 lib/teiserver/o_auth.ex create mode 100644 lib/teiserver/o_auth/queries/application_query.ex create mode 100644 lib/teiserver/o_auth/schemas/application.ex create mode 100644 priv/repo/migrations/20240608144340_oauth_setup.exs create mode 100644 test/teiserver/o_auth/application_test.exs diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex new file mode 100644 index 000000000..5e83d91fd --- /dev/null +++ b/lib/teiserver/o_auth.ex @@ -0,0 +1,21 @@ +defmodule Teiserver.OAuth do + alias Teiserver.Repo + alias Teiserver.OAuth.{Application, ApplicationQueries} + + def create_application(attrs \\ %{}) do + %Application{} + |> Application.changeset(attrs) + |> Repo.insert() + end + + @spec delete_application(Application.t()) :: :ok | {:error, term()} + def delete_application(app) do + case Repo.delete(app) do + {:ok, _} -> :ok + {:error, err} -> {:error, err} + end + end + + @spec get_application_by_uid(Application.app_id()) :: Application.t() | nil + defdelegate get_application_by_uid(uid), to: ApplicationQueries +end diff --git a/lib/teiserver/o_auth/queries/application_query.ex b/lib/teiserver/o_auth/queries/application_query.ex new file mode 100644 index 000000000..0df1a29c7 --- /dev/null +++ b/lib/teiserver/o_auth/queries/application_query.ex @@ -0,0 +1,24 @@ +defmodule Teiserver.OAuth.ApplicationQueries do + use TeiserverWeb, :queries + + alias Teiserver.OAuth.Application + + @doc """ + Returns the application corresponding to the given uid/client id + """ + @spec get_application_by_uid(String.t()) :: Application.t() | nil + def get_application_by_uid(nil), do: nil + + def get_application_by_uid(uid) do + base_query() |> where_uid(uid) |> Repo.one() + end + + def base_query() do + from app in Application, as: :app + end + + def where_uid(query, uid) do + from e in query, + where: e.uid == ^uid + end +end diff --git a/lib/teiserver/o_auth/schemas/application.ex b/lib/teiserver/o_auth/schemas/application.ex new file mode 100644 index 000000000..048d6c34f --- /dev/null +++ b/lib/teiserver/o_auth/schemas/application.ex @@ -0,0 +1,55 @@ +defmodule Teiserver.OAuth.Application do + @moduledoc false + use TeiserverWeb, :schema + alias Teiserver.Account.User + + @type app_id() :: String.t() + @type scopes() :: nonempty_list(String.t()) + @type t :: %__MODULE__{ + name: String.t(), + owner: User.t(), + uid: app_id(), + scopes: scopes(), + redirect_uris: [String.t()], + description: String.t(), + inserted_at: DateTime.t(), + updated_at: DateTime.t() + } + + schema "oauth_applications" do + field :name, :string + belongs_to :owner, Teiserver.Account.User, primary_key: true + field :uid, :string + field :scopes, {:array, :string} + field :redirect_uris, {:array, :string}, default: [] + field :description, :string + + timestamps(type: :utc_datetime) + end + + def allowed_scopes do + ~w(tachyon.lobby) + end + + def changeset(application, attrs) do + attrs = attrs |> uniq_lists(~w(scopes)a) + + application + |> cast(attrs, [:name, :owner_id, :uid, :scopes, :redirect_uris, :description]) + |> validate_required([:name, :owner_id, :uid, :scopes]) + |> Ecto.Changeset.validate_change(:redirect_uris, fn :redirect_uris, uris -> + invalid_uris = Enum.filter(uris, fn uri -> is_nil(URI.parse(uri).host) end) + + if Enum.empty?(invalid_uris) do + [] + else + [redirect_uris: "invalid uri found: #{Enum.join(invalid_uris, ", ")}"] + end + end) + |> Ecto.Changeset.unique_constraint(:uid) + |> Ecto.Changeset.validate_subset(:scopes, allowed_scopes(), + message: "Must be #{allowed_scopes()}" + ) + |> Ecto.Changeset.validate_length(:scopes, min: 1) + end +end diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs new file mode 100644 index 000000000..97935fd4f --- /dev/null +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -0,0 +1,17 @@ +defmodule Teiserver.Repo.Migrations.OauthSetup do + use Ecto.Migration + + def change do + create_if_not_exists table(:oauth_applications) do + add :name, :string, null: false, comment: "display name" + add :owner_id, references(:account_users, on_delete: :delete_all) + add :uid, :string, null: false, comment: "aka client_id" + add :scopes, {:array, :string}, null: false + add :redirect_uris, {:array, :string}, null: false, default: [] + add :description, :text + + timestamps(type: :utc_datetime) + end + create_if_not_exists unique_index(:oauth_applications, [:uid]) + end +end diff --git a/test/teiserver/o_auth/application_test.exs b/test/teiserver/o_auth/application_test.exs new file mode 100644 index 000000000..6fa18321d --- /dev/null +++ b/test/teiserver/o_auth/application_test.exs @@ -0,0 +1,51 @@ +defmodule Teiserver.OAuth.ApplicationTest do + use Teiserver.DataCase, async: true + alias Teiserver.OAuth + + test "reject unknown scopes at creation" do + user = Teiserver.TeiserverTestLib.new_user() + + assert {:error, changeset} = + OAuth.create_application(%{ + name: "Testing app", + uid: "test_app_uid", + owner_id: user.id, + scopes: ["lol"] + }) + assert Keyword.fetch!(changeset.errors, :scopes) + end + + test "can retrieve an app by uid" do + user = Teiserver.TeiserverTestLib.new_user() + + assert {:ok, expected_app} = + OAuth.create_application(%{ + name: "Testing app", + uid: "test_app_uid", + owner_id: user.id, + scopes: ["tachyon.lobby"] + }) + + assert expected_app == OAuth.get_application_by_uid("test_app_uid") + end + + test "delete app" do + user = Teiserver.TeiserverTestLib.new_user() + + assert {:ok, expected_app} = + OAuth.create_application(%{ + name: "Testing app", + uid: "test_app_uid", + owner_id: user.id, + scopes: ["tachyon.lobby"] + }) + + assert :ok = OAuth.delete_application(expected_app) + assert OAuth.get_application_by_uid(expected_app.uid) == nil + end + + test "non existant uid returns nil" do + assert OAuth.get_application_by_uid("u_wot_mate?") == nil + assert OAuth.get_application_by_uid(nil) == nil + end +end From 28a5438454b81ac0e95b80091bcf7371ff9285f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 9 Jun 2024 11:10:30 +0100 Subject: [PATCH 02/27] Authorization code logic and table --- lib/teiserver/o_auth.ex | 66 ++++++++++++++++++- lib/teiserver/o_auth/queries/code_query.ex | 24 +++++++ lib/teiserver/o_auth/schemas/code.ex | 33 ++++++++++ .../migrations/20240608144340_oauth_setup.exs | 13 +++- test/teiserver/o_auth/code_test.exs | 30 +++++++++ 5 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 lib/teiserver/o_auth/queries/code_query.ex create mode 100644 lib/teiserver/o_auth/schemas/code.ex create mode 100644 test/teiserver/o_auth/code_test.exs diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index 5e83d91fd..393c9f1fa 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -1,6 +1,8 @@ defmodule Teiserver.OAuth do alias Teiserver.Repo - alias Teiserver.OAuth.{Application, ApplicationQueries} + alias Teiserver.OAuth.{Application, Code, ApplicationQueries, CodeQueries} + alias Teiserver.Account.User + alias Teiserver.Data.Types, as: T def create_application(attrs \\ %{}) do %Application{} @@ -16,6 +18,68 @@ defmodule Teiserver.OAuth do end end + @type option :: {:now, DateTime.t()} + @type options :: [option] + + @doc """ + Create an authorization token for the given user and application. + The token scopes are the same as the application + """ + @spec create_code(User.t() | T.userid(), Application.t(), options()) :: + {:ok, Code.t()} | {:error, Ecto.Changeset.t()} + def create_code(user, application, opts \\ []) + + def create_code(%User{} = user, application, opts) do + create_code(user.id, application, opts) + end + + def create_code(user, application, opts) when is_map(user) do + create_code(user.id, application, opts) + end + + def create_code(user_id, application, opts) do + now = Keyword.get(opts, :now, Timex.now()) + + attrs = %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32)), + owner_id: user_id, + application_id: application.id, + scopes: application.scopes, + expires_at: Timex.add(now, Timex.Duration.from_minutes(5)) + } + + %Code{} + |> Code.changeset(attrs) + |> Repo.insert() + end + + @doc """ + Given a code returns the corresponding db object, making sure + it is valid (exists, not expired, not revoked) + """ + @spec get_valid_code(String.t(), options()) :: {:ok, Code.t()} | {:error, term()} + def get_valid_code(code, opts \\ []) + + def get_valid_code(code, opts) do + case CodeQueries.get_code(code) do + nil -> + {:error, :no_code} + + code -> + now = Keyword.get(opts, :now, Timex.now()) + + if expired?(code, now) do + {:error, :expired} + else + {:ok, code} + end + end + end + @spec get_application_by_uid(Application.app_id()) :: Application.t() | nil defdelegate get_application_by_uid(uid), to: ApplicationQueries + + defp expired?(obj, now) do + Timex.after?(now, Map.fetch!(obj, :expires_at)) + end end diff --git a/lib/teiserver/o_auth/queries/code_query.ex b/lib/teiserver/o_auth/queries/code_query.ex new file mode 100644 index 000000000..00cf6166c --- /dev/null +++ b/lib/teiserver/o_auth/queries/code_query.ex @@ -0,0 +1,24 @@ +defmodule Teiserver.OAuth.CodeQueries do + use TeiserverWeb, :queries + alias Teiserver.OAuth.Code + + @doc """ + Return the db object corresponding to the given code. + This doesn't validate anything, use the context function instead + """ + @spec get_code(String.t() | nil) :: Code.t() | nil + def get_code(nil), do: nil + + def get_code(code) do + base_query() |> where_code(code) |> Repo.one() + end + + def base_query() do + from code in Code, as: :code + end + + def where_code(query, value) do + from e in query, + where: e.value == ^value + end +end diff --git a/lib/teiserver/o_auth/schemas/code.ex b/lib/teiserver/o_auth/schemas/code.ex new file mode 100644 index 000000000..8644a2332 --- /dev/null +++ b/lib/teiserver/o_auth/schemas/code.ex @@ -0,0 +1,33 @@ +defmodule Teiserver.OAuth.Code do + @moduledoc false + use TeiserverWeb, :schema + + alias Teiserver.OAuth + + @type t :: %__MODULE__{ + value: String.t(), + owner: User.t(), + application: OAuth.Application.t(), + scopes: OAuth.Application.scopes(), + expires_at: DateTime.t() + } + + schema "oauth_codes" do + field :value, :string + belongs_to :owner, Teiserver.Account.User, primary_key: true + belongs_to :application, OAuth.Application, primary_key: true + field :scopes, {:array, :string} + field :expires_at, :utc_datetime + + timestamps() + end + + def changeset(code, attrs) do + attrs = attrs |> uniq_lists(~w(scopes)a) + + code + |> cast(attrs, [:value, :owner_id, :application_id, :scopes, :expires_at]) + |> validate_required([:value, :owner_id, :application_id, :scopes, :expires_at]) + |> Ecto.Changeset.validate_subset(:scopes, OAuth.Application.allowed_scopes()) + end +end diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs index 97935fd4f..927912c90 100644 --- a/priv/repo/migrations/20240608144340_oauth_setup.exs +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -12,6 +12,17 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do timestamps(type: :utc_datetime) end - create_if_not_exists unique_index(:oauth_applications, [:uid]) + + create_if_not_exists table(:oauth_codes, comment: "authorisation codes") do + add :value, :string, null: false + add :owner_id, references(:account_users, on_delete: :delete_all), null: false + add :application_id, references(:oauth_applications, on_delete: :delete_all), null: false + add :scopes, {:array, :string}, null: false + add :expires_at, :utc_datetime, null: false + + timestamps(type: :utc_datetime) + end + create_if_not_exists unique_index(:oauth_codes, [:value]) + end end diff --git a/test/teiserver/o_auth/code_test.exs b/test/teiserver/o_auth/code_test.exs new file mode 100644 index 000000000..04b96efc5 --- /dev/null +++ b/test/teiserver/o_auth/code_test.exs @@ -0,0 +1,30 @@ +defmodule Teiserver.OAuth.CodeTest do + use Teiserver.DataCase, async: true + alias Teiserver.OAuth + + setup do + user = Teiserver.TeiserverTestLib.new_user() + + {:ok, app} = + OAuth.create_application(%{ + name: "Testing app", + uid: "test_app_uid", + owner_id: user.id, + scopes: ["tachyon.lobby"] + }) + + {:ok, user: user, app: app} + end + + test "can get valid code", %{user: user, app: app} do + assert {:ok, code} = OAuth.create_code(user, app) + assert {:ok, ^code} = OAuth.get_valid_code(code.value) + assert {:error, :no_code} = OAuth.get_valid_code(nil) + end + + test "cannot retrieve expired code", %{user: user, app: app} do + yesterday = Timex.shift(Timex.now(), days: -1) + assert {:ok, code} = OAuth.create_code(user, app, now: yesterday) + assert {:error, :expired} = OAuth.get_valid_code(code.value) + end +end From 28d0350b5cc07064109947cb19a13baeb550ca8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Mon, 10 Jun 2024 20:35:10 +0100 Subject: [PATCH 03/27] Basic logic for refresh tokens --- lib/teiserver/o_auth.ex | 97 ++++++++++++++++++- lib/teiserver/o_auth/queries/token_query.ex | 50 ++++++++++ lib/teiserver/o_auth/schemas/token.ex | 38 ++++++++ .../migrations/20240608144340_oauth_setup.exs | 14 +++ test/teiserver/o_auth/token_test.exs | 57 +++++++++++ 5 files changed, 255 insertions(+), 1 deletion(-) create mode 100644 lib/teiserver/o_auth/queries/token_query.ex create mode 100644 lib/teiserver/o_auth/schemas/token.ex create mode 100644 test/teiserver/o_auth/token_test.exs diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index 393c9f1fa..663a558a2 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -1,6 +1,6 @@ defmodule Teiserver.OAuth do alias Teiserver.Repo - alias Teiserver.OAuth.{Application, Code, ApplicationQueries, CodeQueries} + alias Teiserver.OAuth.{Application, Code, Token, ApplicationQueries, CodeQueries, TokenQueries} alias Teiserver.Account.User alias Teiserver.Data.Types, as: T @@ -76,6 +76,101 @@ defmodule Teiserver.OAuth do end end + @spec create_token(User.t() | T.userid(), Application.t(), options()) :: + {:ok, Token.t()} | {:error, Ecto.Changeset.t()} + def create_token(user_id, application, opts \\ []) + + def create_token(%User{} = user, application, opts) do + create_token(user.id, application, opts) + end + + def create_token(user, application, opts) when is_map(user) do + create_token(user.id, application, opts) + end + + def create_token(user_id, application, opts) do + now = Keyword.get(opts, :now, DateTime.utc_now()) + + refresh_attrs = %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), + owner_id: user_id, + application_id: application.id, + scopes: application.scopes, + # there's no real recourse when the refresh token expires and it's + # quite annoying, so make it "never" expire. + expires_at: Timex.add(now, Timex.Duration.from_days(365 * 100)), + type: :refresh, + refresh_token: nil + } + + token_attrs = %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), + owner_id: user_id, + application_id: application.id, + scopes: application.scopes, + expires_at: Timex.add(now, Timex.Duration.from_minutes(30)), + type: :bearer, + refresh_token: refresh_attrs + } + + %Token{} + |> Token.changeset(token_attrs) + |> Repo.insert() + end + + # TODO: get_valid_token is basically the same as get_valid_code, refactor later + + @doc """ + Given a code returns the corresponding db object, making sure + it is valid (exists, not expired, not revoked) + """ + @spec get_valid_token(String.t(), options()) :: {:ok, Token.t()} | {:error, term()} + def get_valid_token(value, opts \\ []) + + def get_valid_token(value, opts) do + case TokenQueries.get_token(value) do + nil -> + {:error, :no_token} + + token -> + now = Keyword.get(opts, :now, Timex.now()) + + if Timex.after?(now, token.expires_at) do + {:error, :expired} + else + {:ok, token} + end + end + end + + @spec refresh_token(Token.t(), options()) :: {:ok, Token.t()} | {:error, term()} + def refresh_token(token, opts \\ []) + + def refresh_token(token, _opts) when token.type != :refresh do + {:error, :invalid_token} + end + + def refresh_token(token, opts) do + now = Keyword.get(opts, :now, Timex.now()) + + if expired?(token, now) do + {:error, :expired} + else + token = + if Ecto.assoc_loaded?(token.application) do + token + else + TokenQueries.get_token(token.value) + end + + Repo.transaction(fn -> + {:ok, new_token} = create_token(token.owner_id, token.application, opts) + TokenQueries.delete_refresh_token(token) + new_token + end) + end + end + @spec get_application_by_uid(Application.app_id()) :: Application.t() | nil defdelegate get_application_by_uid(uid), to: ApplicationQueries diff --git a/lib/teiserver/o_auth/queries/token_query.ex b/lib/teiserver/o_auth/queries/token_query.ex new file mode 100644 index 000000000..b37c027e9 --- /dev/null +++ b/lib/teiserver/o_auth/queries/token_query.ex @@ -0,0 +1,50 @@ +defmodule Teiserver.OAuth.TokenQueries do + use TeiserverWeb, :queries + alias Teiserver.OAuth.Token + + @doc """ + Return the db object corresponding to the given token. + This doesn't validate anything, use the context function instead + """ + def get_token(nil), do: nil + + def get_token(value) do + base_query() |> where_token(value) |> with_app() |> Repo.one() + end + + def base_query() do + from token in Token, + as: :token, + preload: [refresh_token: token] + end + + def where_token(query, value) do + from e in query, + where: e.value == ^value + end + + @doc """ + ensure the related application is loaded + """ + def with_app(query) do + query = + if has_named_binding?(query, :app) do + query + else + from token in query, + join: app in assoc(token, :application), + as: :app + end + + from [app: app] in query, + preload: [application: app] + end + + @doc """ + given a refresh token, deletes it and its potential associated token + """ + def delete_refresh_token(token) do + from(tok in Token, where: tok.id == ^token.id or tok.refresh_token_id == ^token.id) + |> Repo.delete_all() + end +end diff --git a/lib/teiserver/o_auth/schemas/token.ex b/lib/teiserver/o_auth/schemas/token.ex new file mode 100644 index 000000000..ddf223a91 --- /dev/null +++ b/lib/teiserver/o_auth/schemas/token.ex @@ -0,0 +1,38 @@ +defmodule Teiserver.OAuth.Token do + @moduledoc false + use TeiserverWeb, :schema + + alias Teiserver.OAuth + + @type t :: %__MODULE__{ + value: String.t(), + owner: User.t(), + application: OAuth.Application.t(), + scopes: OAuth.Application.scopes(), + expires_at: DateTime.t(), + type: :access | :refresh, + refresh_token: t() + } + + schema "oauth_tokens" do + field :value, :string + belongs_to :owner, Teiserver.Account.User, primary_key: true + belongs_to :application, OAuth.Application, primary_key: true + field :scopes, {:array, :string} + field :expires_at, :utc_datetime + field :type, Ecto.Enum, values: [:access, :refresh] + belongs_to :refresh_token, __MODULE__ + + timestamps() + end + + def changeset(token, attrs) do + attrs = attrs |> uniq_lists(~w(scopes)a) + + token + |> cast(attrs, [:value, :owner_id, :application_id, :scopes, :expires_at, :type]) + |> cast_assoc(:refresh_token) + |> validate_required([:value, :owner_id, :application_id, :scopes, :expires_at, :type]) + |> Ecto.Changeset.validate_subset(:scopes, OAuth.Application.allowed_scopes()) + end +end diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs index 927912c90..5fd9d6a42 100644 --- a/priv/repo/migrations/20240608144340_oauth_setup.exs +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -24,5 +24,19 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do end create_if_not_exists unique_index(:oauth_codes, [:value]) + create_if_not_exists table(:oauth_tokens, comment: "auth tokens and refresh") do + add :value, :string, null: false + add :owner_id, references(:account_users, on_delete: :delete_all), null: false + add :application_id, references(:oauth_applications, on_delete: :delete_all), null: false + add :scopes, {:array, :string}, null: false + add :expires_at, :utc_datetime, null: false + add :type, :string, null: false + # we should create a new refresh token when deleting an auth token and vice versa + add :refresh_token_id, references(:oauth_tokens) + + timestamps(type: :utc_datetime) + end + create_if_not_exists unique_index(:oauth_tokens, [:value]) + end end diff --git a/test/teiserver/o_auth/token_test.exs b/test/teiserver/o_auth/token_test.exs new file mode 100644 index 000000000..db6a45aa5 --- /dev/null +++ b/test/teiserver/o_auth/token_test.exs @@ -0,0 +1,57 @@ +defmodule Teiserver.OAuth.TokenTest do + use Teiserver.DataCase, async: true + alias Teiserver.OAuth + + setup do + user = Teiserver.TeiserverTestLib.new_user() + + {:ok, app} = + OAuth.create_application(%{ + name: "Testing app", + uid: "test_app_uid", + owner_id: user.id, + scopes: ["tachyon.lobby"] + }) + + {:ok, user: user, app: app} + end + + test "can create a token directly", %{user: user, app: app} do + assert {:ok, token} = OAuth.create_token(user, app) + assert token.type == :access + assert token.refresh_token.type == :refresh + assert token.scopes == app.scopes + end + + test "can get valid token", %{user: user, app: app} do + assert {:ok, new_token} = OAuth.create_token(user, app) + assert {:ok, token} = OAuth.get_valid_token(new_token.value) + assert {:error, :no_token} = OAuth.get_valid_token(nil) + assert token.id == new_token.id + assert Ecto.assoc_loaded?(token.application) + end + + test "cannot get expired token", %{user: user, app: app} do + yesterday = Timex.shift(Timex.now(), days: -1) + assert {:ok, token} = OAuth.create_token(user, app, now: yesterday) + assert {:error, :expired} = OAuth.get_valid_token(token.value) + end + + test "cannot use bearer token as refresh token", %{user: user, app: app} do + assert {:ok, token} = OAuth.create_token(user, app) + assert {:error, :invalid_token} = OAuth.refresh_token(token) + end + + test "can refresh a token", %{user: user, app: app} do + assert {:ok, token} = OAuth.create_token(user, app) + assert {:ok, new_token} = OAuth.refresh_token(token.refresh_token) + + # the previous token and its refresh token should have been invalidated + assert {:error, :no_token} = OAuth.get_valid_token(token.value) + assert {:error, :no_token} = OAuth.get_valid_token(token.refresh_token.value) + + # the newly created token is valid + assert {:ok, new_token_fresh} = OAuth.get_valid_token(new_token.value) + assert new_token_fresh.id == new_token.id + end +end From f975dc99e09de937ab9a2a146dfdd33710903c35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Wed, 12 Jun 2024 20:00:11 +0100 Subject: [PATCH 04/27] Refactor: composable joins --- .../o_auth/queries/application_query.ex | 10 +++++++++ lib/teiserver/o_auth/queries/token_query.ex | 22 ++++--------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/lib/teiserver/o_auth/queries/application_query.ex b/lib/teiserver/o_auth/queries/application_query.ex index 0df1a29c7..c414ccb59 100644 --- a/lib/teiserver/o_auth/queries/application_query.ex +++ b/lib/teiserver/o_auth/queries/application_query.ex @@ -21,4 +21,14 @@ defmodule Teiserver.OAuth.ApplicationQueries do from e in query, where: e.uid == ^uid end + + def join_application(query, name \\ :application) do + if has_named_binding?(query, :application) do + query + else + from token in query, + join: app in assoc(token, ^name), + as: :application + end + end end diff --git a/lib/teiserver/o_auth/queries/token_query.ex b/lib/teiserver/o_auth/queries/token_query.ex index b37c027e9..4a56f6899 100644 --- a/lib/teiserver/o_auth/queries/token_query.ex +++ b/lib/teiserver/o_auth/queries/token_query.ex @@ -9,7 +9,10 @@ defmodule Teiserver.OAuth.TokenQueries do def get_token(nil), do: nil def get_token(value) do - base_query() |> where_token(value) |> with_app() |> Repo.one() + base_query() + |> where_token(value) + |> preload(:application) + |> Repo.one() end def base_query() do @@ -23,23 +26,6 @@ defmodule Teiserver.OAuth.TokenQueries do where: e.value == ^value end - @doc """ - ensure the related application is loaded - """ - def with_app(query) do - query = - if has_named_binding?(query, :app) do - query - else - from token in query, - join: app in assoc(token, :application), - as: :app - end - - from [app: app] in query, - preload: [application: app] - end - @doc """ given a refresh token, deletes it and its potential associated token """ From a8a7714e36a2b84f82b4ba7eec46fbd543cc5f0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Wed, 12 Jun 2024 20:28:03 +0100 Subject: [PATCH 05/27] Exchange code for token logic --- lib/teiserver/o_auth.ex | 69 +++++++++++++++---- lib/teiserver/o_auth/schemas/code.ex | 21 +++++- .../migrations/20240608144340_oauth_setup.exs | 1 + test/teiserver/o_auth/code_test.exs | 23 ++++++- 4 files changed, 95 insertions(+), 19 deletions(-) diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index 663a558a2..89856f6fb 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -25,28 +25,35 @@ defmodule Teiserver.OAuth do Create an authorization token for the given user and application. The token scopes are the same as the application """ - @spec create_code(User.t() | T.userid(), Application.t(), options()) :: + @spec create_code( + User.t() | T.userid(), + Application.t(), + %{redirect_uri: String.t()}, + options() + ) :: {:ok, Code.t()} | {:error, Ecto.Changeset.t()} - def create_code(user, application, opts \\ []) + def create_code(user, application, params \\ %{}, opts \\ []) - def create_code(%User{} = user, application, opts) do - create_code(user.id, application, opts) + def create_code(%User{} = user, application, params, opts) do + create_code(user.id, application, params, opts) end - def create_code(user, application, opts) when is_map(user) do - create_code(user.id, application, opts) + def create_code(user, application, params, opts) when is_map(user) do + create_code(user.id, application, params, opts) end - def create_code(user_id, application, opts) do + def create_code(user_id, application, params, opts) do now = Keyword.get(opts, :now, Timex.now()) - attrs = %{ - value: Base.hex_encode32(:crypto.strong_rand_bytes(32)), - owner_id: user_id, - application_id: application.id, - scopes: application.scopes, - expires_at: Timex.add(now, Timex.Duration.from_minutes(5)) - } + attrs = + %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32)), + owner_id: user_id, + application_id: application.id, + scopes: application.scopes, + expires_at: Timex.add(now, Timex.Duration.from_minutes(5)) + } + |> Map.put(:redirect_uri, Map.get(params, :redirect_uri)) %Code{} |> Code.changeset(attrs) @@ -76,7 +83,11 @@ defmodule Teiserver.OAuth do end end - @spec create_token(User.t() | T.userid(), Application.t(), options()) :: + @spec create_token( + User.t() | T.userid(), + %{id: integer(), scopes: Application.scopes()}, + options() + ) :: {:ok, Token.t()} | {:error, Ecto.Changeset.t()} def create_token(user_id, application, opts \\ []) @@ -143,6 +154,34 @@ defmodule Teiserver.OAuth do end end + @doc """ + Given an authorization code, creates and return an authentication token + (and its associated refresh token). + """ + @spec exchange_code(Code.t(), String.t() | nil, options()) :: + {:ok, Token.t()} | {:error, term()} + def exchange_code(code, redirect_uri \\ nil, opts \\ []) do + now = Keyword.get(opts, :now, Timex.now()) + + cond do + expired?(code, now) -> + {:error, :expired} + + code.redirect_uri != redirect_uri -> + {:error, :redirect_uri_mismatch} + + true -> + Repo.transaction(fn -> + Repo.delete!(code) + + {:ok, token} = + create_token(code.owner_id, %{id: code.application_id, scopes: code.scopes}, opts) + + token + end) + end + end + @spec refresh_token(Token.t(), options()) :: {:ok, Token.t()} | {:error, term()} def refresh_token(token, opts \\ []) diff --git a/lib/teiserver/o_auth/schemas/code.ex b/lib/teiserver/o_auth/schemas/code.ex index 8644a2332..5292091d7 100644 --- a/lib/teiserver/o_auth/schemas/code.ex +++ b/lib/teiserver/o_auth/schemas/code.ex @@ -9,7 +9,8 @@ defmodule Teiserver.OAuth.Code do owner: User.t(), application: OAuth.Application.t(), scopes: OAuth.Application.scopes(), - expires_at: DateTime.t() + expires_at: DateTime.t(), + redirect_uri: String.t() | nil } schema "oauth_codes" do @@ -18,6 +19,7 @@ defmodule Teiserver.OAuth.Code do belongs_to :application, OAuth.Application, primary_key: true field :scopes, {:array, :string} field :expires_at, :utc_datetime + field :redirect_uri, :string timestamps() end @@ -26,8 +28,21 @@ defmodule Teiserver.OAuth.Code do attrs = attrs |> uniq_lists(~w(scopes)a) code - |> cast(attrs, [:value, :owner_id, :application_id, :scopes, :expires_at]) - |> validate_required([:value, :owner_id, :application_id, :scopes, :expires_at]) + |> cast(attrs, [ + :value, + :owner_id, + :application_id, + :scopes, + :expires_at, + :redirect_uri + ]) + |> validate_required([ + :value, + :owner_id, + :application_id, + :scopes, + :expires_at + ]) |> Ecto.Changeset.validate_subset(:scopes, OAuth.Application.allowed_scopes()) end end diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs index 5fd9d6a42..0ea03a11a 100644 --- a/priv/repo/migrations/20240608144340_oauth_setup.exs +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -19,6 +19,7 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do add :application_id, references(:oauth_applications, on_delete: :delete_all), null: false add :scopes, {:array, :string}, null: false add :expires_at, :utc_datetime, null: false + add :redirect_uri, :string timestamps(type: :utc_datetime) end diff --git a/test/teiserver/o_auth/code_test.exs b/test/teiserver/o_auth/code_test.exs index 04b96efc5..d50c2c486 100644 --- a/test/teiserver/o_auth/code_test.exs +++ b/test/teiserver/o_auth/code_test.exs @@ -24,7 +24,28 @@ defmodule Teiserver.OAuth.CodeTest do test "cannot retrieve expired code", %{user: user, app: app} do yesterday = Timex.shift(Timex.now(), days: -1) - assert {:ok, code} = OAuth.create_code(user, app, now: yesterday) + assert {:ok, code} = OAuth.create_code(user, app, %{}, now: yesterday) assert {:error, :expired} = OAuth.get_valid_code(code.value) end + + test "can exchange valid code for token", %{user: user, app: app} do + assert {:ok, code} = OAuth.create_code(user, app) + assert {:ok, token} = OAuth.exchange_code(code) + assert token.scopes == code.scopes + assert token.owner_id == user.id + # the code is now consumed and not available anymore + assert {:error, :no_code} = OAuth.get_valid_code(code.value) + end + + test "redirect uri must be provided and match", %{user: user, app: app} do + assert {:ok, code} = OAuth.create_code(user, app, %{redirect_uri: "http://127.0.0.1/foo"}) + assert {:error, :redirect_uri_mismatch} = OAuth.exchange_code(code) + assert {:error, :redirect_uri_mismatch} = OAuth.exchange_code(code, "http://another.url/") + end + + test "cannot exchange expired code for token", %{user: user, app: app} do + yesterday = Timex.shift(Timex.now(), days: -1) + assert {:ok, code} = OAuth.create_code(user, app, %{}, now: yesterday) + assert {:error, :expired} = OAuth.exchange_code(code) + end end From 554b1404ca0f2f5184f6f1f72d0896f0d4d03ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Thu, 13 Jun 2024 19:49:46 +0100 Subject: [PATCH 06/27] Implement PKCE references: https://oauth.net/2/pkce/ and https://www.rfc-editor.org/rfc/rfc7636 --- lib/teiserver/o_auth.ex | 73 ++++++++++++++----- lib/teiserver/o_auth/schemas/code.ex | 14 +++- .../migrations/20240608144340_oauth_setup.exs | 2 + test/teiserver/o_auth/code_test.exs | 66 ++++++++++++++--- 4 files changed, 121 insertions(+), 34 deletions(-) diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index 89856f6fb..0b30dc961 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -27,33 +27,41 @@ defmodule Teiserver.OAuth do """ @spec create_code( User.t() | T.userid(), - Application.t(), - %{redirect_uri: String.t()}, + %{ + id: integer(), + scopes: Application.scopes(), + redirect_uri: String.t(), + challenge: String.t(), + challenge_method: String.t() + }, options() ) :: {:ok, Code.t()} | {:error, Ecto.Changeset.t()} - def create_code(user, application, params \\ %{}, opts \\ []) + def create_code(user, attrs, opts \\ []) - def create_code(%User{} = user, application, params, opts) do - create_code(user.id, application, params, opts) + def create_code(%User{} = user, attrs, opts) do + create_code(user.id, attrs, opts) end - def create_code(user, application, params, opts) when is_map(user) do - create_code(user.id, application, params, opts) + def create_code(user, attrs, opts) when is_map(user) do + create_code(user.id, attrs, opts) end - def create_code(user_id, application, params, opts) do + def create_code(user_id, attrs, opts) do now = Keyword.get(opts, :now, Timex.now()) - attrs = - %{ - value: Base.hex_encode32(:crypto.strong_rand_bytes(32)), - owner_id: user_id, - application_id: application.id, - scopes: application.scopes, - expires_at: Timex.add(now, Timex.Duration.from_minutes(5)) - } - |> Map.put(:redirect_uri, Map.get(params, :redirect_uri)) + # don't do any validation on the challenge yet, this is done when exchanging + # the code for a token + attrs = %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32)), + owner_id: user_id, + application_id: attrs.id, + scopes: attrs.scopes, + expires_at: Timex.add(now, Timex.Duration.from_minutes(5)), + redirect_uri: Map.get(attrs, :redirect_uri), + challenge: Map.get(attrs, :challenge), + challenge_method: Map.get(attrs, :challenge_method) + } %Code{} |> Code.changeset(attrs) @@ -158,9 +166,9 @@ defmodule Teiserver.OAuth do Given an authorization code, creates and return an authentication token (and its associated refresh token). """ - @spec exchange_code(Code.t(), String.t() | nil, options()) :: + @spec exchange_code(Code.t(), String.t(), String.t() | nil, options()) :: {:ok, Token.t()} | {:error, term()} - def exchange_code(code, redirect_uri \\ nil, opts \\ []) do + def exchange_code(code, verifier, redirect_uri \\ nil, opts \\ []) do now = Keyword.get(opts, :now, Timex.now()) cond do @@ -170,6 +178,9 @@ defmodule Teiserver.OAuth do code.redirect_uri != redirect_uri -> {:error, :redirect_uri_mismatch} + not code_verified?(code, verifier) -> + {:error, :code_verification_failed} + true -> Repo.transaction(fn -> Repo.delete!(code) @@ -182,6 +193,30 @@ defmodule Teiserver.OAuth do end end + @spec code_verified?(Code.t(), String.t()) :: boolean() + defp code_verified?(%Code{challenge_method: :plain, challenge: challenge}, verifier) do + valid_verifier?(verifier) and :crypto.hash_equals(challenge, verifier) + end + + defp code_verified?(%Code{challenge_method: :S256, challenge: challenge}, verifier) do + with true <- valid_verifier?(verifier), + {:ok, challenge} <- Base.url_decode64(challenge, padding: false, ignore: :whitespace) do + hashed_verifier = :crypto.hash(:sha256, verifier) + :crypto.hash_equals(challenge, hashed_verifier) + else + _ -> + false + end + end + + defp code_verified?(_, _), do: false + + # A-Z, a-z, 0-9, and the punctuation characters -._~ + defp valid_verifier?(verifier) do + s = byte_size(verifier) + 43 <= s and s <= 128 and String.match?(verifier, ~r/[A-Za-z0-9\-._~]/) + end + @spec refresh_token(Token.t(), options()) :: {:ok, Token.t()} | {:error, term()} def refresh_token(token, opts \\ []) diff --git a/lib/teiserver/o_auth/schemas/code.ex b/lib/teiserver/o_auth/schemas/code.ex index 5292091d7..0eb5d14bb 100644 --- a/lib/teiserver/o_auth/schemas/code.ex +++ b/lib/teiserver/o_auth/schemas/code.ex @@ -10,7 +10,9 @@ defmodule Teiserver.OAuth.Code do application: OAuth.Application.t(), scopes: OAuth.Application.scopes(), expires_at: DateTime.t(), - redirect_uri: String.t() | nil + redirect_uri: String.t() | nil, + challenge: String.t() | nil, + challenge_method: :plain | :S256 | nil } schema "oauth_codes" do @@ -20,6 +22,8 @@ defmodule Teiserver.OAuth.Code do field :scopes, {:array, :string} field :expires_at, :utc_datetime field :redirect_uri, :string + field :challenge, :string + field :challenge_method, Ecto.Enum, values: [:plain, :S256] timestamps() end @@ -34,14 +38,18 @@ defmodule Teiserver.OAuth.Code do :application_id, :scopes, :expires_at, - :redirect_uri + :redirect_uri, + :challenge, + :challenge_method ]) |> validate_required([ :value, :owner_id, :application_id, :scopes, - :expires_at + :expires_at, + :challenge, + :challenge_method ]) |> Ecto.Changeset.validate_subset(:scopes, OAuth.Application.allowed_scopes()) end diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs index 0ea03a11a..cf1d82894 100644 --- a/priv/repo/migrations/20240608144340_oauth_setup.exs +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -20,6 +20,8 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do add :scopes, {:array, :string}, null: false add :expires_at, :utc_datetime, null: false add :redirect_uri, :string + add :challenge, :string + add :challenge_method, :string timestamps(type: :utc_datetime) end diff --git a/test/teiserver/o_auth/code_test.exs b/test/teiserver/o_auth/code_test.exs index d50c2c486..139dbf2c7 100644 --- a/test/teiserver/o_auth/code_test.exs +++ b/test/teiserver/o_auth/code_test.exs @@ -16,36 +16,78 @@ defmodule Teiserver.OAuth.CodeTest do {:ok, user: user, app: app} end + defp generate_challenge(method \\ :S256) do + # A-Z,a-z,0-9 and -._~ are authorized, but can't be bothered to cover all + # of that. hex encoding will fit + verifier = Base.hex_encode32(:crypto.strong_rand_bytes(40), padding: false) + + challenge = + case method do + :plain -> verifier + :S256 -> hash_verifier(verifier) + end + + {verifier, challenge, method} + end + + defp hash_verifier(verifier), + do: Base.url_encode64(:crypto.hash(:sha256, verifier), padding: false) + + defp create_code(user, app, opts \\ []) do + {verifier, challenge, method} = generate_challenge() + + attrs = %{ + id: app.id, + scopes: app.scopes, + redirect_uri: "http://some.host/a/path", + challenge: challenge, + challenge_method: method + } + + {:ok, code} = OAuth.create_code(user, attrs, opts) + {:ok, code, Map.put(attrs, :verifier, verifier)} + end + test "can get valid code", %{user: user, app: app} do - assert {:ok, code} = OAuth.create_code(user, app) + assert {:ok, code, _} = create_code(user, app) assert {:ok, ^code} = OAuth.get_valid_code(code.value) assert {:error, :no_code} = OAuth.get_valid_code(nil) end test "cannot retrieve expired code", %{user: user, app: app} do yesterday = Timex.shift(Timex.now(), days: -1) - assert {:ok, code} = OAuth.create_code(user, app, %{}, now: yesterday) + assert {:ok, code, _} = create_code(user, app, now: yesterday) assert {:error, :expired} = OAuth.get_valid_code(code.value) end test "can exchange valid code for token", %{user: user, app: app} do - assert {:ok, code} = OAuth.create_code(user, app) - assert {:ok, token} = OAuth.exchange_code(code) + assert {:ok, code, attrs} = create_code(user, app) + assert {:ok, token} = OAuth.exchange_code(code, attrs.verifier, attrs.redirect_uri) assert token.scopes == code.scopes assert token.owner_id == user.id # the code is now consumed and not available anymore assert {:error, :no_code} = OAuth.get_valid_code(code.value) end - test "redirect uri must be provided and match", %{user: user, app: app} do - assert {:ok, code} = OAuth.create_code(user, app, %{redirect_uri: "http://127.0.0.1/foo"}) - assert {:error, :redirect_uri_mismatch} = OAuth.exchange_code(code) - assert {:error, :redirect_uri_mismatch} = OAuth.exchange_code(code, "http://another.url/") - end - test "cannot exchange expired code for token", %{user: user, app: app} do yesterday = Timex.shift(Timex.now(), days: -1) - assert {:ok, code} = OAuth.create_code(user, app, %{}, now: yesterday) - assert {:error, :expired} = OAuth.exchange_code(code) + assert {:ok, code, attrs} = create_code(user, app, now: yesterday) + assert {:error, :expired} = OAuth.exchange_code(code, attrs.verifier) + end + + test "must use valid verifier", %{user: user, app: app} do + assert {:ok, code, attrs} = create_code(user, app) + no_match = Base.hex_encode32(:crypto.strong_rand_bytes(38), padding: false) + assert {:error, _} = OAuth.exchange_code(code, no_match) + + verifier = "TOO_SHORT" + challenge = hash_verifier(verifier) + {:ok, code} = OAuth.create_code(user, %{attrs | challenge: challenge}) + assert {:error, _} = OAuth.exchange_code(code, verifier) + + verifier = String.duplicate("a", 129) + challenge = hash_verifier(verifier) + {:ok, code} = OAuth.create_code(user, %{attrs | challenge: challenge}) + assert {:error, _} = OAuth.exchange_code(code, verifier) end end From 2654f6772b03ce4bfcd6f79ae3353fecd08fbf50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Fri, 14 Jun 2024 17:16:57 +0100 Subject: [PATCH 07/27] Nicer redirect uri on login Don't put a `?` symbol if there's no querystring --- lib/teiserver/account/error_handler.ex | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/teiserver/account/error_handler.ex b/lib/teiserver/account/error_handler.ex index 0509f4804..9d02066b5 100644 --- a/lib/teiserver/account/error_handler.ex +++ b/lib/teiserver/account/error_handler.ex @@ -6,7 +6,12 @@ defmodule Teiserver.Account.ErrorHandler do @impl Guardian.Plug.ErrorHandler def auth_error(conn, {:unauthenticated, _reason}, _opts) do - redirect_to = "#{conn.request_path}?#{conn.query_string}" + redirect_to = + if conn.query_string != nil && conn.query_string != "" do + "#{conn.request_path}?#{conn.query_string}" + else + "#{conn.request_path}" + end conn |> put_resp_cookie("_redirect_to", redirect_to, sign: true, max_age: 60 * 5) From 7241336ba787c104bb162e70d4062511be29f0bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sat, 15 Jun 2024 07:56:34 +0100 Subject: [PATCH 08/27] Validate redirection uris --- lib/teiserver/o_auth.ex | 59 ++++++++++++++- test/teiserver/o_auth/application_test.exs | 85 +++++++++++++++++++++- 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index 0b30dc961..ed591efc4 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -21,6 +21,62 @@ defmodule Teiserver.OAuth do @type option :: {:now, DateTime.t()} @type options :: [option] + @spec get_application_by_uid(Application.app_id()) :: Application.t() | nil + defdelegate get_application_by_uid(uid), to: ApplicationQueries + + @doc """ + Given an application and a potential encoded redirect_uri, decode the uri + and validate it against the registered redirect uris for the application. + """ + @spec get_redirect_uri(Application.t(), String.t()) :: {:ok, URI.t()} | {:error, term()} + def get_redirect_uri(app, encoded_uri) do + parsed = encoded_uri |> URI.decode_www_form() |> URI.parse() + + # https://www.rfc-editor.org/rfc/rfc6749.html#section-3.1.2 + cond do + not is_nil(parsed.fragment) -> {:error, "Fragment must not be included"} + Enum.any?(app.redirect_uris, fn app_uri -> equal_uri?(app_uri, parsed) end) -> {:ok, parsed} + true -> {:error, "No matching redirect uri found"} + end + end + + # Compares two uris, but only the scheme, host and path. + # Redirect URI for oauth application shouldn't have query string, and the client + # can add some which must be preserved. + # If the host is localhost, either "localhost", the ipv4 or ipv6 notations are allowed + # If the host is localhost, the port isn't compared since client have the freedom + # of choosing it (usually it's whatever they can bind). + defp equal_uri?(%URI{} = uri1, %URI{} = uri2) do + cond do + uri1.scheme != uri2.scheme || uri1.path != uri2.path -> + false + + localhost?(uri1) and localhost?(uri2) -> + true + + not localhost?(uri1) and not localhost?(uri2) -> + uri1.host == uri2.host && uri1.port == uri2.port + + # one is localhost while the other isn't + true -> + false + end + end + + defp equal_uri?(uri1, uri2) do + equal_uri?(URI.parse(uri1), uri2) + end + + # https://www.rfc-editor.org/rfc/rfc8252#section-7.3 + # although `localhost` is not recommended, it is allowed, see: + # and https://www.rfc-editor.org/rfc/rfc8252#section-8.3 + defp localhost?(%URI{} = uri), do: localhost?(uri.host) + defp localhost?("localhost"), do: true + defp localhost?("127.0.0.1"), do: true + defp localhost?("::1"), do: true + defp localhost?("0:0:0:0:0:0:0:1"), do: true + defp localhost?(_), do: false + @doc """ Create an authorization token for the given user and application. The token scopes are the same as the application @@ -245,9 +301,6 @@ defmodule Teiserver.OAuth do end end - @spec get_application_by_uid(Application.app_id()) :: Application.t() | nil - defdelegate get_application_by_uid(uid), to: ApplicationQueries - defp expired?(obj, now) do Timex.after?(now, Map.fetch!(obj, :expires_at)) end diff --git a/test/teiserver/o_auth/application_test.exs b/test/teiserver/o_auth/application_test.exs index 6fa18321d..021d55fb7 100644 --- a/test/teiserver/o_auth/application_test.exs +++ b/test/teiserver/o_auth/application_test.exs @@ -5,13 +5,14 @@ defmodule Teiserver.OAuth.ApplicationTest do test "reject unknown scopes at creation" do user = Teiserver.TeiserverTestLib.new_user() - assert {:error, changeset} = + assert {:error, changeset} = OAuth.create_application(%{ name: "Testing app", uid: "test_app_uid", owner_id: user.id, scopes: ["lol"] }) + assert Keyword.fetch!(changeset.errors, :scopes) end @@ -48,4 +49,86 @@ defmodule Teiserver.OAuth.ApplicationTest do assert OAuth.get_application_by_uid("u_wot_mate?") == nil assert OAuth.get_application_by_uid(nil) == nil end + + defp valid_attrs(user) do + %{ + name: "Testing app", + uid: "test_app_uid", + owner_id: user.id, + scopes: ["tachyon.lobby"] + } + end + + test "get redirect uri" do + user = Teiserver.TeiserverTestLib.new_user() + + {:ok, app} = + OAuth.create_application( + Map.put(valid_attrs(user), :redirect_uris, ["http://foo.bar/callback/path"]) + ) + + assert {:ok, uri} = OAuth.get_redirect_uri(app, "http://foo.bar/callback/path?state=xyz") + # ensure query string is preserved + assert URI.to_string(uri) == "http://foo.bar/callback/path?state=xyz" + end + + test "redirect uri validation" do + user = Teiserver.TeiserverTestLib.new_user() + + {:ok, app} = + OAuth.create_application( + Map.put(valid_attrs(user), :redirect_uris, ["http://foo.bar/callback/path"]) + ) + + # fragments aren't allowed + assert {:error, _} = OAuth.get_redirect_uri(app, "http://foo.bar/callback/path#fragment") + + assert {:error, _} = OAuth.get_redirect_uri(app, "http://another.host/callback/path") + assert {:error, _} = OAuth.get_redirect_uri(app, "http://foo.bar/different/path") + end + + test "validate the various ways to handle localhost" do + user = Teiserver.TeiserverTestLib.new_user() + + {:ok, app} = + OAuth.create_application( + Map.put(valid_attrs(user), :redirect_uris, ["http://localhost/callback/path"]) + ) + + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://localhost/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://localhost:7689/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://127.0.0.1/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://127.0.0.1:7689/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://[::1]/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://[::1]:7689/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://[0:0:0:0:0:0:0:1]/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://[0:0:0:0:0:0:0:1]:7689/callback/path") + end + + test "ignore ports for localhost only" do + user = Teiserver.TeiserverTestLib.new_user() + + uri = URI.parse("http://some.host:7890/callback/path") + + {:ok, app} = + OAuth.create_application(Map.put(valid_attrs(user), :redirect_uris, [URI.to_string(uri)])) + + assert {:error, _} = OAuth.get_redirect_uri(app, "http://some.host:1234/callback/path") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://some.host:7890/callback/path") + end + + test "can validate against multiple registered uris" do + user = Teiserver.TeiserverTestLib.new_user() + + {:ok, app} = + OAuth.create_application( + Map.put(valid_attrs(user), :redirect_uris, [ + "http://some.host/callback", + "http://another.host/another/callback" + ]) + ) + + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://some.host/callback") + assert {:ok, _} = OAuth.get_redirect_uri(app, "http://another.host/another/callback") + end end From 141eafed11cfe4e2e1e9778ded1ebba8c97cf8f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sat, 15 Jun 2024 10:30:14 +0100 Subject: [PATCH 09/27] Http interface for authorization code Screen for the user to grant access to the given client and then redirect with a freshly generated authorization code. --- .../o_auth/authorize_controller.ex | 131 ++++++++++++++++++ lib/teiserver_web/router.ex | 6 + .../o_auth/authorize/authorize.html.heex | 37 +++++ .../o_auth/authorize/bad_request.html.heex | 16 +++ .../views/o_auth/authorize_view.ex | 3 + .../o_auth/authorize_controller_test.exs | 106 ++++++++++++++ 6 files changed, 299 insertions(+) create mode 100644 lib/teiserver_web/controllers/o_auth/authorize_controller.ex create mode 100644 lib/teiserver_web/templates/o_auth/authorize/authorize.html.heex create mode 100644 lib/teiserver_web/templates/o_auth/authorize/bad_request.html.heex create mode 100644 lib/teiserver_web/views/o_auth/authorize_view.ex create mode 100644 test/teiserver_web/controllers/o_auth/authorize_controller_test.exs diff --git a/lib/teiserver_web/controllers/o_auth/authorize_controller.ex b/lib/teiserver_web/controllers/o_auth/authorize_controller.ex new file mode 100644 index 000000000..3c9379033 --- /dev/null +++ b/lib/teiserver_web/controllers/o_auth/authorize_controller.ex @@ -0,0 +1,131 @@ +defmodule TeiserverWeb.OAuth.AuthorizeController do + use TeiserverWeb, :controller + + alias Teiserver.OAuth + + def authorize(conn, params) when not is_map_key(params, "client_id") do + bad_request(conn, "missing client_id") + end + + def authorize(conn, %{"client_id" => client_id} = params) do + case OAuth.get_application_by_uid(client_id) do + nil -> + bad_request(conn, "invalid client_id") + + app -> + case OAuth.get_redirect_uri(app, Map.get(conn.params, "redirect_uri")) do + {:error, _err} -> + bad_request(conn, "invalid redirection uri") + + {:ok, redir_uri} -> + reject_uri = + error_redirect_uri( + conn, + redir_uri, + "access_denied", + "user refused to authorize application" + ) + + conn + |> render("authorize.html", + app_name: app.name, + params: params, + reject_uri: reject_uri + ) + end + end + end + + def authorize(conn, _params) do + conn |> render("bad_request.html") + end + + def generate_code(conn, params) when not is_map_key(params, "client_id") do + bad_request(conn, "missing client_id") + end + + def generate_code(conn, params) when not is_map_key(params, "redirect_uri") do + bad_request(conn, "missing redirect_uri") + end + + def generate_code(conn, %{"client_id" => client_id, "redirect_uri" => redirect_uri} = params) do + case OAuth.get_application_by_uid(client_id) do + nil -> + bad_request(conn, "invalid client_id") + + app -> + case OAuth.get_redirect_uri(app, redirect_uri) do + {:error, _} -> bad_request(conn, "invalid redirection url") + {:ok, redir_url} -> do_generate_code(conn, app, redir_url, params) + end + end + end + + defp do_generate_code(conn, app, redir_url, params) do + cond do + Map.get(params, "response_type") != "code" -> + error_redirect(conn, redir_url, "unsupported_response_type", "only code is supported") + + Map.get(params, "code_challenge_method") != "S256" -> + error_redirect(conn, redir_url, "invalid_request", "only S256 is supported") + + Map.get(params, "code_challenge") == nil -> + error_redirect(conn, redir_url, "invalid_request", "a code challenge must be provided") + + true -> + code_params = %{ + id: app.id, + redirect_uri: URI.to_string(redir_url), + scopes: app.scopes, + challenge: Map.get(params, "code_challenge"), + challenge_method: "S256" + } + + case OAuth.create_code(conn.assigns.current_user, code_params) do + {:error, _} -> + error_redirect(conn, redir_url, "server_error", "something went wrong") + + {:ok, code} -> + query = + URI.decode_query(redir_url.query || "") + |> Map.put(:code, code.value) + |> Map.merge(Map.take(params, ["state"])) + |> URI.encode_query() + + conn |> redirect(external: URI.to_string(%{redir_url | query: query})) + end + end + end + + # If the server can validate the client id and the redirection url + # it should use the redirect url with an added query string to indicate failure + # otherwise notify the user of the error. See + # https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.2.1 + defp bad_request(conn, reason) do + conn + |> put_status(400) + |> render("bad_request.html", reason: reason) + end + + defp error_redirect_uri(conn, %URI{} = redir_url, error, description) do + final_query = + URI.decode_query(redir_url.query || "") + |> Map.put(:error, error) + |> Map.put(:error_description, description) + |> then(fn q -> + case Map.get(conn.params, "state") do + nil -> q + st -> Map.put(q, :state, st) + end + end) + |> URI.encode_query() + + URI.to_string(%{redir_url | query: final_query}) + end + + defp error_redirect(conn, redir_url, error, description) do + uri = error_redirect_uri(conn, redir_url, error, description) + + conn |> redirect(external: uri) + end +end diff --git a/lib/teiserver_web/router.ex b/lib/teiserver_web/router.ex index e73d3e614..5d2bcd794 100644 --- a/lib/teiserver_web/router.ex +++ b/lib/teiserver_web/router.ex @@ -588,6 +588,12 @@ defmodule TeiserverWeb.Router do end end + scope "/oauth/", TeiserverWeb.OAuth do + pipe_through([:browser, :app_layout, :protected]) + get("/authorize", AuthorizeController, :authorize) + post("/authorize", AuthorizeController, :generate_code) + end + scope "/admin", TeiserverWeb.Admin do pipe_through([:live_browser, :protected]) diff --git a/lib/teiserver_web/templates/o_auth/authorize/authorize.html.heex b/lib/teiserver_web/templates/o_auth/authorize/authorize.html.heex new file mode 100644 index 000000000..a687b8cb2 --- /dev/null +++ b/lib/teiserver_web/templates/o_auth/authorize/authorize.html.heex @@ -0,0 +1,37 @@ +
+
+
+
+

+ Authorize +

+
+ +
+

+ The application <%= @app_name %> would like to have access to + your Beyond All Reason account. +

+ + <% keys_to_copy = + ~w(client_id response_type code_challenge code_challenge_method redirect_uri state) %> +
+ + <%= for key <- keys_to_copy do %> + + <% end %> + +
+
+ + + +
+
+
+
diff --git a/lib/teiserver_web/templates/o_auth/authorize/bad_request.html.heex b/lib/teiserver_web/templates/o_auth/authorize/bad_request.html.heex new file mode 100644 index 000000000..70a5a67c3 --- /dev/null +++ b/lib/teiserver_web/templates/o_auth/authorize/bad_request.html.heex @@ -0,0 +1,16 @@ +
+
+
+
+

+ Bad request: <%= @reason %> +

+
+
+
+
diff --git a/lib/teiserver_web/views/o_auth/authorize_view.ex b/lib/teiserver_web/views/o_auth/authorize_view.ex new file mode 100644 index 000000000..3326d2a06 --- /dev/null +++ b/lib/teiserver_web/views/o_auth/authorize_view.ex @@ -0,0 +1,3 @@ +defmodule TeiserverWeb.OAuth.AuthorizeView do + use TeiserverWeb, :view +end diff --git a/test/teiserver_web/controllers/o_auth/authorize_controller_test.exs b/test/teiserver_web/controllers/o_auth/authorize_controller_test.exs new file mode 100644 index 000000000..76daf2ab9 --- /dev/null +++ b/test/teiserver_web/controllers/o_auth/authorize_controller_test.exs @@ -0,0 +1,106 @@ +defmodule TeiserverWeb.OAuth.AuthorizeControllerTest do + use TeiserverWeb.ConnCase + alias Central.Helpers.GeneralTestLib + alias Teiserver.OAuth + + setup do + {:ok, data} = + GeneralTestLib.conn_setup(Teiserver.TeiserverTestLib.player_permissions()) + |> Teiserver.TeiserverTestLib.conn_setup() + + {:ok, app} = + OAuth.create_application(%{ + name: "testing app", + uid: "test_app_uid", + owner_id: data[:user].id, + scopes: ["tachyon.lobby"], + redirect_uris: ["http://127.0.0.1:6789/oauth/callback"] + }) + + {:ok, Keyword.put(data, :app, app)} + end + + describe "authorize" do + test "must provide client_id", %{conn: conn} do + assert get(conn, ~p"/oauth/authorize").status == 400 + end + + test "must provide valid client_id", %{conn: conn} do + assert get(conn, ~p"/oauth/authorize?client_id=random_client").status == 400 + end + + test "get auth screen with valid client_id", %{conn: conn, app: app} do + redir_uri = "http://127.0.0.1/oauth/callback" + query = %{client_id: app.uid, redirect_uri: redir_uri} + resp = get(conn, ~p"/oauth/authorize?#{query}") + assert html_response(resp, 200) =~ app.name + end + end + + describe "generate code" do + test "get redirected with a code", %{conn: conn, app: app} do + data = %{ + client_id: app.uid, + response_type: "code", + code_challenge: "blah", + code_challenge_method: "S256", + redirect_uri: hd(app.redirect_uris), + state: "some random state" + } + + resp = post(conn, ~p"/oauth/authorize", data) + + assert redired = redirected_to(resp, 302) + parsed = URI.parse(redired) + + assert URI.to_string(%{parsed | query: nil}) == hd(app.redirect_uris) + query = URI.decode_query(parsed.query) + assert query["code"] != nil + assert query["state"] == data[:state] + {:ok, code} = OAuth.get_valid_code(query["code"]) + assert code.redirect_uri == data.redirect_uri + end + + test "must provide client_id and redirect_uri", %{conn: conn, app: app} do + data = %{ + client_id: app.uid, + response_type: "code", + code_challenge: "blah", + code_challenge_method: "S256", + redirect_uri: hd(app.redirect_uris), + state: "some random state" + } + + resp = post(conn, ~p"/oauth/authorize", Map.drop(data, [:client_id])) + assert html_response(resp, 400) =~ "missing client_id" + + resp = post(conn, ~p"/oauth/authorize", Map.drop(data, [:redirect_uri])) + assert html_response(resp, 400) =~ "missing redirect_uri" + + resp = post(conn, ~p"/oauth/authorize", Map.drop(data, [:redirect_uri, :client_id])) + assert resp.status == 400 + + resp = post(conn, ~p"/oauth/authorize", %{data | client_id: "lolnope"}) + assert html_response(resp, 400) =~ "invalid client_id" + end + + test "redirected for invalid response type", %{conn: conn, app: app} do + data = %{ + client_id: app.uid, + response_type: "lolnope", + code_challenge: "blah", + code_challenge_method: "S256", + redirect_uri: hd(app.redirect_uris), + state: "some random state" + } + + resp = post(conn, ~p"/oauth/authorize", data) + assert redired = redirected_to(resp, 302) + parsed = URI.parse(redired).query + assert String.starts_with?(redired, data.redirect_uri) + + assert %{"error" => "unsupported_response_type", "state" => "some random state"} = + URI.decode_query(parsed) + end + end +end From 793a5e33d6ea087b4e1a59eacc96941b5564c119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sat, 15 Jun 2024 18:46:36 +0100 Subject: [PATCH 10/27] Http interface to get token --- .../controllers/o_auth/code_controller.ex | 39 +++++++ lib/teiserver_web/router.ex | 7 ++ .../views/api/o_auth/code_view.ex | 16 +++ test/support/o_auth.ex | 45 ++++++++ .../o_auth/code_controller_test.exs | 108 ++++++++++++++++++ 5 files changed, 215 insertions(+) create mode 100644 lib/teiserver_web/controllers/o_auth/code_controller.ex create mode 100644 lib/teiserver_web/views/api/o_auth/code_view.ex create mode 100644 test/support/o_auth.ex create mode 100644 test/teiserver_web/controllers/o_auth/code_controller_test.exs diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex new file mode 100644 index 000000000..40a7967e1 --- /dev/null +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -0,0 +1,39 @@ +defmodule TeiserverWeb.OAuth.CodeController do + use TeiserverWeb, :controller + alias Teiserver.OAuth + + # https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.3 + @spec exchange_code(Conn.t(), %{}) :: Conn.t() + + def exchange_code(conn, %{"grant_type" => grant_type}) + when grant_type != "authorization_code" do + conn + |> put_status(400) + |> render(:error, error_description: "grant_type must be authorization_code") + end + + def exchange_code(conn, params) do + case Enum.find( + ["client_id", "code", "redirect_uri", "client_id", "code_verifier", "grant_type"], + fn key -> not Map.has_key?(params, key) end + ) do + nil -> + do_exchange_token(conn, params) + + missing_key -> + conn |> put_status(400) |> render(:error, error_description: "missing #{missing_key}") + end + end + + defp do_exchange_token(conn, params) do + with app when app != nil <- OAuth.get_application_by_uid(params["client_id"]), + {:ok, code} <- OAuth.get_valid_code(params["code"]), + true <- code.application_id == app.id, + {:ok, token} <- + OAuth.exchange_code(code, params["code_verifier"], params["redirect_uri"]) do + conn |> put_status(200) |> render(:token, token: token) + else + _ -> conn |> put_status(400) |> render(:error, error_description: "invalid request") + end + end +end diff --git a/lib/teiserver_web/router.ex b/lib/teiserver_web/router.ex index 5d2bcd794..124dc83fc 100644 --- a/lib/teiserver_web/router.ex +++ b/lib/teiserver_web/router.ex @@ -594,6 +594,13 @@ defmodule TeiserverWeb.Router do post("/authorize", AuthorizeController, :generate_code) end + # it's slightly weird to mix html and json api endpoints under the same prefix + # but I'd rather have all of the oauth stuff contained + scope "/oauth/", TeiserverWeb.OAuth do + pipe_through(:api) + post("/token", CodeController, :exchange_code) + end + scope "/admin", TeiserverWeb.Admin do pipe_through([:live_browser, :protected]) diff --git a/lib/teiserver_web/views/api/o_auth/code_view.ex b/lib/teiserver_web/views/api/o_auth/code_view.ex new file mode 100644 index 000000000..7bab7f806 --- /dev/null +++ b/lib/teiserver_web/views/api/o_auth/code_view.ex @@ -0,0 +1,16 @@ +defmodule TeiserverWeb.OAuth.CodeView do + def token(%{token: token}) do + expires_in = DateTime.diff(token.expires_at, DateTime.utc_now(), :second) + + %{ + access_token: token.value, + expires_in: expires_in, + refresh_token: token.refresh_token.value, + token_type: "Bearer" + } + end + + def error(conn) do + Map.take(conn, [:error_description]) |> Map.put("error", "invalid_request") + end +end diff --git a/test/support/o_auth.ex b/test/support/o_auth.ex new file mode 100644 index 000000000..7620378b4 --- /dev/null +++ b/test/support/o_auth.ex @@ -0,0 +1,45 @@ +defmodule Teiserver.Test.Support.OAuth do + alias Teiserver.OAuth + + @doc """ + utility to create an OAuth authorization code for the given user and application + """ + @spec create_code( + Teiserver.Account.User.t(), + %{ + id: integer(), + scopes: OAuth.Application.scopes(), + redirect_uri: String.t(), + challenge: String.t(), + challenge_method: String.t() + }, + OAuth.options() + ) :: {:ok, OAuth.Code.t(), map()} + def create_code(user, app, opts \\ []) do + {verifier, challenge, method} = generate_challenge() + + attrs = %{ + id: app.id, + scopes: app.scopes, + redirect_uri: "http://some.host/a/path", + challenge: challenge, + challenge_method: method + } + + {:ok, code} = OAuth.create_code(user, attrs, opts) + {:ok, code, Map.put(attrs, :verifier, verifier)} + end + + defp generate_challenge() do + # A-Z,a-z,0-9 and -._~ are authorized, but can't be bothered to cover all + # of that. hex encoding will fit + verifier = Base.hex_encode32(:crypto.strong_rand_bytes(40), padding: false) + + challenge = hash_verifier(verifier) + + {verifier, challenge, "S256"} + end + + def hash_verifier(verifier), + do: Base.url_encode64(:crypto.hash(:sha256, verifier), padding: false) +end diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs new file mode 100644 index 000000000..015c37434 --- /dev/null +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -0,0 +1,108 @@ +defmodule TeiserverWeb.OAuth.CodeControllerTest do + use TeiserverWeb.ConnCase + alias Central.Helpers.GeneralTestLib + alias Teiserver.OAuth + alias Teiserver.Test.Support.OAuth, as: OAuthTest + + defp get_valid_data(%{app: app, code: code, code_attrs: code_attrs}) do + %{ + grant_type: "authorization_code", + code: code.value, + redirect_uri: code.redirect_uri, + client_id: app.uid, + code_verifier: code_attrs.verifier + } + end + + defp setup_conn(_context) do + GeneralTestLib.conn_setup(Teiserver.TeiserverTestLib.player_permissions()) + |> Teiserver.TeiserverTestLib.conn_setup() + end + + defp setup_app(context) do + {:ok, app} = + OAuth.create_application(%{ + name: "testing app", + uid: "test_app_uid", + owner_id: context[:user].id, + scopes: ["tachyon.lobby"], + redirect_uris: ["http://127.0.0.1:6789/oauth/callback"] + }) + + %{app: app} + end + + defp setup_code(context) do + {:ok, code, attrs} = OAuthTest.create_code(context[:user], context[:app]) + + %{code: code, code_attrs: attrs} + end + + describe "exchange code for token" do + setup [:setup_conn, :setup_app, :setup_code] + + test "works with the right params", %{conn: conn} = setup_data do + data = get_valid_data(setup_data) + + resp = post(conn, ~p"/oauth/token", data) + json_resp = json_response(resp, 200) + assert is_binary(json_resp["access_token"]), "has access_token" + assert is_integer(json_resp["expires_in"]), "has expires_in" + assert is_binary(json_resp["refresh_token"]), "has refresh_token" + assert json_resp["token_type"] == "Bearer", "bearer token type" + + # within 5 seconds in case extremely slow test + assert_in_delta(json_resp["expires_in"], 60 * 30, 5, "valid for 30 minutes") + + resp2 = post(conn, ~p"/oauth/token", data) + + # code is now used up and invalid + assert json_response(resp2, 400) == %{ + "error_description" => "invalid request", + "error" => "invalid_request" + } + end + + test "must provide grant_type", %{conn: conn} = setup_data do + data = get_valid_data(setup_data) |> Map.drop([:grant_type]) + resp = post(conn, ~p"/oauth/token", data) + assert %{"error" => "invalid_request"} = json_response(resp, 400) + end + + test "must provide code", %{conn: conn} = setup_data do + data = get_valid_data(setup_data) |> Map.drop([:code]) + resp = post(conn, ~p"/oauth/token", data) + assert %{"error" => "invalid_request"} = json_response(resp, 400) + end + + test "must provide redirect_uri", %{conn: conn} = setup_data do + data = get_valid_data(setup_data) |> Map.drop([:redirect_uri]) + resp = post(conn, ~p"/oauth/token", data) + assert %{"error" => "invalid_request"} = json_response(resp, 400) + end + + test "must provide client_id", %{conn: conn} = setup_data do + data = get_valid_data(setup_data) |> Map.drop([:client_id]) + resp = post(conn, ~p"/oauth/token", data) + assert %{"error" => "invalid_request"} = json_response(resp, 400) + end + + test "client_id must match", %{conn: conn} = setup_data do + {:ok, other_app} = + OAuth.create_application(%{ + name: "another testing app", + uid: "another_test_app_uid", + owner_id: setup_data[:user].id, + scopes: ["tachyon.lobby"], + redirect_uris: ["http://127.0.0.1:6789/oauth/callback"] + }) + + data = get_valid_data(setup_data) |> Map.put(:client_id, other_app.uid) + resp = post(conn, ~p"/oauth/token", data) + assert %{"error" => "invalid_request"} = json_response(resp, 400) + end + + # Note: verifier code and redirect URI matching is tested in OAuth.exchange_code + # so they are omitted here + end +end From 8a70a37cd1de631376eae6d1adfa72441f164963 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sat, 15 Jun 2024 17:53:13 +0100 Subject: [PATCH 11/27] Refactor: utilities to generate authorization code --- test/teiserver/o_auth/code_test.exs | 47 +++++------------------------ 1 file changed, 8 insertions(+), 39 deletions(-) diff --git a/test/teiserver/o_auth/code_test.exs b/test/teiserver/o_auth/code_test.exs index 139dbf2c7..a5f632f91 100644 --- a/test/teiserver/o_auth/code_test.exs +++ b/test/teiserver/o_auth/code_test.exs @@ -1,6 +1,7 @@ defmodule Teiserver.OAuth.CodeTest do use Teiserver.DataCase, async: true alias Teiserver.OAuth + alias Teiserver.Test.Support.OAuth, as: OAuthTest setup do user = Teiserver.TeiserverTestLib.new_user() @@ -16,52 +17,20 @@ defmodule Teiserver.OAuth.CodeTest do {:ok, user: user, app: app} end - defp generate_challenge(method \\ :S256) do - # A-Z,a-z,0-9 and -._~ are authorized, but can't be bothered to cover all - # of that. hex encoding will fit - verifier = Base.hex_encode32(:crypto.strong_rand_bytes(40), padding: false) - - challenge = - case method do - :plain -> verifier - :S256 -> hash_verifier(verifier) - end - - {verifier, challenge, method} - end - - defp hash_verifier(verifier), - do: Base.url_encode64(:crypto.hash(:sha256, verifier), padding: false) - - defp create_code(user, app, opts \\ []) do - {verifier, challenge, method} = generate_challenge() - - attrs = %{ - id: app.id, - scopes: app.scopes, - redirect_uri: "http://some.host/a/path", - challenge: challenge, - challenge_method: method - } - - {:ok, code} = OAuth.create_code(user, attrs, opts) - {:ok, code, Map.put(attrs, :verifier, verifier)} - end - test "can get valid code", %{user: user, app: app} do - assert {:ok, code, _} = create_code(user, app) + assert {:ok, code, _} = OAuthTest.create_code(user, app) assert {:ok, ^code} = OAuth.get_valid_code(code.value) assert {:error, :no_code} = OAuth.get_valid_code(nil) end test "cannot retrieve expired code", %{user: user, app: app} do yesterday = Timex.shift(Timex.now(), days: -1) - assert {:ok, code, _} = create_code(user, app, now: yesterday) + assert {:ok, code, _} = OAuthTest.create_code(user, app, now: yesterday) assert {:error, :expired} = OAuth.get_valid_code(code.value) end test "can exchange valid code for token", %{user: user, app: app} do - assert {:ok, code, attrs} = create_code(user, app) + assert {:ok, code, attrs} = OAuthTest.create_code(user, app) assert {:ok, token} = OAuth.exchange_code(code, attrs.verifier, attrs.redirect_uri) assert token.scopes == code.scopes assert token.owner_id == user.id @@ -71,22 +40,22 @@ defmodule Teiserver.OAuth.CodeTest do test "cannot exchange expired code for token", %{user: user, app: app} do yesterday = Timex.shift(Timex.now(), days: -1) - assert {:ok, code, attrs} = create_code(user, app, now: yesterday) + assert {:ok, code, attrs} = OAuthTest.create_code(user, app, now: yesterday) assert {:error, :expired} = OAuth.exchange_code(code, attrs.verifier) end test "must use valid verifier", %{user: user, app: app} do - assert {:ok, code, attrs} = create_code(user, app) + assert {:ok, code, attrs} = OAuthTest.create_code(user, app) no_match = Base.hex_encode32(:crypto.strong_rand_bytes(38), padding: false) assert {:error, _} = OAuth.exchange_code(code, no_match) verifier = "TOO_SHORT" - challenge = hash_verifier(verifier) + challenge = OAuthTest.hash_verifier(verifier) {:ok, code} = OAuth.create_code(user, %{attrs | challenge: challenge}) assert {:error, _} = OAuth.exchange_code(code, verifier) verifier = String.duplicate("a", 129) - challenge = hash_verifier(verifier) + challenge = OAuthTest.hash_verifier(verifier) {:ok, code} = OAuth.create_code(user, %{attrs | challenge: challenge}) assert {:error, _} = OAuth.exchange_code(code, verifier) end From 356b6295c9996e8deb67c46cc8466b0ac612839a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sat, 15 Jun 2024 21:20:04 +0100 Subject: [PATCH 12/27] Add refresh token to the token endpoint --- .../controllers/o_auth/code_controller.ex | 41 +++++++++++---- lib/teiserver_web/router.ex | 2 +- .../o_auth/code_controller_test.exs | 52 +++++++++++++++++++ 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index 40a7967e1..cf366cbaa 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -3,29 +3,39 @@ defmodule TeiserverWeb.OAuth.CodeController do alias Teiserver.OAuth # https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.3 - @spec exchange_code(Conn.t(), %{}) :: Conn.t() + @spec token(Conn.t(), %{}) :: Conn.t() - def exchange_code(conn, %{"grant_type" => grant_type}) - when grant_type != "authorization_code" do - conn - |> put_status(400) - |> render(:error, error_description: "grant_type must be authorization_code") + def token(conn, %{"grant_type" => "authorization_code"} = params) do + case Enum.find( + ["client_id", "code", "redirect_uri", "client_id", "code_verifier", "grant_type"], + fn key -> not Map.has_key?(params, key) end + ) do + nil -> + exchange_token(conn, params) + + missing_key -> + conn |> put_status(400) |> render(:error, error_description: "missing #{missing_key}") + end end - def exchange_code(conn, params) do + def token(conn, %{"grant_type" => "refresh_token"} = params) do case Enum.find( - ["client_id", "code", "redirect_uri", "client_id", "code_verifier", "grant_type"], + ["grant_type", "refresh_token", "client_id"], fn key -> not Map.has_key?(params, key) end ) do nil -> - do_exchange_token(conn, params) + refresh_token(conn, params) missing_key -> conn |> put_status(400) |> render(:error, error_description: "missing #{missing_key}") end end - defp do_exchange_token(conn, params) do + def token(conn, _params) do + conn |> put_status(400) |> render(:error, error_description: "invalid authorization_code") + end + + defp exchange_token(conn, params) do with app when app != nil <- OAuth.get_application_by_uid(params["client_id"]), {:ok, code} <- OAuth.get_valid_code(params["code"]), true <- code.application_id == app.id, @@ -36,4 +46,15 @@ defmodule TeiserverWeb.OAuth.CodeController do _ -> conn |> put_status(400) |> render(:error, error_description: "invalid request") end end + + defp refresh_token(conn, params) do + with app when app != nil <- OAuth.get_application_by_uid(params["client_id"]), + {:ok, token} <- OAuth.get_valid_token(params["refresh_token"]), + true <- token.application_id == app.id, + {:ok, new_token} <- OAuth.refresh_token(token) do + conn |> put_status(200) |> render(:token, token: new_token) + else + _ -> conn |> put_status(400) |> render(:error, error_description: "invalid request") + end + end end diff --git a/lib/teiserver_web/router.ex b/lib/teiserver_web/router.ex index 124dc83fc..48f85c25d 100644 --- a/lib/teiserver_web/router.ex +++ b/lib/teiserver_web/router.ex @@ -598,7 +598,7 @@ defmodule TeiserverWeb.Router do # but I'd rather have all of the oauth stuff contained scope "/oauth/", TeiserverWeb.OAuth do pipe_through(:api) - post("/token", CodeController, :exchange_code) + post("/token", CodeController, :token) end scope "/admin", TeiserverWeb.Admin do diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index 015c37434..3f4f5bed0 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -38,6 +38,11 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do %{code: code, code_attrs: attrs} end + defp setup_token(context) do + {:ok, token} = OAuth.create_token(context[:user], context[:app]) + %{token: token} + end + describe "exchange code for token" do setup [:setup_conn, :setup_app, :setup_code] @@ -105,4 +110,51 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do # Note: verifier code and redirect URI matching is tested in OAuth.exchange_code # so they are omitted here end + + describe "refresh token" do + setup [:setup_conn, :setup_app, :setup_token] + + test "works", %{conn: conn} = setup_data do + data = %{ + grant_type: "refresh_token", + client_id: setup_data[:app].uid, + refresh_token: setup_data[:token].refresh_token.value + } + + resp = post(conn, ~p"/oauth/token", data) + assert json_resp = json_response(resp, 200) + assert is_binary(json_resp["access_token"]), "has access_token" + assert is_integer(json_resp["expires_in"]), "has expires_in" + assert is_binary(json_resp["refresh_token"]), "has refresh_token" + assert json_resp["token_type"] == "Bearer", "bearer token type" + + resp2 = post(conn, ~p"/oauth/token", data) + + # refresh token is now used up and invalid + assert json_response(resp2, 400) == %{ + "error_description" => "invalid request", + "error" => "invalid_request" + } + end + + test "client_id must match", %{conn: conn} = setup_data do + {:ok, other_app} = + OAuth.create_application(%{ + name: "another testing app", + uid: "another_test_app_uid", + owner_id: setup_data[:user].id, + scopes: ["tachyon.lobby"], + redirect_uris: ["http://127.0.0.1:6789/oauth/callback"] + }) + + data = %{ + grant_type: "refresh_token", + client_id: other_app.uid, + refresh_token: setup_data[:token].refresh_token.value + } + + resp = post(conn, ~p"/oauth/token", data) + assert %{"error" => "invalid_request"} = json_response(resp, 400) + end + end end From 2005c112efa170f9108bd6c69d062b433464ad76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 16 Jun 2024 07:07:50 +0100 Subject: [PATCH 13/27] Skeleton autohost table I'm almost sure teiserver is going to use a different table for autohosts going forward with tachyon. This is cleaner than relying on permissions of the user to differentiate between hosts and players. This table is there only so that the client secrets for OAuth can reference the correct table. --- lib/teiserver/autohost.ex | 12 +++++++++++ .../autohost/queries/autohost_query.ex | 20 ++++++++++++++++++ lib/teiserver/autohost/schemas/autohost.ex | 21 +++++++++++++++++++ .../20240607052027_autohost_setup.exs | 15 +++++++++++++ test/teiserver/autohost/autohost_test.exs | 10 +++++++++ 5 files changed, 78 insertions(+) create mode 100644 lib/teiserver/autohost.ex create mode 100644 lib/teiserver/autohost/queries/autohost_query.ex create mode 100644 lib/teiserver/autohost/schemas/autohost.ex create mode 100644 priv/repo/migrations/20240607052027_autohost_setup.exs create mode 100644 test/teiserver/autohost/autohost_test.exs diff --git a/lib/teiserver/autohost.ex b/lib/teiserver/autohost.ex new file mode 100644 index 000000000..e556e11e8 --- /dev/null +++ b/lib/teiserver/autohost.ex @@ -0,0 +1,12 @@ +defmodule Teiserver.Autohost do + alias Teiserver.Autohost.Autohost + alias Teiserver.Repo + + def create_autohost(attrs \\ %{}) do + %Autohost{} + |> Autohost.changeset(attrs) + |> Repo.insert() + end + + defdelegate get_autohost(id), to: Teiserver.AutohostQueries +end diff --git a/lib/teiserver/autohost/queries/autohost_query.ex b/lib/teiserver/autohost/queries/autohost_query.ex new file mode 100644 index 000000000..16ed4ff2f --- /dev/null +++ b/lib/teiserver/autohost/queries/autohost_query.ex @@ -0,0 +1,20 @@ +defmodule Teiserver.AutohostQueries do + use TeiserverWeb, :queries + alias Teiserver.Autohost.Autohost + + @spec get_autohost(Autohost.id()) :: Autohost.t() | nil + def get_autohost(nil), do: nil + + def get_autohost(id) do + base_query() |> where_id(id) |> Repo.one() + end + + def base_query() do + from autohost in Autohost, as: :autohost + end + + def where_id(query, id) do + from autohost in query, + where: autohost.id == ^id + end +end diff --git a/lib/teiserver/autohost/schemas/autohost.ex b/lib/teiserver/autohost/schemas/autohost.ex new file mode 100644 index 000000000..89d52a2fd --- /dev/null +++ b/lib/teiserver/autohost/schemas/autohost.ex @@ -0,0 +1,21 @@ +defmodule Teiserver.Autohost.Autohost do + @moduledoc false + use TeiserverWeb, :schema + + @type id :: integer() + @type t :: %__MODULE__{ + id: id(), + name: String.t() + } + + schema "teiserver_autohosts" do + field :name, :string + + timestamps(type: :utc_datetime) + end + + def changeset(autohost, attrs) do + autohost + |> cast(attrs, [:name]) + end +end diff --git a/priv/repo/migrations/20240607052027_autohost_setup.exs b/priv/repo/migrations/20240607052027_autohost_setup.exs new file mode 100644 index 000000000..a1332f883 --- /dev/null +++ b/priv/repo/migrations/20240607052027_autohost_setup.exs @@ -0,0 +1,15 @@ +defmodule Teiserver.Repo.Migrations.AutohostSetup do + use Ecto.Migration + + # This migration is really more a placeholder for the "new" autohost table + # used in tachyon. It'll be extended later as we implement tachyon and + # figure out what's required here. + # For now, only the basics so that we can bind oauth client secrets creds + # to the correct table from the start + def change do + create_if_not_exists table(:teiserver_autohosts) do + add :name, :string, comment: "short name to identify the host" + timestamps(type: :utc_datetime) + end + end +end diff --git a/test/teiserver/autohost/autohost_test.exs b/test/teiserver/autohost/autohost_test.exs new file mode 100644 index 000000000..7987cd2c8 --- /dev/null +++ b/test/teiserver/autohost/autohost_test.exs @@ -0,0 +1,10 @@ +defmodule Teiserver.Autohost.AutohostTest do + use Teiserver.DataCase, async: true + alias Teiserver.Autohost + + test "can create autohost" do + {:ok, autohost} = Autohost.create_autohost(%{name: "autohost_test"}) + assert autohost != nil + assert Autohost.get_autohost(autohost.id) == autohost + end +end From 161eea278dccb5a53148eb3cde890e802c2849bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 16 Jun 2024 07:42:51 +0100 Subject: [PATCH 14/27] Add basic operations for client credentials Create and retrieve. --- lib/teiserver/o_auth.ex | 66 ++++++++++++++++++- .../o_auth/queries/credential_query.ex | 23 +++++++ lib/teiserver/o_auth/schemas/credential.ex | 27 ++++++++ .../migrations/20240608144340_oauth_setup.exs | 12 ++++ test/teiserver/o_auth/credential_test.exs | 36 ++++++++++ 5 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 lib/teiserver/o_auth/queries/credential_query.ex create mode 100644 lib/teiserver/o_auth/schemas/credential.ex create mode 100644 test/teiserver/o_auth/credential_test.exs diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index ed591efc4..1705b9892 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -1,6 +1,19 @@ defmodule Teiserver.OAuth do alias Teiserver.Repo - alias Teiserver.OAuth.{Application, Code, Token, ApplicationQueries, CodeQueries, TokenQueries} + + alias Teiserver.Autohost.Autohost + + alias Teiserver.OAuth.{ + Application, + Code, + Token, + Credential, + ApplicationQueries, + CodeQueries, + TokenQueries, + CredentialQueries + } + alias Teiserver.Account.User alias Teiserver.Data.Types, as: T @@ -301,6 +314,57 @@ defmodule Teiserver.OAuth do end end + @doc """ + Given a client_id, an application/app_id an autohost/id and a cleartext secret, hash and persist it + """ + @spec create_credentials( + Application.t() | Application.app_id(), + Autohost.t() | Autohost.id(), + String.t(), + String.t() + ) :: + {:ok, Secret.t()} | {:error, term()} + def create_credentials(%Application{} = app, autohost, client_id, secret), + do: create_credentials(app.id, autohost, client_id, secret) + + def create_credentials(app_id, %Autohost{} = autohost, client_id, secret), + do: create_credentials(app_id, autohost.id, client_id, secret) + + def create_credentials(app_id, autohost_id, client_id, secret) do + attrs = %{ + application_id: app_id, + autohost_id: autohost_id, + client_id: client_id, + hashed_secret: Argon2.hash_pwd_salt(secret) + } + + %Credential{} + |> Credential.changeset(attrs) + |> Repo.insert() + end + + @doc """ + Given a client_id and a cleartext secret, check the secret matches and returns the credentials + """ + @spec get_valid_credentials(String.t(), String.t()) :: + {:ok, Credential.t()} | {:error, term()} + def get_valid_credentials(client_id, secret) do + case CredentialQueries.get_credential(client_id) do + nil -> + # Treat client_id as "secret" so force a fake computation to avoid + # timing attack + Argon2.no_user_verify() + {:error, :invalid_client_id} + + credential -> + if Argon2.verify_pass(secret, credential.hashed_secret) do + {:ok, credential} + else + {:error, :invalid_password} + end + end + end + defp expired?(obj, now) do Timex.after?(now, Map.fetch!(obj, :expires_at)) end diff --git a/lib/teiserver/o_auth/queries/credential_query.ex b/lib/teiserver/o_auth/queries/credential_query.ex new file mode 100644 index 000000000..61ce909cc --- /dev/null +++ b/lib/teiserver/o_auth/queries/credential_query.ex @@ -0,0 +1,23 @@ +defmodule Teiserver.OAuth.CredentialQueries do + use TeiserverWeb, :queries + alias Teiserver.OAuth.Credential + + def get_credential(nil), do: nil + + def get_credential(client_id) do + base_query() + |> preload(:application) + |> where_client_id(client_id) + |> Repo.one() + end + + def base_query() do + from credential in Credential, + as: :credential + end + + def where_client_id(query, client_id) do + from credential in query, + where: credential.client_id == ^client_id + end +end diff --git a/lib/teiserver/o_auth/schemas/credential.ex b/lib/teiserver/o_auth/schemas/credential.ex new file mode 100644 index 000000000..749eaa144 --- /dev/null +++ b/lib/teiserver/o_auth/schemas/credential.ex @@ -0,0 +1,27 @@ +defmodule Teiserver.OAuth.Credential do + @moduledoc false + use TeiserverWeb, :schema + + alias Teiserver.OAuth + + @type t :: %__MODULE__{ + application: OAuth.Application.t(), + autohost: Teiserver.Autohost.Autohost.t(), + hashed_secret: binary() + } + + schema "oauth_credentials" do + belongs_to :application, OAuth.Application + belongs_to :autohost, Teiserver.Autohost.Autohost, primary_key: true + field :client_id, :string + field :hashed_secret, :binary + + timestamps(type: :utc_datetime) + end + + def changeset(credential, attrs) do + credential + |> cast(attrs, [:application_id, :autohost_id, :client_id, :hashed_secret]) + |> validate_required([:client_id, :hashed_secret]) + end +end diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs index cf1d82894..b19e6bbe6 100644 --- a/priv/repo/migrations/20240608144340_oauth_setup.exs +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -25,6 +25,7 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do timestamps(type: :utc_datetime) end + create_if_not_exists unique_index(:oauth_codes, [:value]) create_if_not_exists table(:oauth_tokens, comment: "auth tokens and refresh") do @@ -39,7 +40,18 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do timestamps(type: :utc_datetime) end + create_if_not_exists unique_index(:oauth_tokens, [:value]) + create_if_not_exists table(:oauth_credentials, comment: "for client_credentials flow") do + add :application_id, references(:oauth_applications, on_delete: :delete_all), null: false + add :autohost_id, references(:teiserver_autohosts, on_delete: :delete_all), null: false + add :client_id, :string, null: false + add :hashed_secret, :binary, null: false + + timestamps(type: :utc_datetime) + end + + create_if_not_exists unique_index(:oauth_credentials, [:client_id]) end end diff --git a/test/teiserver/o_auth/credential_test.exs b/test/teiserver/o_auth/credential_test.exs new file mode 100644 index 000000000..31e7e8afc --- /dev/null +++ b/test/teiserver/o_auth/credential_test.exs @@ -0,0 +1,36 @@ +defmodule Teiserver.OAuth.CredentialTest do + use Teiserver.DataCase, async: true + alias Teiserver.{OAuth, Autohost} + + defp setup_autohost(_context) do + {:ok, autohost} = Autohost.create_autohost(%{name: "testing_autohost"}) + %{autohost: autohost} + end + + defp setup_app(_context) do + user = Teiserver.TeiserverTestLib.new_user() + + {:ok, app} = + OAuth.create_application(%{ + name: "Testing app", + uid: "test_app_uid", + owner_id: user.id, + scopes: ["tachyon.lobby"] + }) + + %{user: user, app: app} + end + + setup [:setup_autohost, :setup_app] + + test "can create and retrieve credentials", %{app: app, autohost: autohost} do + assert {:ok, created_cred} = + OAuth.create_credentials(app, autohost, "some-client-id", "very-secret") + + assert {:ok, cred} = OAuth.get_valid_credentials("some-client-id", "very-secret") + assert created_cred.id == cred.id + assert cred.application.uid == app.uid + # sanity check to make sure we're not storing cleartext password + refute cred.hashed_secret =~ "very-secret" + end +end From 7c5b4edf471fcef6190ab490195776fa1f042567 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 16 Jun 2024 08:02:56 +0100 Subject: [PATCH 15/27] Add constraint for token owner A token can be owned by a user, or be associated with an autohost. --- lib/teiserver/o_auth/schemas/token.ex | 4 ++-- .../migrations/20240608144340_oauth_setup.exs | 8 +++++++- test/teiserver/o_auth/token_test.exs | 16 ++++++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/lib/teiserver/o_auth/schemas/token.ex b/lib/teiserver/o_auth/schemas/token.ex index ddf223a91..32ad4b7b9 100644 --- a/lib/teiserver/o_auth/schemas/token.ex +++ b/lib/teiserver/o_auth/schemas/token.ex @@ -16,7 +16,7 @@ defmodule Teiserver.OAuth.Token do schema "oauth_tokens" do field :value, :string - belongs_to :owner, Teiserver.Account.User, primary_key: true + belongs_to :owner, Teiserver.Account.User belongs_to :application, OAuth.Application, primary_key: true field :scopes, {:array, :string} field :expires_at, :utc_datetime @@ -32,7 +32,7 @@ defmodule Teiserver.OAuth.Token do token |> cast(attrs, [:value, :owner_id, :application_id, :scopes, :expires_at, :type]) |> cast_assoc(:refresh_token) - |> validate_required([:value, :owner_id, :application_id, :scopes, :expires_at, :type]) + |> validate_required([:value, :application_id, :scopes, :expires_at, :type]) |> Ecto.Changeset.validate_subset(:scopes, OAuth.Application.allowed_scopes()) end end diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs index b19e6bbe6..48fd96012 100644 --- a/priv/repo/migrations/20240608144340_oauth_setup.exs +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -30,19 +30,25 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do create_if_not_exists table(:oauth_tokens, comment: "auth tokens and refresh") do add :value, :string, null: false - add :owner_id, references(:account_users, on_delete: :delete_all), null: false + add :owner_id, references(:account_users, on_delete: :delete_all) add :application_id, references(:oauth_applications, on_delete: :delete_all), null: false add :scopes, {:array, :string}, null: false add :expires_at, :utc_datetime, null: false add :type, :string, null: false # we should create a new refresh token when deleting an auth token and vice versa add :refresh_token_id, references(:oauth_tokens) + add :autohost_id, references(:teiserver_autohosts, on_delete: :delete_all) timestamps(type: :utc_datetime) end create_if_not_exists unique_index(:oauth_tokens, [:value]) + create constraint(:oauth_tokens, "token_must_have_exactly_one_owner", + check: + "(owner_id IS NOT NULL AND autohost_id IS NULL) OR (owner_id IS NULL AND autohost_id IS NOT NULL)" + ) + create_if_not_exists table(:oauth_credentials, comment: "for client_credentials flow") do add :application_id, references(:oauth_applications, on_delete: :delete_all), null: false add :autohost_id, references(:teiserver_autohosts, on_delete: :delete_all), null: false diff --git a/test/teiserver/o_auth/token_test.exs b/test/teiserver/o_auth/token_test.exs index db6a45aa5..b4213e8ad 100644 --- a/test/teiserver/o_auth/token_test.exs +++ b/test/teiserver/o_auth/token_test.exs @@ -16,6 +16,21 @@ defmodule Teiserver.OAuth.TokenTest do {:ok, user: user, app: app} end + test "token must have an owner", %{app: app} do + assert {:error, _} = + Teiserver.OAuth.Token.changeset(%Teiserver.OAuth.Token{}, %{ + value: "coucou", + application_id: app.id, + scopes: ["tachyon.lobby"], + expires_at: DateTime.utc_now(), + type: :access + }) + |> Ecto.Changeset.check_constraint(:oauth_tokens, + name: :token_must_have_exactly_one_owner + ) + |> Teiserver.Repo.insert() + end + test "can create a token directly", %{user: user, app: app} do assert {:ok, token} = OAuth.create_token(user, app) assert token.type == :access @@ -44,6 +59,7 @@ defmodule Teiserver.OAuth.TokenTest do test "can refresh a token", %{user: user, app: app} do assert {:ok, token} = OAuth.create_token(user, app) + assert {:ok, _} = OAuth.get_valid_token(token.refresh_token.value) assert {:ok, new_token} = OAuth.refresh_token(token.refresh_token) # the previous token and its refresh token should have been invalidated From 0251867391a237c4bcdb769cab4ff9751294abc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 16 Jun 2024 09:30:31 +0100 Subject: [PATCH 16/27] Can get a token from client credentials --- lib/teiserver/o_auth.ex | 65 ++++++++++++++--------- lib/teiserver/o_auth/schemas/token.ex | 3 +- test/teiserver/o_auth/credential_test.exs | 10 ++++ 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index 1705b9892..adbc38ea6 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -177,29 +177,35 @@ defmodule Teiserver.OAuth do end def create_token(user_id, application, opts) do - now = Keyword.get(opts, :now, DateTime.utc_now()) + do_create_token(%{owner_id: user_id}, application, opts) + end - refresh_attrs = %{ - value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), - owner_id: user_id, - application_id: application.id, - scopes: application.scopes, - # there's no real recourse when the refresh token expires and it's - # quite annoying, so make it "never" expire. - expires_at: Timex.add(now, Timex.Duration.from_days(365 * 100)), - type: :refresh, - refresh_token: nil - } + defp do_create_token(owner_attr, application, opts \\ []) do + now = Keyword.get(opts, :now, DateTime.utc_now()) - token_attrs = %{ - value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), - owner_id: user_id, - application_id: application.id, - scopes: application.scopes, - expires_at: Timex.add(now, Timex.Duration.from_minutes(30)), - type: :bearer, - refresh_token: refresh_attrs - } + refresh_attrs = + %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), + application_id: application.id, + scopes: application.scopes, + # there's no real recourse when the refresh token expires and it's + # quite annoying, so make it "never" expire. + expires_at: Timex.add(now, Timex.Duration.from_days(365 * 100)), + type: :refresh, + refresh_token: nil + } + |> Map.merge(owner_attr) + + token_attrs = + %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), + application_id: application.id, + scopes: application.scopes, + expires_at: Timex.add(now, Timex.Duration.from_minutes(30)), + type: :access, + refresh_token: refresh_attrs + } + |> Map.merge(owner_attr) %Token{} |> Token.changeset(token_attrs) @@ -338,9 +344,15 @@ defmodule Teiserver.OAuth do hashed_secret: Argon2.hash_pwd_salt(secret) } - %Credential{} - |> Credential.changeset(attrs) - |> Repo.insert() + result = + %Credential{} + |> Credential.changeset(attrs) + |> Repo.insert() + + case result do + {:ok, cred} -> {:ok, Repo.preload(cred, :application)} + err -> err + end end @doc """ @@ -365,6 +377,11 @@ defmodule Teiserver.OAuth do end end + @spec get_token_from_credentials(Credential.t()) :: {:ok, Token.t()} | {:error, term()} + def get_token_from_credentials(credential) do + do_create_token(%{autohost_id: credential.autohost_id}, credential.application) + end + defp expired?(obj, now) do Timex.after?(now, Map.fetch!(obj, :expires_at)) end diff --git a/lib/teiserver/o_auth/schemas/token.ex b/lib/teiserver/o_auth/schemas/token.ex index 32ad4b7b9..cf42a472b 100644 --- a/lib/teiserver/o_auth/schemas/token.ex +++ b/lib/teiserver/o_auth/schemas/token.ex @@ -22,6 +22,7 @@ defmodule Teiserver.OAuth.Token do field :expires_at, :utc_datetime field :type, Ecto.Enum, values: [:access, :refresh] belongs_to :refresh_token, __MODULE__ + belongs_to :autohost, Teiserver.Autohost.Autohost timestamps() end @@ -30,7 +31,7 @@ defmodule Teiserver.OAuth.Token do attrs = attrs |> uniq_lists(~w(scopes)a) token - |> cast(attrs, [:value, :owner_id, :application_id, :scopes, :expires_at, :type]) + |> cast(attrs, [:value, :owner_id, :application_id, :scopes, :expires_at, :type, :autohost_id]) |> cast_assoc(:refresh_token) |> validate_required([:value, :application_id, :scopes, :expires_at, :type]) |> Ecto.Changeset.validate_subset(:scopes, OAuth.Application.allowed_scopes()) diff --git a/test/teiserver/o_auth/credential_test.exs b/test/teiserver/o_auth/credential_test.exs index 31e7e8afc..9000ee2e2 100644 --- a/test/teiserver/o_auth/credential_test.exs +++ b/test/teiserver/o_auth/credential_test.exs @@ -33,4 +33,14 @@ defmodule Teiserver.OAuth.CredentialTest do # sanity check to make sure we're not storing cleartext password refute cred.hashed_secret =~ "very-secret" end + + test "can get a token from credentials", %{app: app, autohost: autohost} do + assert {:ok, created_cred} = + OAuth.create_credentials(app, autohost, "some-client-id", "very-secret") + + assert {:ok, token} = OAuth.get_token_from_credentials(created_cred) + assert token.application_id == app.id + assert token.owner_id == nil + assert token.autohost_id == autohost.id + end end From 4989eaede40ed0f75c923052b8f01266b6a910bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 16 Jun 2024 10:10:44 +0100 Subject: [PATCH 17/27] Http interface to get token from client credentials --- .../controllers/o_auth/code_controller.ex | 24 +++++++++- .../o_auth/code_controller_test.exs | 44 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index cf366cbaa..b05d2420e 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -31,8 +31,21 @@ defmodule TeiserverWeb.OAuth.CodeController do end end + def token(conn, %{"grant_type" => "client_credentials"} = params) do + case Enum.find( + ["grant_type", "client_id", "client_secret"], + fn key -> not Map.has_key?(params, key) end + ) do + nil -> + get_token_from_credentials(conn, params) + + missing_key -> + conn |> put_status(400) |> render(:error, error_description: "missing #{missing_key}") + end + end + def token(conn, _params) do - conn |> put_status(400) |> render(:error, error_description: "invalid authorization_code") + conn |> put_status(400) |> render(:error, error_description: "invalid grant_type") end defp exchange_token(conn, params) do @@ -57,4 +70,13 @@ defmodule TeiserverWeb.OAuth.CodeController do _ -> conn |> put_status(400) |> render(:error, error_description: "invalid request") end end + + defp get_token_from_credentials(conn, params) do + with {:ok, cred} <- OAuth.get_valid_credentials(params["client_id"], params["client_secret"]), + {:ok, token} <- OAuth.get_token_from_credentials(cred) do + conn |> put_status(200) |> render(:token, token: token) + else + _ -> conn |> put_status(400) |> render(:error, error_description: "invalid request") + end + end end diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index 3f4f5bed0..d895a1506 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -38,6 +38,17 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do %{code: code, code_attrs: attrs} end + defp setup_autohost(_context) do + {:ok, autohost} = Teiserver.Autohost.create_autohost(%{name: "testing_autohost"}) + %{autohost: autohost} + end + + defp setup_credential(%{autohost: autohost, app: app}) do + secret = "very-much-secret" + {:ok, cred} = OAuth.create_credentials(app, autohost, "cred-client-id", secret) + %{credential: cred, credential_secret: secret} + end + defp setup_token(context) do {:ok, token} = OAuth.create_token(context[:user], context[:app]) %{token: token} @@ -111,6 +122,39 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do # so they are omitted here end + describe "get token from client credentials" do + setup [:setup_conn, :setup_app, :setup_autohost, :setup_credential] + + test "it works with the right params", %{ + conn: conn, + credential: credential, + credential_secret: secret + } do + data = %{ + grant_type: "client_credentials", + client_id: credential.client_id, + client_secret: secret + } + + resp = post(conn, ~p"/oauth/token", data) + json_resp = json_response(resp, 200) + assert is_binary(json_resp["access_token"]), "has access_token" + assert is_integer(json_resp["expires_in"]), "has expires_in" + assert is_binary(json_resp["refresh_token"]), "has refresh_token" + assert json_resp["token_type"] == "Bearer", "bearer token type" + end + + test "must provide correct secret", %{conn: conn, credential: credential} do + data = %{ + grant_type: "client_credentials", + client_id: credential.client_id, + client_secret: "definitely-not-the-correct-secret" + } + resp = post(conn, ~p"/oauth/token", data) + json_response(resp, 400) + end + end + describe "refresh token" do setup [:setup_conn, :setup_app, :setup_token] From 4bd1e6b71373670008732a8d0bfbaac34f00a728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 16 Jun 2024 10:39:43 +0100 Subject: [PATCH 18/27] Add oauth metadata endpoint --- config/runtime.exs | 8 ++++--- .../controllers/o_auth/code_controller.ex | 4 ++++ lib/teiserver_web/router.ex | 6 +++++ .../views/api/o_auth/code_view.ex | 19 ++++++++++++++++ .../o_auth/code_controller_test.exs | 22 +++++++++++++++++++ 5 files changed, 56 insertions(+), 3 deletions(-) diff --git a/config/runtime.exs b/config/runtime.exs index 0c64106de..005e481bf 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -24,13 +24,13 @@ if Teiserver.ConfigHelpers.get_env("PHX_SERVER", nil) do config :teiserver, TeiserverWeb.Endpoint, server: true end +# used for mailing, checking origins, finding tls certs… +domain_name = Teiserver.ConfigHelpers.get_env("TEI_DOMAIN_NAME", "beyondallreason.info") + # Only do some runtime configuration in production since in dev and tests the # files are automatically recompiled on the fly and thus, config/{dev,test}.exs # are just fine if config_env() == :prod do - # used for mailing, checking origins, finding tls certs… - domain_name = Teiserver.ConfigHelpers.get_env("TEI_DOMAIN_NAME", "beyondallreason.info") - certificates = [ keyfile: Teiserver.ConfigHelpers.get_env("TEI_TLS_PRIVATE_KEY_PATH"), certfile: Teiserver.ConfigHelpers.get_env("TEI_TLS_CERT_PATH"), @@ -190,3 +190,5 @@ if config_env() == :prod do bot_name: Teiserver.ConfigHelpers.get_env("TEI_DISCORD_BOT_NAME") end end + +config :teiserver, Teiserver.OAuth, issuer: "https://#{domain_name}" diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index b05d2420e..960e46c1b 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -79,4 +79,8 @@ defmodule TeiserverWeb.OAuth.CodeController do _ -> conn |> put_status(400) |> render(:error, error_description: "invalid request") end end + + def metadata(conn, _params) do + conn |> put_status(200) |> render(:metadata) + end end diff --git a/lib/teiserver_web/router.ex b/lib/teiserver_web/router.ex index 48f85c25d..d1f522d66 100644 --- a/lib/teiserver_web/router.ex +++ b/lib/teiserver_web/router.ex @@ -601,6 +601,12 @@ defmodule TeiserverWeb.Router do post("/token", CodeController, :token) end + scope "/", TeiserverWeb.OAuth do + pipe_through(:api) + # https://datatracker.ietf.org/doc/html/rfc8414 + get("/.well-known/oauth-authorization-server", CodeController, :metadata) + end + scope "/admin", TeiserverWeb.Admin do pipe_through([:live_browser, :protected]) diff --git a/lib/teiserver_web/views/api/o_auth/code_view.ex b/lib/teiserver_web/views/api/o_auth/code_view.ex index 7bab7f806..341e0897f 100644 --- a/lib/teiserver_web/views/api/o_auth/code_view.ex +++ b/lib/teiserver_web/views/api/o_auth/code_view.ex @@ -1,4 +1,6 @@ defmodule TeiserverWeb.OAuth.CodeView do + use TeiserverWeb, :view + def token(%{token: token}) do expires_in = DateTime.diff(token.expires_at, DateTime.utc_now(), :second) @@ -13,4 +15,21 @@ defmodule TeiserverWeb.OAuth.CodeView do def error(conn) do Map.take(conn, [:error_description]) |> Map.put("error", "invalid_request") end + + def metadata(_) do + base = Application.fetch_env!(:teiserver, Teiserver.OAuth)[:issuer] + + %{ + issuer: base, + authorization_endpoint: base <> ~p"/oauth/authorize", + token_endpoint: base <> ~p"/oauth/token", + token_endpoint_auth_methods_supported: ["none", "client_secret_post"], + grant_types_supported: [ + "authorization_code", + "refresh_token", + "client_credentials" + ], + code_challenge_methods_supported: ["S256"] + } + end end diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index d895a1506..b8585efd1 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -150,6 +150,7 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do client_id: credential.client_id, client_secret: "definitely-not-the-correct-secret" } + resp = post(conn, ~p"/oauth/token", data) json_response(resp, 400) end @@ -201,4 +202,25 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do assert %{"error" => "invalid_request"} = json_response(resp, 400) end end + + describe "medatata endpoint" do + setup :setup_conn + + test "can query oauth metadata", %{conn: conn} do + resp = json_response(get(conn, ~p"/.well-known/oauth-authorization-server"), 200) + + assert resp == %{ + "issuer" => "https://beyondallreason.info", + "authorization_endpoint" => "https://beyondallreason.info/oauth/authorize", + "token_endpoint" => "https://beyondallreason.info/oauth/token", + "token_endpoint_auth_methods_supported" => ["none", "client_secret_post"], + "grant_types_supported" => [ + "authorization_code", + "refresh_token", + "client_credentials" + ], + "code_challenge_methods_supported" => ["S256"] + } + end + end end From aa1ff9474c9abaf45dc6d8e6f670685d6d71408c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Thu, 27 Jun 2024 15:21:05 +0200 Subject: [PATCH 19/27] Add support for basic auth in OAuth endpoint --- .../controllers/o_auth/code_controller.ex | 30 +++++++++----- .../views/api/o_auth/code_view.ex | 2 +- .../o_auth/code_controller_test.exs | 40 +++++++++++++++++-- 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index 960e46c1b..30dd436dc 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -32,15 +32,12 @@ defmodule TeiserverWeb.OAuth.CodeController do end def token(conn, %{"grant_type" => "client_credentials"} = params) do - case Enum.find( - ["grant_type", "client_id", "client_secret"], - fn key -> not Map.has_key?(params, key) end - ) do - nil -> - get_token_from_credentials(conn, params) + case get_credentials(conn, params) do + {:ok, client_id, client_secret} -> + get_token_from_credentials(conn, client_id, client_secret) - missing_key -> - conn |> put_status(400) |> render(:error, error_description: "missing #{missing_key}") + {:error, msg} -> + conn |> put_status(400) |> render(:error, error_description: msg) end end @@ -71,8 +68,21 @@ defmodule TeiserverWeb.OAuth.CodeController do end end - defp get_token_from_credentials(conn, params) do - with {:ok, cred} <- OAuth.get_valid_credentials(params["client_id"], params["client_secret"]), + defp get_credentials(conn, params) do + basic = Plug.BasicAuth.parse_basic_auth(conn) + post_params = {Map.get(params, "client_id"), Map.get(params, "client_secret")} + + case {basic, post_params} do + {:error, {nil, nil}} -> {:error, "unauthorized"} + {{user, pass}, _} -> {:ok, user, pass} + {_, {nil, _}} -> {:error, "missing client_id"} + {_, {_, nil}} -> {:error, "missing client_secret"} + {_, {client_id, client_secret}} -> {:ok, client_id, client_secret} + end + end + + defp get_token_from_credentials(conn, client_id, client_secret) do + with {:ok, cred} <- OAuth.get_valid_credentials(client_id, client_secret), {:ok, token} <- OAuth.get_token_from_credentials(cred) do conn |> put_status(200) |> render(:token, token: token) else diff --git a/lib/teiserver_web/views/api/o_auth/code_view.ex b/lib/teiserver_web/views/api/o_auth/code_view.ex index 341e0897f..1df896536 100644 --- a/lib/teiserver_web/views/api/o_auth/code_view.ex +++ b/lib/teiserver_web/views/api/o_auth/code_view.ex @@ -23,7 +23,7 @@ defmodule TeiserverWeb.OAuth.CodeView do issuer: base, authorization_endpoint: base <> ~p"/oauth/authorize", token_endpoint: base <> ~p"/oauth/token", - token_endpoint_auth_methods_supported: ["none", "client_secret_post"], + token_endpoint_auth_methods_supported: ["none", "client_secret_post", "client_secret_basic"], grant_types_supported: [ "authorization_code", "refresh_token", diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index b8585efd1..80ba58ef9 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -15,8 +15,9 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do end defp setup_conn(_context) do - GeneralTestLib.conn_setup(Teiserver.TeiserverTestLib.player_permissions()) - |> Teiserver.TeiserverTestLib.conn_setup() + conn = Phoenix.ConnTest.build_conn() + user = GeneralTestLib.make_user() + {:ok, conn: conn, user: user} end defp setup_app(context) do @@ -154,6 +155,35 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do resp = post(conn, ~p"/oauth/token", data) json_response(resp, 400) end + + test "can also use basic auth", + %{conn: conn, credential: credential, credential_secret: secret} do + data = %{grant_type: "client_credentials"} + auth_header = Plug.BasicAuth.encode_basic_auth(credential.client_id, secret) + + conn = + conn + |> put_req_header("authorization", auth_header) + + resp = post(conn, ~p"/oauth/token", data) + json_resp = json_response(resp, 200) + assert is_binary(json_resp["access_token"]), "has access_token" + assert is_integer(json_resp["expires_in"]), "has expires_in" + assert is_binary(json_resp["refresh_token"]), "has refresh_token" + assert json_resp["token_type"] == "Bearer", "bearer token type" + end + + test "basic auth check", %{conn: conn, credential: credential} do + data = %{grant_type: "client_credentials"} + auth_header = Plug.BasicAuth.encode_basic_auth(credential.client_id, "lolnope") + + conn = + conn + |> put_req_header("authorization", auth_header) + + resp = post(conn, ~p"/oauth/token", data) + json_response(resp, 400) + end end describe "refresh token" do @@ -213,7 +243,11 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do "issuer" => "https://beyondallreason.info", "authorization_endpoint" => "https://beyondallreason.info/oauth/authorize", "token_endpoint" => "https://beyondallreason.info/oauth/token", - "token_endpoint_auth_methods_supported" => ["none", "client_secret_post"], + "token_endpoint_auth_methods_supported" => [ + "none", + "client_secret_post", + "client_secret_basic" + ], "grant_types_supported" => [ "authorization_code", "refresh_token", From 5a9385f4407171881feb930866ad2f234d1852a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Thu, 27 Jun 2024 15:42:00 +0200 Subject: [PATCH 20/27] Fix dialyzer errors --- lib/teiserver/account.ex | 2 ++ lib/teiserver/account/schemas/user.ex | 3 +++ lib/teiserver/o_auth.ex | 2 +- lib/teiserver/o_auth/schemas/code.ex | 1 + lib/teiserver/o_auth/schemas/token.ex | 1 + lib/teiserver_web/controllers/o_auth/code_controller.ex | 2 +- 6 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/teiserver/account.ex b/lib/teiserver/account.ex index 9aabc5bdf..ca48ca266 100644 --- a/lib/teiserver/account.ex +++ b/lib/teiserver/account.ex @@ -9,6 +9,8 @@ defmodule Teiserver.Account do alias Teiserver.Account.UserLib + @type t :: UserLib.t() + @spec icon :: String.t() def icon, do: "fa-solid fa-user-alt" diff --git a/lib/teiserver/account/schemas/user.ex b/lib/teiserver/account/schemas/user.ex index 2792f1274..daa7ffa84 100644 --- a/lib/teiserver/account/schemas/user.ex +++ b/lib/teiserver/account/schemas/user.ex @@ -5,6 +5,9 @@ defmodule Teiserver.Account.User do alias Argon2 + # TODO: this is where a user should be defined. This is only a placeholder for now + @type t :: term() + schema "account_users" do field :name, :string field :email, :string diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index adbc38ea6..db85e9b68 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -329,7 +329,7 @@ defmodule Teiserver.OAuth do String.t(), String.t() ) :: - {:ok, Secret.t()} | {:error, term()} + {:ok, Credential.t()} | {:error, term()} def create_credentials(%Application{} = app, autohost, client_id, secret), do: create_credentials(app.id, autohost, client_id, secret) diff --git a/lib/teiserver/o_auth/schemas/code.ex b/lib/teiserver/o_auth/schemas/code.ex index 0eb5d14bb..38f37b68f 100644 --- a/lib/teiserver/o_auth/schemas/code.ex +++ b/lib/teiserver/o_auth/schemas/code.ex @@ -3,6 +3,7 @@ defmodule Teiserver.OAuth.Code do use TeiserverWeb, :schema alias Teiserver.OAuth + alias Teiserver.Account.User @type t :: %__MODULE__{ value: String.t(), diff --git a/lib/teiserver/o_auth/schemas/token.ex b/lib/teiserver/o_auth/schemas/token.ex index cf42a472b..6f1af4e06 100644 --- a/lib/teiserver/o_auth/schemas/token.ex +++ b/lib/teiserver/o_auth/schemas/token.ex @@ -3,6 +3,7 @@ defmodule Teiserver.OAuth.Token do use TeiserverWeb, :schema alias Teiserver.OAuth + alias Teiserver.Account.User @type t :: %__MODULE__{ value: String.t(), diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index 30dd436dc..a6728f284 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -3,7 +3,7 @@ defmodule TeiserverWeb.OAuth.CodeController do alias Teiserver.OAuth # https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.3 - @spec token(Conn.t(), %{}) :: Conn.t() + @spec token(Plug.Conn.t(), %{}) :: Plug.Conn.t() def token(conn, %{"grant_type" => "authorization_code"} = params) do case Enum.find( From 5ddcc77c27fbe73f1f38fa9dffee465f489fc605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Sun, 28 Jul 2024 11:23:08 +0100 Subject: [PATCH 21/27] Add unique constraint on app uid --- priv/repo/migrations/20240608144340_oauth_setup.exs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/priv/repo/migrations/20240608144340_oauth_setup.exs b/priv/repo/migrations/20240608144340_oauth_setup.exs index 48fd96012..a37ab87ed 100644 --- a/priv/repo/migrations/20240608144340_oauth_setup.exs +++ b/priv/repo/migrations/20240608144340_oauth_setup.exs @@ -13,6 +13,8 @@ defmodule Teiserver.Repo.Migrations.OauthSetup do timestamps(type: :utc_datetime) end + create_if_not_exists unique_index(:oauth_applications, [:uid]) + create_if_not_exists table(:oauth_codes, comment: "authorisation codes") do add :value, :string, null: false add :owner_id, references(:account_users, on_delete: :delete_all), null: false From c7293b17eaec6ead451216e266dbac6df92671a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Mon, 5 Aug 2024 07:31:47 +0100 Subject: [PATCH 22/27] Improve and surface error messages Because it's more obnoxious than anything to just get "invalid_request" when something goes wrong without any indication about the problem. --- lib/teiserver/o_auth.ex | 135 ++++++++++-------- .../o_auth/authorize_controller.ex | 4 +- .../controllers/o_auth/code_controller.ex | 46 +++++- test/support/o_auth.ex | 11 +- test/teiserver/o_auth/code_test.exs | 25 +++- .../o_auth/code_controller_test.exs | 16 ++- 6 files changed, 156 insertions(+), 81 deletions(-) diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index db85e9b68..33c874532 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -151,12 +151,7 @@ defmodule Teiserver.OAuth do code -> now = Keyword.get(opts, :now, Timex.now()) - - if expired?(code, now) do - {:error, :expired} - else - {:ok, code} - end + check_expiry(code, now) end end @@ -212,8 +207,6 @@ defmodule Teiserver.OAuth do |> Repo.insert() end - # TODO: get_valid_token is basically the same as get_valid_code, refactor later - @doc """ Given a code returns the corresponding db object, making sure it is valid (exists, not expired, not revoked) @@ -228,12 +221,7 @@ defmodule Teiserver.OAuth do token -> now = Keyword.get(opts, :now, Timex.now()) - - if Timex.after?(now, token.expires_at) do - {:error, :expired} - else - {:ok, token} - end + check_expiry(token, now) end end @@ -246,50 +234,69 @@ defmodule Teiserver.OAuth do def exchange_code(code, verifier, redirect_uri \\ nil, opts \\ []) do now = Keyword.get(opts, :now, Timex.now()) - cond do - expired?(code, now) -> - {:error, :expired} - - code.redirect_uri != redirect_uri -> - {:error, :redirect_uri_mismatch} - - not code_verified?(code, verifier) -> - {:error, :code_verification_failed} - - true -> - Repo.transaction(fn -> - Repo.delete!(code) + with {:ok, code} <- check_expiry(code, now), + :ok <- + if(code.redirect_uri == redirect_uri, do: :ok, else: {:error, :redirect_uri_mismatch}), + :ok <- check_verifier(code, verifier) do + Repo.transaction(fn -> + Repo.delete!(code) - {:ok, token} = - create_token(code.owner_id, %{id: code.application_id, scopes: code.scopes}, opts) + {:ok, token} = + create_token(code.owner_id, %{id: code.application_id, scopes: code.scopes}, opts) - token - end) + token + end) end end - @spec code_verified?(Code.t(), String.t()) :: boolean() - defp code_verified?(%Code{challenge_method: :plain, challenge: challenge}, verifier) do - valid_verifier?(verifier) and :crypto.hash_equals(challenge, verifier) + @spec check_verifier(Code.t(), String.t()) :: :ok | {:error, term()} + defp check_verifier(%Code{challenge_method: :plain, challenge: challenge}, verifier) do + with :ok <- valid_verifier(verifier) do + compare_hash(challenge, verifier) + end end - defp code_verified?(%Code{challenge_method: :S256, challenge: challenge}, verifier) do - with true <- valid_verifier?(verifier), - {:ok, challenge} <- Base.url_decode64(challenge, padding: false, ignore: :whitespace) do - hashed_verifier = :crypto.hash(:sha256, verifier) - :crypto.hash_equals(challenge, hashed_verifier) - else - _ -> - false + defp check_verifier(%Code{challenge_method: :S256, challenge: challenge}, verifier) do + with :ok <- valid_verifier(verifier), + {:ok, challenge} <- Base.url_decode64(challenge, padding: false, ignore: :whitespace), + :ok <- + compare_hash(challenge, :crypto.hash(:sha256, verifier)) do + :ok end end - defp code_verified?(_, _), do: false + defp check_verifier(_, _), do: {:error, :invalid_verifier} # A-Z, a-z, 0-9, and the punctuation characters -._~ - defp valid_verifier?(verifier) do + defp valid_verifier(verifier) do s = byte_size(verifier) - 43 <= s and s <= 128 and String.match?(verifier, ~r/[A-Za-z0-9\-._~]/) + + cond do + s < 43 -> + {:error, "verifier cannot be less than 43 chars"} + + s > 128 -> + {:error, "verifier cannot be more than 128 chars"} + + not String.match?(verifier, ~r/[A-Za-z0-9\-._~]/) -> + {:error, "verifier contains illegal characters"} + + true -> + :ok + end + end + + defp compare_hash(challenge, verifier) do + cond do + String.length(challenge) != String.length(verifier) -> + {:error, "challenge and verifier length mismatch"} + + not :crypto.hash_equals(challenge, verifier) -> + {:error, "verifier doesn't match challenge"} + + true -> + :ok + end end @spec refresh_token(Token.t(), options()) :: {:ok, Token.t()} | {:error, term()} @@ -302,21 +309,23 @@ defmodule Teiserver.OAuth do def refresh_token(token, opts) do now = Keyword.get(opts, :now, Timex.now()) - if expired?(token, now) do - {:error, :expired} - else - token = - if Ecto.assoc_loaded?(token.application) do - token - else - TokenQueries.get_token(token.value) - end + case check_expiry(token, now) do + {:error, :expired} -> + {:error, :expired} - Repo.transaction(fn -> - {:ok, new_token} = create_token(token.owner_id, token.application, opts) - TokenQueries.delete_refresh_token(token) - new_token - end) + _ -> + token = + if Ecto.assoc_loaded?(token.application) do + token + else + TokenQueries.get_token(token.value) + end + + Repo.transaction(fn -> + {:ok, new_token} = create_token(token.owner_id, token.application, opts) + TokenQueries.delete_refresh_token(token) + new_token + end) end end @@ -382,7 +391,11 @@ defmodule Teiserver.OAuth do do_create_token(%{autohost_id: credential.autohost_id}, credential.application) end - defp expired?(obj, now) do - Timex.after?(now, Map.fetch!(obj, :expires_at)) + defp check_expiry(obj, now) do + if Timex.after?(now, Map.fetch!(obj, :expires_at)) do + {:error, :expired} + else + {:ok, obj} + end end end diff --git a/lib/teiserver_web/controllers/o_auth/authorize_controller.ex b/lib/teiserver_web/controllers/o_auth/authorize_controller.ex index 3c9379033..1e5ccb7bb 100644 --- a/lib/teiserver_web/controllers/o_auth/authorize_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/authorize_controller.ex @@ -14,8 +14,8 @@ defmodule TeiserverWeb.OAuth.AuthorizeController do app -> case OAuth.get_redirect_uri(app, Map.get(conn.params, "redirect_uri")) do - {:error, _err} -> - bad_request(conn, "invalid redirection uri") + {:error, err} -> + bad_request(conn, "invalid redirection uri: #{inspect(err)}") {:ok, redir_uri} -> reject_uri = diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index a6728f284..967e10d84 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -46,21 +46,32 @@ defmodule TeiserverWeb.OAuth.CodeController do end defp exchange_token(conn, params) do - with app when app != nil <- OAuth.get_application_by_uid(params["client_id"]), + with {:ok, app} <- get_app_by_uid(params["client_id"]), {:ok, code} <- OAuth.get_valid_code(params["code"]), - true <- code.application_id == app.id, + :ok <- + if(code.application_id == app.id, + do: :ok, + else: {:error, "code doesn't match application. Invalid code for this client_id"} + ), {:ok, token} <- OAuth.exchange_code(code, params["code_verifier"], params["redirect_uri"]) do conn |> put_status(200) |> render(:token, token: token) else - _ -> conn |> put_status(400) |> render(:error, error_description: "invalid request") + {:error, err} -> + conn + |> put_status(400) + |> render(:error, error_description: "invalid request: #{err_message(err)}") end end defp refresh_token(conn, params) do - with app when app != nil <- OAuth.get_application_by_uid(params["client_id"]), + with {:ok, app} <- get_app_by_uid(params["client_id"]), {:ok, token} <- OAuth.get_valid_token(params["refresh_token"]), - true <- token.application_id == app.id, + :ok <- + if(token.application_id == app.id, + do: :ok, + else: {:error, "token doesn't match application. Invalid token for this client_id"} + ), {:ok, new_token} <- OAuth.refresh_token(token) do conn |> put_status(200) |> render(:token, token: new_token) else @@ -93,4 +104,29 @@ defmodule TeiserverWeb.OAuth.CodeController do def metadata(conn, _params) do conn |> put_status(200) |> render(:metadata) end + + defp get_app_by_uid(uid) do + case OAuth.get_application_by_uid(uid) do + app when not is_nil(app) -> {:ok, app} + _ -> {:error, "no application found for uid #{inspect(uid)}"} + end + end + + defp err_message(err) do + # Used to customize the error message to return to the user + case err do + :no_code -> + "no authorization code found" + + :expired -> + "authorization code has expired" + + _ -> + try do + to_string(err) + rescue + _ -> inspect(err) + end + end + end end diff --git a/test/support/o_auth.ex b/test/support/o_auth.ex index 7620378b4..dd992d591 100644 --- a/test/support/o_auth.ex +++ b/test/support/o_auth.ex @@ -18,10 +18,12 @@ defmodule Teiserver.Test.Support.OAuth do def create_code(user, app, opts \\ []) do {verifier, challenge, method} = generate_challenge() + redir_uri = Map.get(app, :redirect_uri, "http://some.host/a/path") + attrs = %{ id: app.id, scopes: app.scopes, - redirect_uri: "http://some.host/a/path", + redirect_uri: redir_uri, challenge: challenge, challenge_method: method } @@ -33,7 +35,12 @@ defmodule Teiserver.Test.Support.OAuth do defp generate_challenge() do # A-Z,a-z,0-9 and -._~ are authorized, but can't be bothered to cover all # of that. hex encoding will fit - verifier = Base.hex_encode32(:crypto.strong_rand_bytes(40), padding: false) + # hardcoded random bytes generated with :crypto.strong_rand_bytes(32) + bytes = + <<30, 33, 141, 180, 13, 27, 42, 190, 106, 230, 111, 140, 162, 230, 128, 110, 149, 65, 33, + 124, 129, 9, 89, 93, 94, 248, 46, 34, 116, 186, 8, 24>> + + verifier = Base.hex_encode32(bytes, padding: false) challenge = hash_verifier(verifier) diff --git a/test/teiserver/o_auth/code_test.exs b/test/teiserver/o_auth/code_test.exs index a5f632f91..7d8d088fa 100644 --- a/test/teiserver/o_auth/code_test.exs +++ b/test/teiserver/o_auth/code_test.exs @@ -11,7 +11,8 @@ defmodule Teiserver.OAuth.CodeTest do name: "Testing app", uid: "test_app_uid", owner_id: user.id, - scopes: ["tachyon.lobby"] + scopes: ["tachyon.lobby"], + redirect_uri: ["http://localhost/callback"] }) {:ok, user: user, app: app} @@ -41,22 +42,34 @@ defmodule Teiserver.OAuth.CodeTest do test "cannot exchange expired code for token", %{user: user, app: app} do yesterday = Timex.shift(Timex.now(), days: -1) assert {:ok, code, attrs} = OAuthTest.create_code(user, app, now: yesterday) - assert {:error, :expired} = OAuth.exchange_code(code, attrs.verifier) + assert {:error, :expired} = OAuth.exchange_code(code, attrs.verifier, attrs.redirect_uri) end test "must use valid verifier", %{user: user, app: app} do assert {:ok, code, attrs} = OAuthTest.create_code(user, app) - no_match = Base.hex_encode32(:crypto.strong_rand_bytes(38), padding: false) - assert {:error, _} = OAuth.exchange_code(code, no_match) + no_match = + "lollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollollol" + + assert {:error, err} = OAuth.exchange_code(code, no_match, attrs.redirect_uri) + assert err =~ "doesn't match" + end + + test "verifier cannot be too short", %{user: user, app: app} do + assert {:ok, _code, attrs} = OAuthTest.create_code(user, app) verifier = "TOO_SHORT" challenge = OAuthTest.hash_verifier(verifier) {:ok, code} = OAuth.create_code(user, %{attrs | challenge: challenge}) - assert {:error, _} = OAuth.exchange_code(code, verifier) + assert {:error, err} = OAuth.exchange_code(code, verifier, attrs.redirect_uri) + assert err =~ "cannot be less than" + end + test "verifier cannot be too long", %{user: user, app: app} do + assert {:ok, _code, attrs} = OAuthTest.create_code(user, app) verifier = String.duplicate("a", 129) challenge = OAuthTest.hash_verifier(verifier) {:ok, code} = OAuth.create_code(user, %{attrs | challenge: challenge}) - assert {:error, _} = OAuth.exchange_code(code, verifier) + assert {:error, err} = OAuth.exchange_code(code, verifier, attrs.redirect_uri) + assert err =~ "cannot be more than" end end diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index 80ba58ef9..7806a4d44 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -74,10 +74,12 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do resp2 = post(conn, ~p"/oauth/token", data) # code is now used up and invalid - assert json_response(resp2, 400) == %{ - "error_description" => "invalid request", - "error" => "invalid_request" - } + assert %{ + "error" => "invalid_request", + "error_description" => description + } = json_response(resp2, 400) + + assert description =~ "no authorization code" end test "must provide grant_type", %{conn: conn} = setup_data do @@ -116,7 +118,11 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do data = get_valid_data(setup_data) |> Map.put(:client_id, other_app.uid) resp = post(conn, ~p"/oauth/token", data) - assert %{"error" => "invalid_request"} = json_response(resp, 400) + + assert %{"error" => "invalid_request", "error_description" => description} = + json_response(resp, 400) + + assert description =~ "code doesn't match application" end # Note: verifier code and redirect URI matching is tested in OAuth.exchange_code From 802ab08fa1b9717e35de5f9f83ca1abc51d300f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Mon, 5 Aug 2024 08:12:02 +0100 Subject: [PATCH 23/27] Only include state in redirection when provided --- .../o_auth/authorize_controller.ex | 8 +++++++- .../o_auth/authorize_controller_test.exs | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/lib/teiserver_web/controllers/o_auth/authorize_controller.ex b/lib/teiserver_web/controllers/o_auth/authorize_controller.ex index 1e5ccb7bb..33ba7b876 100644 --- a/lib/teiserver_web/controllers/o_auth/authorize_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/authorize_controller.ex @@ -89,7 +89,13 @@ defmodule TeiserverWeb.OAuth.AuthorizeController do query = URI.decode_query(redir_url.query || "") |> Map.put(:code, code.value) - |> Map.merge(Map.take(params, ["state"])) + |> then(fn query -> + # only include state if it was provided in the first place + case Map.get(params, "state", "") do + "" -> query + st -> Map.put(query, "state", st) + end + end) |> URI.encode_query() conn |> redirect(external: URI.to_string(%{redir_url | query: query})) diff --git a/test/teiserver_web/controllers/o_auth/authorize_controller_test.exs b/test/teiserver_web/controllers/o_auth/authorize_controller_test.exs index 76daf2ab9..8b15b3871 100644 --- a/test/teiserver_web/controllers/o_auth/authorize_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/authorize_controller_test.exs @@ -61,6 +61,25 @@ defmodule TeiserverWeb.OAuth.AuthorizeControllerTest do assert code.redirect_uri == data.redirect_uri end + test "only include state in redirection if provided", %{conn: conn, app: app} do + data = %{ + client_id: app.uid, + response_type: "code", + code_challenge: "blah", + code_challenge_method: "S256", + state: "", + redirect_uri: hd(app.redirect_uris) + # no state + } + + resp = post(conn, ~p"/oauth/authorize", data) + + assert redired = redirected_to(resp, 302) + parsed = URI.parse(redired) + query = URI.decode_query(parsed.query) + refute Map.has_key?(query, "state") + end + test "must provide client_id and redirect_uri", %{conn: conn, app: app} do data = %{ client_id: app.uid, From a13c76f0846a960a4a0c020344cab1618c408c8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Mon, 5 Aug 2024 08:26:43 +0100 Subject: [PATCH 24/27] Do not create refresh token when using client credentials See https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.3 --- lib/teiserver/o_auth.ex | 43 +++++++++++-------- lib/teiserver/o_auth/schemas/token.ex | 2 +- .../controllers/o_auth/code_controller.ex | 2 +- .../views/api/o_auth/code_view.ex | 7 ++- test/teiserver/o_auth/token_test.exs | 6 +++ .../o_auth/code_controller_test.exs | 4 +- 6 files changed, 41 insertions(+), 23 deletions(-) diff --git a/lib/teiserver/o_auth.ex b/lib/teiserver/o_auth.ex index 33c874532..6fb8b31e8 100644 --- a/lib/teiserver/o_auth.ex +++ b/lib/teiserver/o_auth.ex @@ -158,7 +158,7 @@ defmodule Teiserver.OAuth do @spec create_token( User.t() | T.userid(), %{id: integer(), scopes: Application.scopes()}, - options() + create_refresh: boolean() | options() ) :: {:ok, Token.t()} | {:error, Ecto.Changeset.t()} def create_token(user_id, application, opts \\ []) @@ -175,33 +175,38 @@ defmodule Teiserver.OAuth do do_create_token(%{owner_id: user_id}, application, opts) end - defp do_create_token(owner_attr, application, opts \\ []) do + defp do_create_token(owner_attr, application, opts) do now = Keyword.get(opts, :now, DateTime.utc_now()) - refresh_attrs = - %{ - value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), - application_id: application.id, - scopes: application.scopes, - # there's no real recourse when the refresh token expires and it's - # quite annoying, so make it "never" expire. - expires_at: Timex.add(now, Timex.Duration.from_days(365 * 100)), - type: :refresh, - refresh_token: nil - } - |> Map.merge(owner_attr) - token_attrs = %{ value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), application_id: application.id, scopes: application.scopes, expires_at: Timex.add(now, Timex.Duration.from_minutes(30)), - type: :access, - refresh_token: refresh_attrs + type: :access } |> Map.merge(owner_attr) + refresh_attrs = + if Keyword.get(opts, :create_refresh, true) do + %{ + value: Base.hex_encode32(:crypto.strong_rand_bytes(32), padding: false), + application_id: application.id, + scopes: application.scopes, + # there's no real recourse when the refresh token expires and it's + # quite annoying, so make it "never" expire. + expires_at: Timex.add(now, Timex.Duration.from_days(365 * 100)), + type: :refresh, + refresh_token: nil + } + |> Map.merge(owner_attr) + else + nil + end + + token_attrs = Map.put(token_attrs, :refresh_token, refresh_attrs) + %Token{} |> Token.changeset(token_attrs) |> Repo.insert() @@ -388,7 +393,9 @@ defmodule Teiserver.OAuth do @spec get_token_from_credentials(Credential.t()) :: {:ok, Token.t()} | {:error, term()} def get_token_from_credentials(credential) do - do_create_token(%{autohost_id: credential.autohost_id}, credential.application) + do_create_token(%{autohost_id: credential.autohost_id}, credential.application, + create_refresh: false + ) end defp check_expiry(obj, now) do diff --git a/lib/teiserver/o_auth/schemas/token.ex b/lib/teiserver/o_auth/schemas/token.ex index 6f1af4e06..0a7aee66d 100644 --- a/lib/teiserver/o_auth/schemas/token.ex +++ b/lib/teiserver/o_auth/schemas/token.ex @@ -12,7 +12,7 @@ defmodule Teiserver.OAuth.Token do scopes: OAuth.Application.scopes(), expires_at: DateTime.t(), type: :access | :refresh, - refresh_token: t() + refresh_token: t() | nil } schema "oauth_tokens" do diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index 967e10d84..279a58ef3 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -7,7 +7,7 @@ defmodule TeiserverWeb.OAuth.CodeController do def token(conn, %{"grant_type" => "authorization_code"} = params) do case Enum.find( - ["client_id", "code", "redirect_uri", "client_id", "code_verifier", "grant_type"], + ["client_id", "code", "redirect_uri", "code_verifier", "grant_type"], fn key -> not Map.has_key?(params, key) end ) do nil -> diff --git a/lib/teiserver_web/views/api/o_auth/code_view.ex b/lib/teiserver_web/views/api/o_auth/code_view.ex index 1df896536..b268705e4 100644 --- a/lib/teiserver_web/views/api/o_auth/code_view.ex +++ b/lib/teiserver_web/views/api/o_auth/code_view.ex @@ -7,9 +7,14 @@ defmodule TeiserverWeb.OAuth.CodeView do %{ access_token: token.value, expires_in: expires_in, - refresh_token: token.refresh_token.value, token_type: "Bearer" } + |> then(fn res -> + case Map.get(token, :refresh_token) do + nil -> res + refresh_token -> Map.put(res, :refresh_token, refresh_token.value) + end + end) end def error(conn) do diff --git a/test/teiserver/o_auth/token_test.exs b/test/teiserver/o_auth/token_test.exs index b4213e8ad..1cf53c6a5 100644 --- a/test/teiserver/o_auth/token_test.exs +++ b/test/teiserver/o_auth/token_test.exs @@ -38,6 +38,12 @@ defmodule Teiserver.OAuth.TokenTest do assert token.scopes == app.scopes end + test "can create a token without refresh token", %{user: user, app: app} do + assert {:ok, token} = OAuth.create_token(user, app, create_refresh: false) + assert token.type == :access + assert token.refresh_token == nil + end + test "can get valid token", %{user: user, app: app} do assert {:ok, new_token} = OAuth.create_token(user, app) assert {:ok, token} = OAuth.get_valid_token(new_token.value) diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index 7806a4d44..ef4de97e6 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -147,7 +147,7 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do json_resp = json_response(resp, 200) assert is_binary(json_resp["access_token"]), "has access_token" assert is_integer(json_resp["expires_in"]), "has expires_in" - assert is_binary(json_resp["refresh_token"]), "has refresh_token" + refute Map.has_key?(json_resp, "refresh_token"), "no refresh token" assert json_resp["token_type"] == "Bearer", "bearer token type" end @@ -175,7 +175,7 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do json_resp = json_response(resp, 200) assert is_binary(json_resp["access_token"]), "has access_token" assert is_integer(json_resp["expires_in"]), "has expires_in" - assert is_binary(json_resp["refresh_token"]), "has refresh_token" + refute Map.has_key?(json_resp, "refresh_token"), "no refresh token" assert json_resp["token_type"] == "Bearer", "bearer token type" end From c50142d8abaeb408318dc85995f3680a81d1320b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Mon, 5 Aug 2024 17:08:22 +0100 Subject: [PATCH 25/27] Allow basic auth to pass client_id for code flow --- .../controllers/o_auth/code_controller.ex | 53 ++++++++++++------- .../o_auth/code_controller_test.exs | 16 ++++++ 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index 279a58ef3..0b9eb76fb 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -6,28 +6,23 @@ defmodule TeiserverWeb.OAuth.CodeController do @spec token(Plug.Conn.t(), %{}) :: Plug.Conn.t() def token(conn, %{"grant_type" => "authorization_code"} = params) do - case Enum.find( - ["client_id", "code", "redirect_uri", "code_verifier", "grant_type"], - fn key -> not Map.has_key?(params, key) end - ) do - nil -> - exchange_token(conn, params) - - missing_key -> - conn |> put_status(400) |> render(:error, error_description: "missing #{missing_key}") + with :ok <- + check_required_keys(params, ["code", "redirect_uri", "code_verifier", "grant_type"]), + {:ok, client_id} <- get_client_id(conn, params) do + exchange_token(conn, client_id, params) + else + {:error, msg} -> + conn |> put_status(400) |> render(:error, error_description: msg) end end def token(conn, %{"grant_type" => "refresh_token"} = params) do - case Enum.find( - ["grant_type", "refresh_token", "client_id"], - fn key -> not Map.has_key?(params, key) end - ) do - nil -> + case check_required_keys(params, ["grant_type", "refresh_token", "client_id"]) do + :ok -> refresh_token(conn, params) - missing_key -> - conn |> put_status(400) |> render(:error, error_description: "missing #{missing_key}") + {:error, msg} -> + conn |> put_status(400) |> render(:error, error_description: msg) end end @@ -45,8 +40,15 @@ defmodule TeiserverWeb.OAuth.CodeController do conn |> put_status(400) |> render(:error, error_description: "invalid grant_type") end - defp exchange_token(conn, params) do - with {:ok, app} <- get_app_by_uid(params["client_id"]), + defp check_required_keys(params, keys) do + case Enum.find(keys, fn key -> not Map.has_key?(params, key) end) do + nil -> :ok + missing_key -> {:error, "missing #{missing_key}"} + end + end + + defp exchange_token(conn, client_id, params) do + with {:ok, app} <- get_app_by_uid(client_id), {:ok, code} <- OAuth.get_valid_code(params["code"]), :ok <- if(code.application_id == app.id, @@ -79,12 +81,25 @@ defmodule TeiserverWeb.OAuth.CodeController do end end + defp get_client_id(conn, params) do + case Map.get(params, "client_id") do + nil -> + case Plug.BasicAuth.parse_basic_auth(conn) do + {user, _pass} -> {:ok, user} + :error -> {:error, "missing client_id"} + end + + cid -> + {:ok, cid} + end + end + defp get_credentials(conn, params) do basic = Plug.BasicAuth.parse_basic_auth(conn) post_params = {Map.get(params, "client_id"), Map.get(params, "client_secret")} case {basic, post_params} do - {:error, {nil, nil}} -> {:error, "unauthorized"} + {:error, {nil, nil}} -> {:error, "Invalid basic auth header"} {{user, pass}, _} -> {:ok, user, pass} {_, {nil, _}} -> {:error, "missing client_id"} {_, {_, nil}} -> {:error, "missing client_secret"} diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index ef4de97e6..db6fef8cc 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -82,6 +82,22 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do assert description =~ "no authorization code" end + test "can use basic auth to exchange", %{conn: conn} = setup_data do + data = get_valid_data(setup_data) + + client_id = data.client_id + data = Map.drop(data, [:client_id]) + auth_header = Plug.BasicAuth.encode_basic_auth(client_id, "") + + resp = + conn + |> put_req_header("authorization", auth_header) + |> post(~p"/oauth/token", data) + + json_resp = json_response(resp, 200) + assert is_binary(json_resp["access_token"]), "has access_token" + end + test "must provide grant_type", %{conn: conn} = setup_data do data = get_valid_data(setup_data) |> Map.drop([:grant_type]) resp = post(conn, ~p"/oauth/token", data) From a97dc99def3ab3960d7cb126951d45f5bb95888e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Mon, 5 Aug 2024 18:52:54 +0100 Subject: [PATCH 26/27] Forbid basic auth and query params at the same time As per https://www.rfc-editor.org/rfc/rfc6749.html#section-2.3 > The client MUST NOT use more than one authentication method > in each request. --- .../controllers/o_auth/code_controller.ex | 47 ++++++++++++++----- .../o_auth/code_controller_test.exs | 46 ++++++++++++++++++ 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index 0b9eb76fb..a92eb147e 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -82,15 +82,21 @@ defmodule TeiserverWeb.OAuth.CodeController do end defp get_client_id(conn, params) do - case Map.get(params, "client_id") do - nil -> - case Plug.BasicAuth.parse_basic_auth(conn) do - {user, _pass} -> {:ok, user} - :error -> {:error, "missing client_id"} - end + query = Map.get(params, "client_id") + basic = Plug.BasicAuth.parse_basic_auth(conn) + + case {basic, query} do + {:error, nil} -> + {:error, "missing client_id"} - cid -> - {:ok, cid} + {{user, _}, client_id} when client_id != nil and user != nil and user != "" -> + {:error, "cannot provide client_id through both basic auth header and query parameters"} + + {{user, _}, nil} -> + {:ok, user} + + {_, client_id} -> + {:ok, client_id} end end @@ -99,11 +105,26 @@ defmodule TeiserverWeb.OAuth.CodeController do post_params = {Map.get(params, "client_id"), Map.get(params, "client_secret")} case {basic, post_params} do - {:error, {nil, nil}} -> {:error, "Invalid basic auth header"} - {{user, pass}, _} -> {:ok, user, pass} - {_, {nil, _}} -> {:error, "missing client_id"} - {_, {_, nil}} -> {:error, "missing client_secret"} - {_, {client_id, client_secret}} -> {:ok, client_id, client_secret} + {:error, {nil, nil}} -> + {:error, "Invalid basic auth header"} + + {{user, _}, {client_id, _}} when user != nil and client_id != nil -> + {:error, "Must not provide client_id both in basic auth header and query parameter"} + + {{_, pass}, {_, secret}} when pass != nil and secret != nil -> + {:error, "Must not provide client_secret both in basic auth header and query parameter"} + + {{user, pass}, _} -> + {:ok, user, pass} + + {_, {nil, _}} -> + {:error, "missing client_id"} + + {_, {_, nil}} -> + {:error, "missing client_secret"} + + {_, {client_id, client_secret}} -> + {:ok, client_id, client_secret} end end diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index db6fef8cc..0aa02be08 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -98,6 +98,20 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do assert is_binary(json_resp["access_token"]), "has access_token" end + test "must not provide client_id twice", %{conn: conn} = setup_data do + data = get_valid_data(setup_data) + + client_id = data.client_id + auth_header = Plug.BasicAuth.encode_basic_auth(client_id, "") + + resp = + conn + |> put_req_header("authorization", auth_header) + |> post(~p"/oauth/token", data) + + json_response(resp, 400) + end + test "must provide grant_type", %{conn: conn} = setup_data do data = get_valid_data(setup_data) |> Map.drop([:grant_type]) resp = post(conn, ~p"/oauth/token", data) @@ -206,6 +220,38 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do resp = post(conn, ~p"/oauth/token", data) json_response(resp, 400) end + + test "cannot provide client_id twice", %{ + conn: conn, + credential: credential, + credential_secret: secret + } do + data = %{grant_type: "client_credentials", client_id: credential.client_id} + auth_header = Plug.BasicAuth.encode_basic_auth(credential.client_id, secret) + + conn = + conn + |> put_req_header("authorization", auth_header) + + resp = post(conn, ~p"/oauth/token", data) + json_response(resp, 400) + end + + test "cannot provide client_secret twice", %{ + conn: conn, + credential: credential, + credential_secret: secret + } do + data = %{grant_type: "client_credentials", client_secret: secret} + auth_header = Plug.BasicAuth.encode_basic_auth(credential.client_id, secret) + + conn = + conn + |> put_req_header("authorization", auth_header) + + resp = post(conn, ~p"/oauth/token", data) + json_response(resp, 400) + end end describe "refresh token" do From a479a143935fbbd5d286436e0f1378a0ad6b6be0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Charvet=20=E9=BB=91=E7=93=9C?= Date: Mon, 5 Aug 2024 19:50:49 +0100 Subject: [PATCH 27/27] Basic check for scopes when refreshing This is incomplete but until teiserver supports more than one scope it is not really possible to test that. It also doesn't really make sense. --- .../controllers/o_auth/code_controller.ex | 27 +++++++++++++++++++ .../o_auth/code_controller_test.exs | 12 +++++++++ 2 files changed, 39 insertions(+) diff --git a/lib/teiserver_web/controllers/o_auth/code_controller.ex b/lib/teiserver_web/controllers/o_auth/code_controller.ex index a92eb147e..51a64dcda 100644 --- a/lib/teiserver_web/controllers/o_auth/code_controller.ex +++ b/lib/teiserver_web/controllers/o_auth/code_controller.ex @@ -16,6 +16,11 @@ defmodule TeiserverWeb.OAuth.CodeController do end end + # As per https://datatracker.ietf.org/doc/html/rfc6749#section-6 this + # endpoint should accept the parameter `scope`. + # As of writing, there is only ever one scope allowed: tachyon.lobby + # so this parameter is ignored. + # If we ever expand the allowed scopes, this feature should be added def token(conn, %{"grant_type" => "refresh_token"} = params) do case check_required_keys(params, ["grant_type", "refresh_token", "client_id"]) do :ok -> @@ -68,6 +73,9 @@ defmodule TeiserverWeb.OAuth.CodeController do defp refresh_token(conn, params) do with {:ok, app} <- get_app_by_uid(params["client_id"]), + # Once we support more than one single scope, modify this function to allow + # changing the scope of the new token + {:ok, _scopes} <- check_scopes(app, params), {:ok, token} <- OAuth.get_valid_token(params["refresh_token"]), :ok <- if(token.application_id == app.id, @@ -137,6 +145,25 @@ defmodule TeiserverWeb.OAuth.CodeController do end end + defp check_scopes(app, params) do + scopes = Map.get(params, "scope", "") |> String.split() |> Enum.into(MapSet.new()) + app_scopes = MapSet.new(app.scopes) + + cond do + MapSet.size(scopes) == 0 -> + {:ok, app.scopes} + + MapSet.subset?(scopes, app_scopes) -> + {:ok, MapSet.to_list(scopes)} + + true -> + invalid_scopes = MapSet.difference(scopes, app_scopes) + + {:error, + "the following scopes aren't allowed: #{inspect(MapSet.to_list(invalid_scopes))}"} + end + end + def metadata(conn, _params) do conn |> put_status(200) |> render(:metadata) end diff --git a/test/teiserver_web/controllers/o_auth/code_controller_test.exs b/test/teiserver_web/controllers/o_auth/code_controller_test.exs index 0aa02be08..60db8ec78 100644 --- a/test/teiserver_web/controllers/o_auth/code_controller_test.exs +++ b/test/teiserver_web/controllers/o_auth/code_controller_test.exs @@ -299,6 +299,18 @@ defmodule TeiserverWeb.OAuth.CodeControllerTest do resp = post(conn, ~p"/oauth/token", data) assert %{"error" => "invalid_request"} = json_response(resp, 400) end + + test "must provide valid scope", %{conn: conn} = setup_data do + data = %{ + grant_type: "refresh_token", + client_id: setup_data[:app].uid, + refresh_token: setup_data[:token].refresh_token.value, + scope: "lolscope" + } + + resp = post(conn, ~p"/oauth/token", data) + assert %{"error" => "invalid_request"} = json_response(resp, 400) + end end describe "medatata endpoint" do