maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Remove <provider>_created_at columns (#13264)

Browse source 
* Remove <provider>_created_at columns from User

* Update code

* Clean up

* Fix authenticator specs

* Restore old behavior

* Update provider account age logic, again
This commit is contained in:
Michael Kohl 2021-04-29 22:24:16 +07:00 • committed by GitHub
parent 5af33a63dd
commit 3173bb4bc2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 26 additions and 109 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
app/black_box/black_box.rb
View file

@ -37,14 +37,7 @@ class BlackBox
end
def calculate_spaminess(story)
user = story.user
return 100 unless user
return 0 if user.trusted
return 0 if user.badge_achievements_count.positive?
base_spaminess = 0
base_spaminess += 25 if social_auth_registration_recent?(user) && user.registered_at > 25.days.ago
base_spaminess
story.user ? 0 : 100
end
private
@ -62,11 +55,5 @@ class BlackBox
([article.comment_score, 650].min * 2) -
(article.spaminess_rating * 5)
end
def social_auth_registration_recent?(user)
# was the social auth account created very recently?
social_auth_date_plus_two_days = ((user.github_created_at || user.twitter_created_at || 3.days.ago) + 2.days)
user.registered_at < social_auth_date_plus_two_days
end
end
end

5
app/controllers/admin/feedback_messages_controller.rb
View file

@ -17,10 +17,7 @@ module Admin
.where("score > ? AND score < ?", -10, 8)
.limit(120)
@possible_spam_users = User.registered.where(
"github_created_at > ? OR twitter_created_at > ? OR length(name) > ?",
50.hours.ago, 50.hours.ago, 30
)
@possible_spam_users = User.registered.where("length(name) > ?", 30)
.where("created_at > ?", 48.hours.ago)
.order(created_at: :desc)
.select(:username, :name, :id)

9
app/models/user.rb
View file

@ -36,7 +36,14 @@ class User < ApplicationRecord
youtube_url
].freeze
self.ignored_columns = PROFILE_COLUMNS
PROVIDER_COLUMNS = %w[
apple_created_at
facebook_created_at
github_created_at
twitter_created_at
].freeze
self.ignored_columns = PROFILE_COLUMNS + PROVIDER_COLUMNS
# NOTE: @citizen428 This is temporary code during profile migration and will
# be removed.

6
app/services/authentication/authenticator.rb
View file

@ -146,10 +146,8 @@ module Authentication
user.profile_updated_at = Time.current if user.public_send(field_name)
end
def account_less_than_a_week_old?(user, logged_in_identity)
provider_created_at = user.public_send(provider.user_created_at_field)
user_identity_age = provider_created_at
user_identity_age ||= extract_created_at_from_payload(logged_in_identity)
def account_less_than_a_week_old?(_user, logged_in_identity)
user_identity_age = extract_created_at_from_payload(logged_in_identity)
# last one is a fallback in case both are nil
range = 1.week.ago.beginning_of_day..Time.current

10
app/services/authentication/providers/apple.rb
View file

@ -10,11 +10,9 @@ module Authentication
def new_user_data
# Apple sends `first_name` and `last_name` as separate fields
name = "#{info.first_name} #{info.last_name}"
timestamp = raw_info.id_info.auth_time
user_data = {
email: info.email,
apple_created_at: Time.zone.at(timestamp),
apple_username: user_nickname,
name: name
}
@ -33,12 +31,10 @@ module Authentication
# the first login. To cover the case where a user disconnects their
# Apple authorization, signs in again and then changes their name,
# we update the username only if the name is not nil
apple_username = info.first_name.present? ? info.first_name.downcase : nil
timestamp = raw_info.id_info.auth_time
apple_username = info.first_name&.downcase
return {} unless apple_username
data = { apple_created_at: Time.zone.at(timestamp) }
data[:apple_username] = apple_username if apple_username
data
{ apple_username: apple_username }
end
# For Apple we override this method because the `info` payload doesn't

6
app/services/authentication/providers/facebook.rb
View file

@ -11,15 +11,13 @@ module Authentication
name: @info.name,
email: @info.email || "",
remote_profile_image_url: Users::SafeRemoteProfileImageUrl.call(image_url),
facebook_username: user_nickname,
facebook_created_at: Time.zone.now
facebook_username: user_nickname
}
end
def existing_user_data
{
facebook_username: @info.name,
facebook_created_at: Time.zone.now
facebook_username: @info.name
}
end

2
app/services/authentication/providers/github.rb
View file

@ -10,7 +10,6 @@ module Authentication
{
email: info.email.to_s,
github_created_at: raw_info.created_at,
github_username: info.nickname,
name: name,
remote_profile_image_url: Users::SafeRemoteProfileImageUrl.call(info.image.to_s)
@ -19,7 +18,6 @@ module Authentication
def existing_user_data
{
github_created_at: raw_info.created_at,
github_username: info.nickname
}
end

6
app/services/authentication/providers/provider.rb
View file

@ -3,7 +3,7 @@ module Authentication
# Authentication provider
class Provider
delegate :email, to: :info, prefix: :user
delegate :user_created_at_field, :user_username_field, to: :class
delegate :user_username_field, to: :class
def initialize(auth_payload)
@auth_payload = cleanup_payload(auth_payload.dup)
@ -37,10 +37,6 @@ module Authentication
name.demodulize.downcase.to_sym
end
def self.user_created_at_field
"#{provider_name}_created_at".to_sym
end
def self.user_username_field
"#{provider_name}_username".to_sym
end

2
app/services/authentication/providers/twitter.rb
View file

@ -12,14 +12,12 @@ module Authentication
email: info.email.to_s,
name: name,
remote_profile_image_url: Users::SafeRemoteProfileImageUrl.call(remote_profile_image_url),
twitter_created_at: raw_info.created_at,
twitter_username: info.nickname
}
end
def existing_user_data
{
twitter_created_at: raw_info.created_at,
twitter_username: info.nickname
}
end

34
spec/black_box/black_box_spec.rb
View file

@ -37,39 +37,5 @@ RSpec.describe BlackBox, type: :black_box do
story = instance_double("Comment", user: nil)
expect(described_class.calculate_spaminess(story)).to eq(100)
end
it "returns 25 if there is a recent github_created_at" do
user = create(:user)
user.update_column(:github_created_at, 1.day.ago)
user.update_column(:registered_at, 1.hour.ago)
article = create(:article, score: 99, published_at: Time.current, user: user)
expect(described_class.calculate_spaminess(article)).to eq(25)
end
it "returns 0 if there is non-recent github_created_at " do
user = create(:user)
user.update_column(:github_created_at, 10.days.ago)
user.update_column(:registered_at, 1.hour.ago)
article = create(:article, score: 99, published_at: Time.current)
expect(described_class.calculate_spaminess(article)).to eq(0)
end
it "returns 0 if user is trusted even if new social" do
user = create(:user)
user.update_column(:github_created_at, 1.day.ago)
user.update_column(:registered_at, 1.hour.ago)
user.add_role(:trusted)
article = create(:article, score: 99, published_at: Time.current)
expect(described_class.calculate_spaminess(article)).to eq(0)
end
it "returns 0 if badge_achievements_count is high" do
user = create(:user)
user.update_column(:github_created_at, 1.day.ago)
user.update_column(:registered_at, 1.hour.ago)
user.update_column(:badge_achievements_count, 2)
article = create(:article, score: 99, published_at: Time.current)
expect(described_class.calculate_spaminess(article)).to eq(0)
end
end
end

5
spec/models/user_spec.rb
View file

@ -588,11 +588,6 @@ RSpec.describe User, type: :model do
end
end
it "persists extracts relevant identity data from new twitter user" do
new_user = user_from_authorization_service(:twitter, nil, "navbar_basic")
expect(new_user.twitter_created_at).to be_kind_of(ActiveSupport::TimeWithZone)
end
it "assigns multiple identities to the same user", :aggregate_failures, vcr: { cassette_name: "fastly_sloan" } do
providers = Authentication::Providers.available

35
spec/services/authentication/authenticator_spec.rb
View file

@ -36,12 +36,10 @@ RSpec.describe Authentication::Authenticator, type: :service do
user = service.call
info = auth_payload.info
raw_info = auth_payload.extra.raw_info
expect(user.email).to eq(info.email)
expect(user.name).to eq("#{info.first_name} #{info.last_name}")
expect(user.profile_image).not_to be_nil
expect(user.apple_created_at.to_i).to eq(raw_info.id_info.auth_time)
expect(user.apple_username).to match(/#{info.first_name.downcase}_\w+/)
end
@ -130,13 +128,11 @@ RSpec.describe Authentication::Authenticator, type: :service do
it "updates the proper data from the auth payload" do
# simulate changing apple data
auth_payload.extra.raw_info.id_info.auth_time = 10.days.ago.to_i
auth_payload.info.first_name = "Newname"
user = described_class.call(auth_payload)
raw_info = auth_payload.extra.raw_info
expect(user.apple_created_at.to_i).to eq(raw_info.id_info.auth_time)
expect do
described_class.call(auth_payload)
end.to change { user.reload.apple_username }.to("newname")
end
it "sets remember_me for the existing user" do
@ -239,7 +235,6 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.email).to eq(info.email)
expect(user.name).to eq(raw_info.name)
expect(user.remote_profile_image_url).to eq(info.image)
expect(user.github_created_at.to_i).to eq(Time.zone.parse(raw_info.created_at).to_i)
expect(user.github_username).to eq(info.nickname)
end
@ -419,15 +414,6 @@ RSpec.describe Authentication::Authenticator, type: :service do
let!(:auth_payload) { OmniAuth.config.mock_auth[:facebook] }
let!(:service) { described_class.new(auth_payload) }
# Freeze time since `facebook_created_at` will be based on server time
before do
Timecop.freeze
end
after do
Timecop.return
end
describe "new user" do
it "creates a new user" do
expect do
@ -450,7 +436,6 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.email).to eq(info.email)
expect(user.name).to eq(raw_info.name)
expect(user.remote_profile_image_url).to eq(info.image)
expect(user.facebook_created_at.to_i).to eq(Time.zone.now.to_i)
expect(user.facebook_username).to match(/#{info.name.sub(' ', '_')}_\S*\z/)
end
@ -540,7 +525,6 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.email).to eq(info.email)
expect(user.name).to eq(raw_info.name)
expect(user.remote_profile_image_url).to eq(info.image.to_s.gsub("_normal", ""))
expect(user.twitter_created_at.to_i).to eq(Time.zone.parse(raw_info.created_at).to_i)
expect(user.twitter_username).to eq(info.nickname)
end
@ -629,14 +613,11 @@ RSpec.describe Authentication::Authenticator, type: :service do
it "updates the proper data from the auth payload" do
# simulate changing twitter data
auth_payload.extra.raw_info.followers_count = rand(100).to_s
auth_payload.extra.raw_info.friends_count = rand(100).to_s
auth_payload.info.nickname = "newnick"
user = described_class.call(auth_payload)
raw_info = auth_payload.extra.raw_info
expect(user.twitter_created_at.to_i).to eq(Time.zone.parse(raw_info.created_at).to_i)
expect do
described_class.call(auth_payload)
end.to change { user.reload.twitter_username }.to eq("newnick")
end
it "sets remember_me for the existing user" do