AF
Ash Framework•3mo ago
aidalgol

Ash Policies around :change_password action

I have a User resource with (mostly) the default update :change_password action generated by the igniter installer, 
update :change_password do
  # Use this action to allow users to change their password by providing
  # their current password and a new password.

  require_atomic? false
  accept []
  argument :current_password, :string, sensitive?: true, allow_nil?: false

  argument :password, :string,
    sensitive?: true,
    allow_nil?: false,
    constraints: [min_length: 8]

  argument :password_confirmation, :string, sensitive?: true, allow_nil?: false

  validate confirm(:password, :password_confirmation)

  validate {AshAuthentication.Strategy.Password.PasswordValidation,
            strategy_name: :password, password_argument: :current_password}

  change {AshAuthentication.Strategy.Password.HashPasswordChange, strategy_name: :password}

  # validates that the password meets our password policy
  if Application.compile_env(:paddy, :enforce_password_policy) do
    validate MyApp.Validations.PasswordAllowed, before_action?: true
  end
end
update :change_password do
  # Use this action to allow users to change their password by providing
  # their current password and a new password.

  require_atomic? false
  accept []
  argument :current_password, :string, sensitive?: true, allow_nil?: false

  argument :password, :string,
    sensitive?: true,
    allow_nil?: false,
    constraints: [min_length: 8]

  argument :password_confirmation, :string, sensitive?: true, allow_nil?: false

  validate confirm(:password, :password_confirmation)

  validate {AshAuthentication.Strategy.Password.PasswordValidation,
            strategy_name: :password, password_argument: :current_password}

  change {AshAuthentication.Strategy.Password.HashPasswordChange, strategy_name: :password}

  # validates that the password meets our password policy
  if Application.compile_env(:paddy, :enforce_password_policy) do
    validate MyApp.Validations.PasswordAllowed, before_action?: true
  end
end
And a LiveComponent (attached) to provide a form to allow the user to change their password. When I start typing in the form, the validation event is triggered, and the LiveView crashes because of an Ash error, 
** (Ash.Error.Unknown) 
Bread Crumbs:
  > action validation {AshAuthentication.Strategy.Password.PasswordValidation, [strategy_name: :password, password_argument: :current_password]}
  > building changeset for MyApp.Accounts.User.change_password
** (Ash.Error.Unknown) 
Bread Crumbs:
  > action validation {AshAuthentication.Strategy.Password.PasswordValidation, [strategy_name: :password, password_argument: :current_password]}
  > building changeset for MyApp.Accounts.User.change_password
For the LiveViews provided by AshAuthenticationPhoenix, the AshAuthentication.Checks.AshAuthenticationInteraction bypass is satisfied, but here I am running actions directly from my code. What would be a safe way to allow access to the password fields for only the password-changen workflow, without exposing it entirely?

SAKMWzE.ex

Solution:
Yes, you could set up a policy like that 👍
Jump to solution
29 Replies
aidalgol
aidalgolOP•3mo ago
I suppose allowing access if the actor is the user we are trying to run the action on, and only for this action. Would that be narrow enough to not compromise security?
Solution
ZachDaniel
ZachDaniel•3mo ago
Yes, you could set up a policy like that 👍
aidalgol
aidalgolOP•3mo ago
Ergh, jumped the gun a bit there. My attempt at implementing this has not worked. I added a policy 
policy action(:change_password) do
  description "Users can change their own password"
  authorize_if expr(id == ^actor(:id))
end
policy action(:change_password) do
  description "Users can change their own password"
  authorize_if expr(id == ^actor(:id))
end
And a field policy, 
field_policy [:hashed_password] do
  description "Users can access password field only for password change"
  authorize_if action(:change_password)
end
field_policy [:hashed_password] do
  description "Users can access password field only for password change"
  authorize_if action(:change_password)
end
But the action is still getting a forbidden field. 
** (Ash.Error.Unknown) 
Bread Crumbs:
  > action validation {AshAuthentication.Strategy.Password.PasswordValidation, [strategy_name: :password, password_argument: :current_password]}
  > building changeset for Paddy.Accounts.User.change_password

Unknown Error

* ** (FunctionClauseError) no function clause matching in AshAuthentication.BcryptProvider.valid?/2
  (ash_authentication 4.9.8) lib/ash_authentication/bcrypt_provider.ex:37: AshAuthentication.BcryptProvider.valid?("badpasswd", #Ash.ForbiddenField<field: :hashed_password, type: :attribute, ...>)
** (Ash.Error.Unknown) 
Bread Crumbs:
  > action validation {AshAuthentication.Strategy.Password.PasswordValidation, [strategy_name: :password, password_argument: :current_password]}
  > building changeset for Paddy.Accounts.User.change_password

Unknown Error

* ** (FunctionClauseError) no function clause matching in AshAuthentication.BcryptProvider.valid?/2
  (ash_authentication 4.9.8) lib/ash_authentication/bcrypt_provider.ex:37: AshAuthentication.BcryptProvider.valid?("badpasswd", #Ash.ForbiddenField<field: :hashed_password, type: :attribute, ...>)
ZachDaniel
ZachDaniel•3mo ago
What other field policies do you have?
aidalgol
aidalgolOP•3mo ago
field_policies do
  field_policy_bypass :* do
    description "AshAuthentication has full access"
    authorize_if AshAuthentication.Checks.AshAuthenticationInteraction
  end

  field_policy_bypass :* do
    description "Admin users have full access"
    authorize_if actor_attribute_equals(:role, :admin)
  end

  field_policy_bypass [:email, :display_name] do
    description "Internal emailers have access"
    authorize_if actor_attribute_equals(:role, :email)
  end

  private_fields :hide

  field_policy [:display_name] do
    description "Display name is always accessible"
    authorize_if always()
  end

  field_policy_bypass [:hashed_password] do
    description "Users can access password field only for password change"
    authorize_if action(:change_password)
  end

  field_policy [:email] do
    description "User email is only accessible by that user"
    authorize_if expr(id == ^actor(:id))
  end
end
field_policies do
  field_policy_bypass :* do
    description "AshAuthentication has full access"
    authorize_if AshAuthentication.Checks.AshAuthenticationInteraction
  end

  field_policy_bypass :* do
    description "Admin users have full access"
    authorize_if actor_attribute_equals(:role, :admin)
  end

  field_policy_bypass [:email, :display_name] do
    description "Internal emailers have access"
    authorize_if actor_attribute_equals(:role, :email)
  end

  private_fields :hide

  field_policy [:display_name] do
    description "Display name is always accessible"
    authorize_if always()
  end

  field_policy_bypass [:hashed_password] do
    description "Users can access password field only for password change"
    authorize_if action(:change_password)
  end

  field_policy [:email] do
    description "User email is only accessible by that user"
    authorize_if expr(id == ^actor(:id))
  end
end
ZachDaniel
ZachDaniel•3mo ago
Ah Its how you're reading the record Whatever read action you're using to bring in the record is causing the problem you'll need to load the hashed password back from the db for example
aidalgol
aidalgolOP•3mo ago
How should I do that?
ZachDaniel
ZachDaniel•3mo ago
Actually, I forget if this is the case, but the hashed password may actually be available in the original_value field of the hashed password, only hidden I don't think so IIRC thats reserved for a specific purpose but otherwise you'll need to do something like pattern match on the value in a before_action hook and Ash.load(...) i.e %{changeset | data: Ash.load!(changeset.data, :hashed_password, authorize?: false} if its a forbidden field
aidalgol
aidalgolOP•3mo ago
Looks like it will need to be a before_transaction, because validations run before before_action. https://hexdocs.pm/ash/actions.html#hook-execution-order
ZachDaniel
ZachDaniel•3mo ago
actually it will need to not be a hook at all because before_transaction also runs after validations
aidalgol
aidalgolOP•3mo ago
Am I reading the docs wrong, then? I have it working with assign_form/1 in the above component changed to this. 
  defp assign_form(%{assigns: %{current_user: user}} = socket) do
    form =
      AshPhoenix.Form.for_update(user, :change_password,
        as: "user",
        prepare_source: fn changeset ->
          %{changeset | data: Ash.load!(changeset.data, :hashed_password, authorize?: false)}
        end,
        actor: user
      )

    assign(socket, form: to_form(form))
  end
  defp assign_form(%{assigns: %{current_user: user}} = socket) do
    form =
      AshPhoenix.Form.for_update(user, :change_password,
        as: "user",
        prepare_source: fn changeset ->
          %{changeset | data: Ash.load!(changeset.data, :hashed_password, authorize?: false)}
        end,
        actor: user
      )

    assign(socket, form: to_form(form))
  end
Should probably do this properly with policies now. I have gone with a similar pattern to AshAuthentication's check. 
defmodule MyApp.Checks.PasswordChangeInteraction do
  use Ash.Policy.SimpleCheck

  @impl Ash.Policy.Check
  def describe(_) do
    "MyApp is performing a password change for this interaction"
  end

  @impl Ash.Policy.SimpleCheck
  def match?(_, %{subject: %{context: %{private: %{password_change?: true}}}}, _), do: true

  def match?(_, _map, _), do: false
end
defmodule MyApp.Checks.PasswordChangeInteraction do
  use Ash.Policy.SimpleCheck

  @impl Ash.Policy.Check
  def describe(_) do
    "MyApp is performing a password change for this interaction"
  end

  @impl Ash.Policy.SimpleCheck
  def match?(_, %{subject: %{context: %{private: %{password_change?: true}}}}, _), do: true

  def match?(_, _map, _), do: false
end
defp assign_form(%{assigns: %{current_user: user}} = socket) do
  form =
    AshPhoenix.Form.for_update(user, :change_password,
      as: "user",
      prepare_source: fn changeset ->
        %{
          changeset
          | data:
              Ash.load!(changeset.data, :hashed_password,
                context: %{private: %{password_change?: true}}
              )
        }
      end,
      actor: user
    )

  assign(socket, form: to_form(form))
end
defp assign_form(%{assigns: %{current_user: user}} = socket) do
  form =
    AshPhoenix.Form.for_update(user, :change_password,
      as: "user",
      prepare_source: fn changeset ->
        %{
          changeset
          | data:
              Ash.load!(changeset.data, :hashed_password,
                context: %{private: %{password_change?: true}}
              )
        }
      end,
      actor: user
    )

  assign(socket, form: to_form(form))
end
ZachDaniel
ZachDaniel•3mo ago
I would set the context in the form but load the hashed_password in the action that way if you ever call that action you don't have to also load hashed_password in the caller
aidalgol
aidalgolOP•3mo ago
I thought that couldn't be done because all hooks are run after validations.
ZachDaniel
ZachDaniel•3mo ago
Not all change logic happens in a hook Actually...your way is better for now
aidalgol
aidalgolOP•3mo ago
Where else can you change the changeset outside of hooks that also happens before validations?
ZachDaniel
ZachDaniel•3mo ago
just in the body of a change function 
change fn changeset, _ -> 
  changeset
end
change fn changeset, _ -> 
  changeset
end
aidalgol
aidalgolOP•3mo ago
The exact same changeset function does not work if I put it in a change function in the action. 
change fn changeset, _ ->
  %{
    changeset
    | data:
        Ash.load!(changeset.data, :hashed_password,
          context: %{private: %{password_change?: true}}
        )
  }
end
change fn changeset, _ ->
  %{
    changeset
    | data:
        Ash.load!(changeset.data, :hashed_password,
          context: %{private: %{password_change?: true}}
        )
  }
end
If I tack |> IO.inspect(pretty: true, structs: false) on to the end, I see that hashed_password on changeset.data is a forbidden field.
ZachDaniel
ZachDaniel•3mo ago
Did you put it before the validation?
aidalgol
aidalgolOP•3mo ago
Yes
ZachDaniel
ZachDaniel•3mo ago
Hm...I'd have to investigate Your way works as a reasonable approach though.
aidalgol
aidalgolOP•3mo ago
Yeah, I'm happy with it for now, but it feels like it could be a bit cleaner still. Thanks for your help! This actually doesn't work either. I think when I thought it had worked, my liveview was in the old state from when I had authorize?: false. So why is this field policy bypass insufficient? 
field_policy_bypass :* do
  description "Users can access password field for password change"
  authorize_if MyApp.Checks.PasswordChangeInteraction
end
field_policy_bypass :* do
  description "Users can access password field for password change"
  authorize_if MyApp.Checks.PasswordChangeInteraction
end
ZachDaniel
ZachDaniel•3mo ago
It's about the read action not the update action The read action is what needs to allow viewing the changing password in this case
aidalgol
aidalgolOP•3mo ago
Right, but shouldn't that policy make this load work? 
AshPhoenix.Form.for_update(user, :change_password,
  as: "user",
  prepare_source: fn changeset ->
    %{
      changeset
      | data:
          Ash.load!(changeset.data, :hashed_password,
            context: %{private: %{password_change?: true}}
          )
    }
  end,
  actor: user
)
AshPhoenix.Form.for_update(user, :change_password,
  as: "user",
  prepare_source: fn changeset ->
    %{
      changeset
      | data:
          Ash.load!(changeset.data, :hashed_password,
            context: %{private: %{password_change?: true}}
          )
    }
  end,
  actor: user
)
I confirmed that the match?/3 clause that returns true is being hit by adding some logging. https://discord.com/channels/711271361523351632/1398837010848026644/1398884659286249593
ZachDaniel
ZachDaniel•3mo ago
Yes I think so Is the data coming back without hashed passed filled in?
aidalgol
aidalgolOP•3mo ago
Yes.
ZachDaniel
ZachDaniel•3mo ago
🤔 🤔 🤔 weird This might be a bug then 😢 Can you add require_atomic? false to the update action? It may already have it thoguh
aidalgol
aidalgolOP•3mo ago
Yep, already there.
ZachDaniel
ZachDaniel•3mo ago
could you reproduce this perhaps and I can look into it?
aidalgol
aidalgolOP•3mo ago
No need. Found the culprit! In field_policies, private_fields :hide needs to be private_fields :include.

Did you find this page helpful?