From 78925dadaa95b615574e2474709f218989513ce2 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Wed, 1 Mar 2023 21:59:03 -0500 Subject: [PATCH] feat!: Configure accepted fields on register also fixes some .formatter.exs issues and standardizes it using spark tools This is a breaking change because users of previous versions may be relying on this behavior. To upgrade, add `register_action_accept [:additional, :fields]` to your password strategy. This prevents users accidentally adding common security flaw which is accepting more than intended on a given action. --- .formatter.exs | 25 +++- .github/workflows/elixir_lib.yml | 12 ++ lib/ash_authentication/strategies/password.ex | 2 + .../strategies/password/dsl.ex | 5 + .../strategies/password/transformer.ex | 6 + mix.exs | 3 + mix.lock | 2 +- .../20230302020116_migrate_resources2.exs | 23 ++++ .../repo/user/20230302020116.json | 108 ++++++++++++++++++ .../strategies/password/actions_test.exs | 53 +++++++++ test/support/data_case.ex | 4 +- test/support/example/user.ex | 4 + 12 files changed, 244 insertions(+), 3 deletions(-) create mode 100644 priv/repo/migrations/20230302020116_migrate_resources2.exs create mode 100644 priv/resource_snapshots/repo/user/20230302020116.json diff --git a/.formatter.exs b/.formatter.exs index 5f432c2..490e994 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -1,4 +1,6 @@ spark_locals_without_parens = [ + access_token_attribute_name: 1, + access_token_expires_at_attribute_name: 1, api: 1, auth0: 0, auth0: 1, @@ -16,12 +18,16 @@ spark_locals_without_parens = [ confirmation: 2, confirmation_required?: 1, confirmed_at_field: 1, + destroy_action_name: 1, enabled?: 1, expunge_expired_action_name: 1, expunge_interval: 1, get_by_subject_action_name: 1, get_changes_action_name: 1, get_token_action_name: 1, + github: 0, + github: 1, + github: 2, hash_provider: 1, hashed_password_field: 1, identity_field: 1, @@ -30,6 +36,9 @@ spark_locals_without_parens = [ identity_resource: 1, inhibit_updates?: 1, is_revoked_action_name: 1, + magic_link: 0, + magic_link: 1, + magic_link: 2, monitor_fields: 1, oauth2: 0, oauth2: 1, @@ -41,27 +50,41 @@ spark_locals_without_parens = [ password_field: 1, password_reset_action_name: 1, private_key: 1, + read_action_name: 1, read_expired_action_name: 1, redirect_uri: 1, + refresh_token_attribute_name: 1, + register_action_accept: 1, register_action_name: 1, registration_enabled?: 1, + request_action_name: 1, request_password_reset_action_name: 1, require_token_presence_for_authentication?: 1, resettable: 0, resettable: 1, revoke_token_action_name: 1, + select_for_senders: 1, sender: 1, sign_in_action_name: 1, + sign_in_enabled?: 1, signing_algorithm: 1, signing_secret: 1, + single_use_token?: 1, site: 1, store_all_tokens?: 1, store_changes_action_name: 1, store_token_action_name: 1, + strategy_attribute_name: 1, subject_name: 1, token_lifetime: 1, - token_url: 1, + token_param_name: 1, token_resource: 1, + token_url: 1, + uid_attribute_name: 1, + upsert_action_name: 1, + user_id_attribute_name: 1, + user_relationship_name: 1, + user_resource: 1, user_url: 1 ] diff --git a/.github/workflows/elixir_lib.yml b/.github/workflows/elixir_lib.yml index 77bfae6..a385337 100644 --- a/.github/workflows/elixir_lib.yml +++ b/.github/workflows/elixir_lib.yml @@ -40,6 +40,17 @@ jobs: with: mix-env: test + spark-formatter: + name: mix spark.formatter --check + runs-on: ubuntu-latest + needs: build-test + steps: + - uses: actions/checkout@v3 + - uses: team-alembic/staple-actions/actions/mix-task@main + with: + mix-env: test + task: spark.formatter --check + credo: name: mix credo --strict runs-on: ubuntu-latest @@ -113,6 +124,7 @@ jobs: - credo - doctor - formatter + - spark-formatter - auditor - test - dialyzer diff --git a/lib/ash_authentication/strategies/password.ex b/lib/ash_authentication/strategies/password.ex index 500c0de..e9b138f 100644 --- a/lib/ash_authentication/strategies/password.ex +++ b/lib/ash_authentication/strategies/password.ex @@ -106,6 +106,7 @@ defmodule AshAuthentication.Strategy.Password do registration_enabled?: true, sign_in_enabled?: true, resettable: [], + register_action_accept: [], name: nil, provider: :password, resource: nil @@ -130,6 +131,7 @@ defmodule AshAuthentication.Strategy.Password do confirmation_required?: boolean, password_field: atom, password_confirmation_field: atom, + register_action_accept: [atom], register_action_name: atom, sign_in_action_name: atom, registration_enabled?: boolean, diff --git a/lib/ash_authentication/strategies/password/dsl.ex b/lib/ash_authentication/strategies/password/dsl.ex index d7a9081..8c4c531 100644 --- a/lib/ash_authentication/strategies/password/dsl.ex +++ b/lib/ash_authentication/strategies/password/dsl.ex @@ -72,6 +72,11 @@ defmodule AshAuthentication.Strategy.Password.Dsl do """, default: true ], + register_action_accept: [ + type: {:list, :atom}, + default: [], + doc: "A list of additional fields to be accepted in the register action." + ], password_field: [ type: :atom, doc: """ diff --git a/lib/ash_authentication/strategies/password/transformer.ex b/lib/ash_authentication/strategies/password/transformer.ex index db701fc..7803a8f 100644 --- a/lib/ash_authentication/strategies/password/transformer.ex +++ b/lib/ash_authentication/strategies/password/transformer.ex @@ -144,8 +144,14 @@ defmodule AshAuthentication.Strategy.Password.Transformer do [] end + accept = + [strategy.identity_field] + |> Enum.concat(List.wrap(strategy.register_action_accept)) + |> Enum.uniq() + Transformer.build_entity(Resource.Dsl, [:actions], :create, name: strategy.register_action_name, + accept: accept, arguments: arguments, changes: changes, metadata: metadata, diff --git a/mix.exs b/mix.exs index a90e6d5..1fe0cce 100644 --- a/mix.exs +++ b/mix.exs @@ -179,6 +179,9 @@ defmodule AshAuthentication.MixProject do "hex.audit", "test" ], + "spark.formatter": [ + "spark.formatter --extensions AshAuthentication,AshAuthentication.TokenResource,AshAuthentication.UserIdentity,AshAuthentication.Strategy.MagicLink,AshAuthentication.AddOn.Confirmation,AshAuthentication.Strategy.Auth0,AshAuthentication.Strategy.Github,AshAuthentication.Strategy.OAuth2,AshAuthentication.Strategy.Password" + ], docs: ["docs", "ash.replace_doc_links"], test: ["ecto.create --quiet", "ecto.migrate --quiet", "test"] ] diff --git a/mix.lock b/mix.lock index 03b0a1b..6fb7ad7 100644 --- a/mix.lock +++ b/mix.lock @@ -53,7 +53,7 @@ "postgrex": {:hex, :postgrex, "0.16.5", "fcc4035cc90e23933c5d69a9cd686e329469446ef7abba2cf70f08e2c4b69810", [:mix], [{:connection, "~> 1.1", [hex: :connection, repo: "hexpm", optional: false]}, {:db_connection, "~> 2.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "edead639dc6e882618c01d8fc891214c481ab9a3788dfe38dd5e37fd1d5fb2e8"}, "ranch": {:hex, :ranch, "1.8.0", "8c7a100a139fd57f17327b6413e4167ac559fbc04ca7448e9be9057311597a1d", [:make, :rebar3], [], "hexpm", "49fbcfd3682fab1f5d109351b61257676da1a2fdbe295904176d5e521a2ddfe5"}, "sourceror": {:hex, :sourceror, "0.12.1", "239a98ae2ed191528d64e079eaa355f6f1f69318dbb51796e08497dd3b24d10e", [:mix], [], "hexpm", "b5e310385813d0c791e8a481516654a4e10b7a0fdb55b4fc4ef915fbc0899b8f"}, - "spark": {:hex, :spark, "0.4.5", "fbb6e7b30ca38b75a2af4b2c3d167d038c9f8f35ea6d0a014dad6256089f0d62", [:mix], [{:nimble_options, "~> 0.5", [hex: :nimble_options, repo: "hexpm", optional: false]}, {:sourceror, "~> 0.1", [hex: :sourceror, repo: "hexpm", optional: false]}], "hexpm", "948f4370521d04c4f99c46b3c4bb781fa00e1bc5172e76540a37eb1f2b5e75a8"}, + "spark": {:hex, :spark, "0.4.7", "72e2288eb813fd8f84c5339fb99421776512c2a5ed7dc5c191041595208f8559", [:mix], [{:nimble_options, "~> 0.5", [hex: :nimble_options, repo: "hexpm", optional: false]}, {:sourceror, "~> 0.1", [hex: :sourceror, repo: "hexpm", optional: false]}], "hexpm", "ea99174f2d5047f75322a0a2590462c973bb3cf95f31725e5ec1c232ef244d9f"}, "stream_data": {:hex, :stream_data, "0.5.0", "b27641e58941685c75b353577dc602c9d2c12292dd84babf506c2033cd97893e", [:mix], [], "hexpm", "012bd2eec069ada4db3411f9115ccafa38540a3c78c4c0349f151fc761b9e271"}, "telemetry": {:hex, :telemetry, "1.2.1", "68fdfe8d8f05a8428483a97d7aab2f268aaff24b49e0f599faa091f1d4e7f61c", [:rebar3], [], "hexpm", "dad9ce9d8effc621708f99eac538ef1cbe05d6a874dd741de2e689c47feafed5"}, "typable": {:hex, :typable, "0.3.0", "0431e121d124cd26f312123e313d2689b9a5322b15add65d424c07779eaa3ca1", [:mix], [], "hexpm", "880a0797752da1a4c508ac48f94711e04c86156f498065a83d160eef945858f8"}, diff --git a/priv/repo/migrations/20230302020116_migrate_resources2.exs b/priv/repo/migrations/20230302020116_migrate_resources2.exs new file mode 100644 index 0000000..4a1ba09 --- /dev/null +++ b/priv/repo/migrations/20230302020116_migrate_resources2.exs @@ -0,0 +1,23 @@ +defmodule Example.Repo.Migrations.MigrateResources2 do + @moduledoc """ + Updates resources based on their most recent snapshots. + + This file was autogenerated with `mix ash_postgres.generate_migrations` + """ + + use Ecto.Migration + + def up do + alter table(:user) do + add :extra_stuff, :text + add :not_accepted_extra_stuff, :text + end + end + + def down do + alter table(:user) do + remove :not_accepted_extra_stuff + remove :extra_stuff + end + end +end \ No newline at end of file diff --git a/priv/resource_snapshots/repo/user/20230302020116.json b/priv/resource_snapshots/repo/user/20230302020116.json new file mode 100644 index 0000000..2381af4 --- /dev/null +++ b/priv/resource_snapshots/repo/user/20230302020116.json @@ -0,0 +1,108 @@ +{ + "attributes": [ + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "primary_key?": false, + "references": null, + "size": null, + "source": "confirmed_at", + "type": "utc_datetime_usec" + }, + { + "allow_nil?": false, + "default": "fragment(\"uuid_generate_v4()\")", + "generated?": false, + "primary_key?": true, + "references": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "primary_key?": false, + "references": null, + "size": null, + "source": "username", + "type": "citext" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "primary_key?": false, + "references": null, + "size": null, + "source": "extra_stuff", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "primary_key?": false, + "references": null, + "size": null, + "source": "not_accepted_extra_stuff", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "primary_key?": false, + "references": null, + "size": null, + "source": "hashed_password", + "type": "text" + }, + { + "allow_nil?": false, + "default": "fragment(\"now()\")", + "generated?": false, + "primary_key?": false, + "references": null, + "size": null, + "source": "created_at", + "type": "utc_datetime_usec" + }, + { + "allow_nil?": false, + "default": "fragment(\"now()\")", + "generated?": false, + "primary_key?": false, + "references": null, + "size": null, + "source": "updated_at", + "type": "utc_datetime_usec" + } + ], + "base_filter": null, + "check_constraints": [], + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "D59416503FF032F48041F4E1793CD42EE01FF6AB3D5C4FF4A9D9CABA836E96C1", + "identities": [ + { + "base_filter": null, + "index_name": "user_username_index", + "keys": [ + "username" + ], + "name": "username" + } + ], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.Example.Repo", + "schema": null, + "table": "user" +} \ No newline at end of file diff --git a/test/ash_authentication/strategies/password/actions_test.exs b/test/ash_authentication/strategies/password/actions_test.exs index 4676098..4d64c4a 100644 --- a/test/ash_authentication/strategies/password/actions_test.exs +++ b/test/ash_authentication/strategies/password/actions_test.exs @@ -78,6 +78,59 @@ defmodule AshAuthentication.Strategy.Password.ActionsTest do assert claims["sub"] =~ "user?id=#{user.id}" end + test "it can register a new user with additional accepted fields" do + {:ok, strategy} = Info.strategy(Example.User, :password) + + username = username() + password = password() + + assert {:ok, user} = + Actions.register( + strategy, + %{ + "username" => username, + "password" => password, + "password_confirmation" => password, + "extra_stuff" => "Extra" + }, + [] + ) + + assert user.extra_stuff == "Extra" + + assert strategy.hash_provider.valid?(password, user.hashed_password) + + assert {:ok, claims} = Jwt.peek(user.__metadata__.token) + assert claims["sub"] =~ "user?id=#{user.id}" + end + + test "it cant set unaccepted fields" do + {:ok, strategy} = Info.strategy(Example.User, :password) + + username = username() + password = password() + + assert {:error, + %Ash.Error.Invalid{ + errors: [ + %Ash.Error.Changes.InvalidAttribute{ + message: "cannot be changed", + field: :not_accepted_extra_stuff + } + ] + }} = + Actions.register( + strategy, + %{ + "username" => username, + "password" => password, + "password_confirmation" => password, + "not_accepted_extra_stuff" => "Extra" + }, + [] + ) + end + test "it returns an error if the user already exists" do user = build_user() {:ok, strategy} = Info.strategy(Example.User, :password) diff --git a/test/support/data_case.ex b/test/support/data_case.ex index b0ed7ba..782cada 100644 --- a/test/support/data_case.ex +++ b/test/support/data_case.ex @@ -72,17 +72,19 @@ defmodule DataCase do def build_user(attrs \\ []) do password = password() - attrs = + {force_change_attrs, attrs} = attrs |> Map.new() |> Map.put_new(:username, username()) |> Map.put_new(:password, password) |> Map.put_new(:password_confirmation, password) + |> Map.split([:id]) user = Example.User |> Ash.Changeset.new() |> Ash.Changeset.for_create(:register_with_password, attrs) + |> Ash.Changeset.force_change_attributes(force_change_attrs) |> Example.create!() attrs diff --git a/test/support/example/user.ex b/test/support/example/user.ex index f2df0a8..e43a503 100644 --- a/test/support/example/user.ex +++ b/test/support/example/user.ex @@ -23,6 +23,8 @@ defmodule Example.User do uuid_primary_key :id, writable?: true attribute :username, :ci_string, allow_nil?: false + attribute :extra_stuff, :string + attribute :not_accepted_extra_stuff, :string attribute :hashed_password, :string, allow_nil?: true, sensitive?: true, private?: true create_timestamp :created_at @@ -165,6 +167,8 @@ defmodule Example.User do strategies do password do + register_action_accept [:extra_stuff] + resettable do sender fn user, token, _opts -> Logger.debug(