Phoenix LiveView and Ecto changeset associations

Phoenix Forum Questions / Help
,
1

Hello,

I’ve been in the process of rewriting parts of my frontend with LiveView (written with React/Relay until then). I’m really happy how things turned out so far and it has been a a lot of fun to dive into LiveView:

Now I did rewrite some CRUD controller logic to leverage real-time validation of forms and was wondering if I’m doing things wrong when working with changeset associations…

For example, I have a Comment schema:

  schema "comments" do
    belongs_to :repo, Repo
    belongs_to :author, User
    belongs_to :parent, __MODULE__
    has_many :children, __MODULE__, foreign_key: :parent_id
    has_many :revisions, CommentRevision
    field :body, :string
    timestamps()
  end

and a very basic changeset for this schema:

  @doc """
  Returns a changeset for the given `params`.
  """
  @spec changeset(t, map) :: Ecto.Changeset.t
  def changeset(%__MODULE__{} = comment, params \\ %{}) do
    comment
    |> cast(params, [:repo_id, :author_id, :parent_id, :body])
    |> validate_required([:repo_id, :author_id, :body])
    |> assoc_constraint(:repo)
    |> assoc_constraint(:author)
    |> assoc_constraint(:parent)
  end

So far, when working with changesets with associations, I’ve been leaving fields such as :repo_id and :author_id when creating new changeset and add them in my controller before inserting the model into the database.

# in controller new/2
changeset = Comment.changeset(%Comment{})

# in controller create/2
comments_params = Map.put(comment_params, "repo_id", repo.id)
comments_params = Map.put(comment_params, "author_id", current_user(conn).id)
changeset = Comment.changeset(%Comment{}, comment_params)

Not that the changeset in new/2 is actually not valid and has few errors regarding association fields:

#Ecto.Changeset<
  action: nil,
  changes: %{},
  errors: [
    repo_id: {"can't be blank", [validation: :required]},
    author_id: {"can't be blank", [validation: :required]},
    body: {"can't be blank", [validation: :required]}
  ],
  data: #GitGud.Comment<>,
  valid?: false
>

When using this changeset, the errors where not displayed when rendering the <form> until :action is set to :insert.

But now I’d like to use the @changeset state in my LiveView to disable the submit button if the changeset is not valid:

<%= submit "Add comment", class: "button is-success", disabled: !@changeset.valid? %>

In order to do so, I need to fix errors for :repo_id and :author_id when initiating the changeset otherwise the changeset will also be invalid…

So here’s the actual question of my post :wink:.

When working with changesets with fields that should not be rendered in the <form>, is it a good approach to pass these fields directly to the data struct:

changeset = Comment.changeset(
  %Comment{
    repo_id: repo.id,
    author_id: current_user(socket).id
  }
)

Or better pass them as parameters:

changeset = Comment.changeset(
  %Comment{},
  %{"repo_id" => repo.id, "author_id" => current_user(socket).id}
)

Or remove :repo_id and :author_id from the changeset entirely and always set them manually before persisting to the database:

  @doc """
  Returns a changeset for the given `params`.
  """
  @spec changeset(t, map) :: Ecto.Changeset.t
  def changeset(%__MODULE__{} = comment, params \\ %{}) do
    comment
    |> cast(params, [:parent_id, :body])
    |> validate_required([:body])
    |> assoc_constraint(:parent)
  end

# in controller new/2
changeset = Comment.changeset(%Comment{})

# in controller create/2
changeset = Comment.changeset(
  %Comment{
    repo_id: repo.id,
    author_id: current_user(socket).id
  },
  comment_params
)

Thank you in advance :heart:

2 Likes
2

You can also use hidden_input and render them in forms that way (though I’m actually not sure if that opens you up to attack?)

3

Given the principle of “never trust user input”, do this where you can. simplify your logic, changesets too.

unrelated security point very simplified that I wanted to highlight to phoenix+ecto community for a while:

Ecto changesets suffer from bulk assignment threat vectors, although you have a white list within them. Each item would not be applicable to every use case.

stuff {
  relationship_id int
  quantity int
  status enum
}

Imagine you had the following controller like many of us do

form_prams = whatever the pattern match got us from http payload
Create(form_params)

I could order something approved for another user, associating validation would only check if the XX exists in DB then succeed.

POST relationship_id=XX, status=approved, quantity=99

The ideal scenario would be your controller managing the relationship intrinsically and making sure things exist and set up the right relationships with the right SCOPE.

controller

item = fetch_for_user_or_scope_with_bang!(current_user)
{"quantity" => quantity} = form_params

# now the ecto changeset can do proper validation logic
Context.create_changeset(relationship_id=item.id, status=pending, quantity=quantity)

create_changeset
validate quantity between 0 and 10

When you need to approve the status

controller

item = fetch_for_user_or_scope_with_bang!(current_user)
{"status" => status} = form_params

# now the ecto changeset can do proper validation logic
Context.update_changeset(rstatus=status, role=current_user.role)

update_changeset
validate status in [approved, rejected]
validate role in [approver]

Now you are safe against bulk assignments, simplified your changesets and catered per use case, as well as decreasing your wire traffic.

ps: role isn’t the best validation for ecto changeset but imagine quota or anything else changeset, db related.

1 Like
4

It does, never use hidden_input other than that things like CSRF tokens

2 Likes
5

Hi @cenotaph and thank you for the reply.

While I’ve been following the “never trust user input” mantra so far (no hidden inputs or query params for passing things around), I’m still not sure how to simplify my changesets…

I’ve been building Web apps with Phoenix for some years now and always thought that having all the association stuff in my changeset was the way to go.

From my understanding, I could simply remove all the fields that are not meant to be set/changed by the user from the changesets. But what is the actual correct way of passing theses params to the changeset when working with Ecto.Repo.insert/2?

Simply passing the schema struct with initialised values?

changeset = Comment.changeset(
  %Comment{repo_id: repo_id, author_id: author_id},
  param
)

DB.insert(changeset)
1 Like
6

I like to pass the “scope” as argument. In fact, I have a rule: all context functions must receive at least one scope as argument. For some apps, this means the “organization” or “current user” will always be given. This means you will remember to properly scope your queries, inserts, etc. and when the scope is ignored, you can add a comment on why it is safe to do so.

In your case, the author_id would likely come from the scope. Then you may have additional scopes, such as the repo.

7 Likes
7

My pleasure, my notes were more for secure development in any language rather than elixir + ecto.

That is the correct way #1.

If you went a bit philosophical and actually changed things via a changeset

param = %{repo_id: repo_id, author_id: author_id}
changeset = Comment.changeset(
  %Comment{},
  param
)

That would be #2 of the correct way.

Whichever floats your boat. The second might give you some more validation too as you can control via the changeset validation.

Never trust user input - including the most destructive user - developer themselves :wink:

3 Likes
8

That is another important fact and I have been making use of this greatly with the plugs, pipelines and scopes for the endpoints. One of the rare occasions of every gear fitting nicely in engineering!