Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
FEATURE: require admins to re-validate their email addresses if they …
…haven't been seen for a number of days, configurable with the invalidate_inactive_admin_email_after_days site setting. Social logins are also revoked. Default is 365 days.
- Loading branch information
Showing
4 changed files
with
99 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
module Jobs | ||
|
||
class InvalidateInactiveAdmins < Jobs::Scheduled | ||
every 1.day | ||
|
||
def execute(_) | ||
return if SiteSetting.invalidate_inactive_admin_email_after_days == 0 | ||
|
||
User.human_users | ||
.where(admin: true) | ||
.where('last_seen_at < ?', SiteSetting.invalidate_inactive_admin_email_after_days.days.ago) | ||
.each do |user| | ||
|
||
user.email_tokens.update_all(confirmed: false, expired: true) | ||
|
||
Discourse.authenticators.each do |authenticator| | ||
if authenticator.can_revoke? && authenticator.description_for_user(user).present? | ||
authenticator.revoke(user) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
|
||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
require 'rails_helper' | ||
|
||
require_dependency 'jobs/scheduled/invalidate_inactive_admins' | ||
|
||
describe Jobs::InvalidateInactiveAdmins do | ||
let!(:active_admin) { Fabricate(:admin, last_seen_at: 1.hour.ago) } | ||
before { active_admin.email_tokens.update_all(confirmed: true) } | ||
|
||
subject { Jobs::InvalidateInactiveAdmins.new.execute({}) } | ||
|
||
it "does nothing when all admins have been seen recently" do | ||
SiteSetting.invalidate_inactive_admin_email_after_days = 365 | ||
subject | ||
expect(active_admin.active).to eq(true) | ||
expect(active_admin.email_tokens.where(confirmed: true).exists?).to eq(true) | ||
end | ||
|
||
context "with an admin who hasn't been seen recently" do | ||
let!(:not_seen_admin) { Fabricate(:admin, last_seen_at: 370.days.ago) } | ||
before { not_seen_admin.email_tokens.update_all(confirmed: true) } | ||
|
||
context 'invalidate_inactive_admin_email_after_days = 365' do | ||
before do | ||
SiteSetting.invalidate_inactive_admin_email_after_days = 365 | ||
end | ||
|
||
it 'marks email tokens as unconfirmed and keeps user as active' do | ||
subject | ||
expect(not_seen_admin.email_tokens.where(confirmed: true).exists?).to eq(false) | ||
end | ||
|
||
it 'keeps the user active' do | ||
subject | ||
expect(not_seen_admin.active).to eq(true) | ||
end | ||
|
||
context 'with social logins' do | ||
before do | ||
GithubUserInfo.create!(user_id: not_seen_admin.id, screen_name: 'bob', github_user_id: 100) | ||
InstagramUserInfo.create!(user_id: not_seen_admin.id, screen_name: 'bob', instagram_user_id: 'examplel123123') | ||
UserOpenId.create!(url: 'https://me.yahoo.com/id/123' , user_id: not_seen_admin.id, email: 'bob@example.com', active: true) | ||
GoogleUserInfo.create!(user_id: not_seen_admin.id, google_user_id: 100, email: 'bob@example.com') | ||
end | ||
|
||
it 'removes the social logins' do | ||
subject | ||
expect(GithubUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false) | ||
expect(InstagramUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false) | ||
expect(GoogleUserInfo.where(user_id: not_seen_admin.id).exists?).to eq(false) | ||
expect(UserOpenId.where(user_id: not_seen_admin.id).exists?).to eq(false) | ||
end | ||
end | ||
end | ||
|
||
context 'invalidate_inactive_admin_email_after_days = 0 to disable this feature' do | ||
before do | ||
SiteSetting.invalidate_inactive_admin_email_after_days = 0 | ||
end | ||
|
||
it 'does nothing' do | ||
subject | ||
expect(active_admin.active).to eq(true) | ||
expect(active_admin.email_tokens.where(confirmed: true).exists?).to eq(true) | ||
expect(not_seen_admin.active).to eq(true) | ||
expect(not_seen_admin.email_tokens.where(confirmed: true).exists?).to eq(true) | ||
end | ||
end | ||
end | ||
end |
a1db15f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel this is quite strong enough. Why not update the admin as
active = false
? This change only means they will need to re-connect social logins, they will still be able to login using username/password without email confirmation as far as I can tell.a1db15f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a1db15f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed up here: https://review.discourse.org/t/fix-invalidating-inactive-admin-emails-should-mark-them-as-not-active/724
looks good to me!
a1db15f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neil Lalonde posted:
What other bit?
[quote="SamSaffron, post:3, topic:723"]
they will still be able to login using username/password without email confirmation as far as I can tell.
[/quote]
That's not the case in my testing. The behaviour is identical to deactivating. (Both cases show the same modal as if you had signed up but not clicked the activation link yet, which is not great.)