use created_at for checking active_for_authentication

[nhattan]

If we set config.confirm_within = 1.days and config.allow_unconfirmed_access_for = 1.days, then a user can just trigger a resend_confirmation_instructions which will cause the update field confirmation_sent_at to be updated.

And since that gets set to Time.now.utc, the user is now able to log in for the next 24h, without confirming the email address.

Using created_at instead of confirmation_sent_at would help to solve the issue.

[nhattan]

Maybe it will solve this issue: #4536

[feliperenan]

Hi @nhattan, thanks for the pull request.

Could you explain the motivation behind this PR? This is going to help us understand what this patch is solving and make the properly code review.

Thanks :)

[nhattan]

@feliperenan I updated the description. Please help to take a look. Thanks!

[rubyrider]

I think created_at is ambiguous/misleading to consider for this scenario. So what will be the case for the 'Shadow' User who are created from leads or other source and they become real user later ? I think it should be done considering the confirmation_sent_at date instead of created_at timestamp.

[nhattan]

@rubyrider Unfortunately, confirmation_sent_at gets updated to Time.now.utc every time user triggers a resend_confirmation_instructions which causes the issue, using created_at would help to solve it. In case created_at is ambiguous, WDYT if we add a separate column called first_confirmation_sent_at? @feliperenan

[rubyrider]

@rubyrider Unfortunately, confirmation_sent_at gets updated to Time.now.utc every time user triggers a resend_confirmation_instructions which causes the issue, using created_at would help to solve it. In case created_at is ambiguous, WDYT if we add a separate column called first_confirmation_sent_at? @feliperenan

@nhattan I got your point. I would suggest to create a field confirmation_resend_at and confirmation_sent_count instead of first_confirmation_sent_at ?