Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot confirm email by setting :email_confirmed_at. #28

Closed
jung-hunsoo opened this issue Sep 18, 2018 · 5 comments
Closed

Cannot confirm email by setting :email_confirmed_at. #28

jung-hunsoo opened this issue Sep 18, 2018 · 5 comments

Comments

@jung-hunsoo
Copy link
Contributor

In the readme of PowEmailConfirmation, manual email confirmation by setting up :email_confirmed_at is described.

But in my test seed it didn't work. I think :email_confirmed_at field is not casted in the changeset.

Seed codes are like this;

user_params = %{name: "na", email: "na@some_url.com", password: "abcd1234", confirm_password: "abcd1234", email_confirmed_at: DateTime.utc_now()}
{:ok, na_user} = Pow.Operations.create(user_params, otp_app: :myapp)

Fortunately, PowEmailConfirmation.Ecto.Context.confirm_email/2 is working as expected.

@danschultzer
Copy link
Collaborator

Yeah, the default changeset is strict and won't permit params that users shouldn't be able to submit (like the :email_confirmed_at value). You're doing it the right way.

There's another way of doing it, but it's much less secure:

defmodule MyApp.Users.User do
  use Ecto.Schema
  use Pow.Ecto.Schema
  use Pow.Extension.Ecto.Schema,
    extensions: [PowEmailConfirmation]

  schema "users" do
    pow_user_fields()

    timestamps()
  end

  def changeset(user_or_changeset, attrs) do
    user
    |> Ecto.Changeset.cast(attrs, [:email_confirmed_at])
    |> pow_changeset(attrs)
    |> pow_extension_changeset(attrs)
  end
end

To highlight the issue with permitting mass assignment, Coherence has a huge security issue due to its lax treatment of user params: smpallen99/coherence#270

@danschultzer
Copy link
Collaborator

I've updated readme, thanks 👍

@jung-hunsoo
Copy link
Contributor Author

Casting fields in changeset should be dealt with care, so I agree your approach is proper.

By the way how about replacing PowEmailConfirmation.Ecto.Context.confirm_email/2 with PowEmailConfirmation.confirm_email/2 or PowEmailConfirmation.Operations.confirm_email/2? It will wrap lower level functions and allow possible independent with Ecto.

@danschultzer
Copy link
Collaborator

That's a great idea. The extensions have been built so they are tightly connected between the plug and ecto APIs, mostly because I wasn't 100% sure how the extensions should be working. Having an Operations module for the extensions makes a lot of sense. Let me reopen this until I can create a PR to add a similar high level API 👍

@danschultzer danschultzer reopened this Sep 18, 2018
@danschultzer
Copy link
Collaborator

I've been looking at this several times, but I think as Pow currently is it works pretty well. There hasn't been a need to decouple extension plug and extension ecto modules so I don't think it's really needed. I'll revisit this if it becomes relevant, but it seems like extensions are so tightly coupled that any modification would in any case require developers to plan carefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants