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 only one confirmation #519

Closed
wants to merge 1 commit into from
Closed

allow only one confirmation #519

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 31, 2016

When a user confirms and account (after a registration), if the registration is valid, the user is logged in automatically in the system.

After a successful confirmation the user can still use the confirmation email to login into the system (without a password) and this represents a security flaw.

If the token is valid and if the user is already confirmed, the devise_token_auth should return an expired token. This will raise an auth:email-confirmation-error in ng-token-auth.

This will require #410 because it looks for sign_in counter.

@zachfeldman
Copy link
Contributor

@lynndylanhurley appears to be a decently sized security hole that's also easy to merge!

@zachfeldman
Copy link
Contributor

I'm down to merge this right after #410 if it gets approval from one other person.

@zachfeldman zachfeldman self-requested a review October 9, 2017 15:46
@lynndylanhurley
Copy link
Owner

Changes look good, I'll resolve the conflicts with the test file. We can merge once the conflicts are resolved.

Copy link
Collaborator

@MaicolBen MaicolBen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rmvenancio can you rebase?

@ethagnawl
Copy link
Contributor

It's very encouraging that each issue I've bumped while using this library has either been 1) recently fixed, 2) has a PR pending or 3) has a documented workaround.

👍

@ghost
Copy link
Author

ghost commented Oct 23, 2017

@MaicolBen Hi. I Don't have a Dev environment anymore. I can't help at this time.

@MaicolBen
Copy link
Collaborator

No problem, when I have a chance I'll do it myself

@ghost
Copy link
Author

ghost commented Oct 23, 2017

Thanks for the understanding. Continue with the good work. BR.

@MaicolBen
Copy link
Collaborator

I opened this PR with the conflicts solved (because I can't push to a fork)

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

Successfully merging this pull request may close these issues.

4 participants