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

Allow unconfirmed access #428

Open
augnustin opened this issue Feb 18, 2020 · 8 comments
Open

Allow unconfirmed access #428

augnustin opened this issue Feb 18, 2020 · 8 comments

Comments

@augnustin
Copy link

augnustin commented Feb 18, 2020

With coherence, it was possible to set (from the doc)

:allow_unconfirmed_access_for - number of days to allow login access to the account before confirmation. default 0 (disabled)

Ain't there a similar option in pow? Why not?

@Schultzer
Copy link
Contributor

This is something you could easily deal with in a plug.

I would just diff on the inserted_at, but I don't really know your use case.

It's also very impl. specific. For me, Pow gives you the building blocks to easily add and restructere your impl. without compromsing security or development speed.

@augnustin
Copy link
Author

Hello @Schultzer

Thanks for getting back.

I would just diff on the inserted_at

I don't understand that.

It's also very impl. specific.

How is it? It just does what is expected: login after registration and allow login for N days before getting locked.

It feels to me like the promise of migrating from coherence like a breathe is not held here.

@Schultzer
Copy link
Contributor

Looking closer at the docs you properly have to use the :confirmation_sent_at column from your user table to do the diffing.

This can still easily been done with a plug.

So I would just create a plug and add it to my pipeline right after Pow.Plug.RequireAuthenticated

There might be application that would never require a feature like that, but if you feel strongly about this you could always open a PR with an extension that add this feature.

@augnustin
Copy link
Author

But if I set it after Pow.Plug.RequireAuthenticated, the plug would already have rejected the request and halted the pipeline => I can't make it accepted afterwards, can I?

@Schultzer
Copy link
Contributor

Ohh sorry if that's the case, I would just copy and paste the Pow.Plug.RequireAuthenticated and add your conditional to the process or write your own domain specific plug and not use the default.

@danschultzer
Copy link
Collaborator

It's discussed here: #66 (and also in #323)

The suggested approach won't work as the session is deleted before it reaches Pow.Plug.RequireAuthenticated. This kind of logic is outside the scope of Pow, but I would like for it to be easy to customize. Currently you can only do this with some hacks, or set up a custom registration controller or a custom extension to handle this.

I'm a bit swamped at the moment but I'll take a look at the options when I've some more time to think it through. My thought is to make it customizable with a plug private key in #66 (comment). It makes sense to adjust the flow in the plug, but at the same time I feel this touches a core issue with how to adjust auth flow explicitly.

I do think that I made a mistake by halting flow with PowEmailConfirmation by default. It should probably be optional, since a common UX issue would be if the e-mail has a typo during registration. I may make a change so from v1.1.0 the default behavior would be pass-thru with warning, and in the meanwhile adding a warning to explicitly set up logic to halt the flow if needed.

Let me get back to this soon 😄

@augnustin
Copy link
Author

I forked to allow unconfirmed access, and disabled the automatic resend of confirmation email, which doesn't match the UX we want to provide.

  def resend_confirmation_email(conn, _params) do
    current_user = Pow.Plug.current_user(conn) |> Vae.Repo.preload(:applications)

    PowEmailConfirmation.Phoenix.ControllerCallbacks.send_confirmation_email(current_user, conn)

    conn
    |> put_flash(:success, "L'email de confirmation vient de vous être renvoyé.")
    |> redirect_back(default: '/')
  end

But then I have a new case: if a user triggers this action but has no email_confirmation_token, it fails.

I'll customize that, but if you add the allow_unconfirmed_access feature, do not forget to add something like maybe_generate_email_confirmation_token.

@Schultzer
Copy link
Contributor

I'll customize that, but if you add the allow_unconfirmed_access feature, do not forget to add something like maybe_generate_email_confirmation_token.

This sounds like a can of worms.

I think a better approach would be plugs and custom controllers rather than forking and trying to inject configurations in a codebase you don't have the full scope of yet.

The Idea with plugs, extensions and custom controllers is that you can deal with a problem in an isolated state, that means that you don't have to change everything and can slowly build out Pow to your needs and, at some point maybe even share it with the community.

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

3 participants