From: Andy Allan Date: Thu, 18 Jan 2024 10:47:17 +0000 (+0000) Subject: Merge pull request #4218 from AntonKhorev/no-user-id-renames X-Git-Tag: live~816 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/7406ae5dcc9cbb177a8ea33085af9caf6e3ebb1b?hp=-c Merge pull request #4218 from AntonKhorev/no-user-id-renames Disallow username changes to user_n if n isn't their id --- 7406ae5dcc9cbb177a8ea33085af9caf6e3ebb1b diff --combined .rubocop_todo.yml index 8a196f52e,d80653712..9874aa379 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@@ -14,11 -14,6 +14,11 @@@ require - rubocop-rails - rubocop-rake +# Offense count: 11 +# Configuration parameters: Include, MaxAmount +FactoryBot/ExcessiveCreateList: + MaxAmount: 200 + # Offense count: 557 # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns. @@@ -66,12 -61,12 +66,12 @@@ Metrics/BlockNesting # Offense count: 26 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 299 + Max: 305 # Offense count: 59 # Configuration parameters: AllowedMethods, AllowedPatterns. Metrics/CyclomaticComplexity: - Max: 26 + Max: 29 # Offense count: 753 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. @@@ -86,7 -81,7 +86,7 @@@ Metrics/ParameterLists # Offense count: 56 # Configuration parameters: AllowedMethods, AllowedPatterns. Metrics/PerceivedComplexity: - Max: 29 + Max: 30 # Offense count: 2394 # This cop supports safe autocorrection (--autocorrect). diff --combined app/models/user.rb index b04f8e2b9,d0993f7ee..7ca7b7a3b --- a/app/models/user.rb +++ b/app/models/user.rb @@@ -34,13 -34,12 +34,13 @@@ # # Indexes # -# users_auth_idx (auth_provider,auth_uid) UNIQUE -# users_display_name_idx (display_name) UNIQUE -# users_display_name_lower_idx (lower((display_name)::text)) -# users_email_idx (email) UNIQUE -# users_email_lower_idx (lower((email)::text)) -# users_home_idx (home_tile) +# users_auth_idx (auth_provider,auth_uid) UNIQUE +# users_display_name_canonical_idx (lower(NORMALIZE(display_name, NFKC))) +# users_display_name_idx (display_name) UNIQUE +# users_display_name_lower_idx (lower((display_name)::text)) +# users_email_idx (email) UNIQUE +# users_email_lower_idx (lower((email)::text)) +# users_home_idx (home_tile) # class User < ApplicationRecord @@@ -96,10 -95,11 +96,11 @@@ validates :display_name, :presence => true, :length => 3..255, :exclusion => %w[new terms save confirm confirm-email go_public reset-password forgot-password suspended] validates :display_name, :if => proc { |u| u.display_name_changed? }, - :uniqueness => { :case_sensitive => false } + :normalized_uniqueness => { :case_sensitive => false } validates :display_name, :if => proc { |u| u.display_name_changed? }, :characters => { :url_safe => true }, :whitespace => { :leading => false, :trailing => false } + validate :display_name_cannot_be_user_id_with_other_id, :if => proc { |u| u.display_name_changed? } validates :email, :presence => true, :confirmation => true, :characters => true validates :email, :if => proc { |u| u.email_changed? }, :uniqueness => { :case_sensitive => false } @@@ -120,10 -120,17 +121,16 @@@ alias_attribute :created_at, :creation_time - after_initialize :encrypt_password before_save :encrypt_password before_save :update_tile after_save :spam_check + def display_name_cannot_be_user_id_with_other_id + display_name&.match(/^user_(\d+)$/i) do |m| + errors.add :display_name, I18n.t("activerecord.errors.messages.display_name_is_user_n") unless m[1].to_i == id + end + end + def to_param display_name end @@@ -133,7 -140,7 +140,7 @@@ user = find_by("email = ? OR display_name = ?", options[:username].strip, options[:username]) if user.nil? - users = where("LOWER(email) = LOWER(?) OR LOWER(display_name) = LOWER(?)", options[:username].strip, options[:username]) + users = where("LOWER(email) = LOWER(?) OR LOWER(NORMALIZE(display_name, NFKC)) = LOWER(NORMALIZE(?, NFKC))", options[:username].strip, options[:username]) user = users.first if users.count == 1 end diff --combined config/locales/en.yml index 653e363a6,b266a87d6..ca22de9bd --- a/config/locales/en.yml +++ b/config/locales/en.yml @@@ -40,6 -40,7 +40,7 @@@ en messages: invalid_email_address: does not appear to be a valid e-mail address email_address_not_routable: is not routable + display_name_is_user_n: can't be user_n unless n is your user id models: user_mute: attributes: @@@ -1439,9 -1440,9 +1440,9 @@@ one: "%{count} report" other: "%{count} reports" no_reports: No reports - report_created_at: "First reported at %{datetime}" - last_resolved_at: "Last resolved at %{datetime}" - last_updated_at: "Last updated at %{datetime} by %{displayname}" + report_created_at_html: "First reported at %{datetime}" + last_resolved_at_html: "Last resolved at %{datetime}" + last_updated_at_html: "Last updated at %{datetime} by %{displayname}" resolve: Resolve ignore: Ignore reopen: Reopen @@@ -1590,7 -1591,6 +1591,7 @@@ loaded: one: "loaded successfully with %{trace_points} out of a possible %{count} point." other: "loaded successfully with %{trace_points} out of a possible %{count} points." + all_your_traces_html: "All your successfully uploaded GPX traces can be found at %{url}." subject: "[OpenStreetMap] GPX Import success" signup_confirm: subject: "[OpenStreetMap] Welcome to OpenStreetMap" @@@ -1871,7 -1871,6 +1872,7 @@@ image: Image alt: Alt text url: URL + codeblock: Code block richtext_field: edit: Edit preview: Preview @@@ -2307,36 -2306,41 +2308,36 @@@ cycleway_national: "National cycleway" cycleway_regional: "Regional cycleway" cycleway_local: "Local cycleway" + cycleway_mtb: "Mountain bike route" footway: "Footway" rail: "Railway" train: "Train" subway: "Subway" ferry: "Ferry" light_rail: "Light rail" - tram_only: "Tram" + tram: "Tram" trolleybus: "Trolleybus" bus: "Bus" - cable: - - Cable car - - chair lift - runway: - - Airport Runway - - taxiway - apron_only: "Airport apron" + cable_car: "Cable car" + chair_lift: "Chair lift" + runway: "Airport Runway" + taxiway: "Taxiway" + apron: "Airport apron" admin: "Administrative boundary" - orchard: - - Orchard - - vineyard - forest: - - Forest - - wood + capital: "Capital" + city: "City" + orchard: "Orchard" + vineyard: "Vineyard" + forest: "Forest" + wood: "Wood" farmland: "Farmland" - grass: - - Grass - - meadow + grass: "Grass" + meadow: "Meadow" bare_rock: "Bare rock" sand: "Sand" golf: "Golf course" park: "Park" - common: - - Common - - meadow - - garden + common: "Common" built_up: "Built-up area" resident: "Residential area" retail: "Retail area" @@@ -2344,8 -2348,9 +2345,8 @@@ commercial: "Commercial area" heathland: "Heathland" scrubland: "Scrubland" - lake: - - Lake - - reservoir + lake: "Lake" + reservoir: "Reservoir" intermittent_water: "Intermittent waterbody" glacier: "Glacier" reef: "Reef" @@@ -2356,16 -2361,17 +2357,16 @@@ allotments: "Allotments" pitch: "Sports pitch" centre: "Sports centre" + beach: "Beach" reserve: "Nature reserve" military: "Military area" - school: - - School - - university - - hospital + school: "School" + university: "University" + hospital: "Hospital" building: "Significant building" station: "Railway station" - summit: - - Summit - - peak + summit: "Summit" + peak: "Peak" tunnel: "Dashed casing = tunnel" bridge: "Black casing = bridge" private: "Private access" @@@ -2374,9 -2380,7 +2375,9 @@@ bus_stop: "Bus stop" stop: "Stop" bicycle_shop: "Bicycle shop" + bicycle_rental: "Bicycle rental" bicycle_parking: "Bicycle parking" + bicycle_parking_small: "Small bicycle parking" toilets: "Toilets" welcome: title: Welcome! @@@ -2606,7 -2610,6 +2607,7 @@@ read_gpx: Read private GPS traces write_gpx: Upload GPS traces write_notes: Modify notes + write_redactions: Redact map data read_email: Read user email address skip_authorization: Auto approve application oauth_clients: @@@ -2691,7 -2694,6 +2692,7 @@@ application: "Application" permissions: "Permissions" no_applications_html: "You have not yet authorized any %{oauth2} applications." + oauth_2: "OAuth 2" application: revoke: "Revoke Access" confirm_revoke: "Revoke access for this application?" @@@ -2794,7 -2796,6 +2795,7 @@@ importer: "Revoke importer access" block_history: "Active Blocks" moderator_history: "Blocks Given" + revoke_all_blocks: "Revoke all blocks" comments: "Comments" create_block: "Block this User" activate_user: "Activate this User" @@@ -2897,16 -2898,6 +2898,16 @@@ confirm: "Are you sure you wish to revoke this block?" revoke: "Revoke!" flash: "This block has been revoked." + revoke_all: + title: "Revoking all blocks on %{block_on}" + heading_html: "Revoking all blocks on %{block_on}" + empty: "%{name} has no active blocks." + confirm: "Are you sure you wish to revoke %{active_blocks}?" + active_blocks: + one: "%{count} active block" + other: "%{count} active blocks" + revoke: "Revoke!" + flash: "All active blocks have been revoked." helper: time_future_html: "Ends in %{time}." until_login: "Active until the user logs in." @@@ -3023,7 -3014,6 +3024,7 @@@ reactivate: Reactivate comment_and_resolve: Comment & Resolve comment: Comment + log_in_to_comment: "Log in to comment on this note" report_link_html: "If this note contains sensitive information that needs to be removed, you can %{link}." other_problems_resolve: "For all other problems with the note, please resolve it yourself with a comment." other_problems_resolved: "For all other problems, resolving is sufficient." diff --combined test/models/user_test.rb index d21512f2a,37f66e178..42949504f --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@@ -10,7 -10,7 +10,7 @@@ class UserTest < ActiveSupport::TestCas :home_lat => nil, :home_lon => nil, :home_zoom => nil) - assert_not user.valid? + assert_not_predicate user, :valid? assert_predicate user.errors[:email], :any? assert_predicate user.errors[:pass_crypt], :any? assert_predicate user.errors[:display_name], :any? @@@ -27,13 -27,10 +27,13 @@@ end def test_unique_display_name - existing_user = create(:user) - new_user = build(:user, :display_name => existing_user.display_name) - assert_not new_user.save - assert_includes new_user.errors[:display_name], "has already been taken" + create(:user, :display_name => "H\u{e9}nryIV") + + %W[H\u{e9}nryIV he\u{301}nryiv H\u{c9}nry\u2163 he\u{301}nry\u2173].each do |name| + new_user = build(:user, :display_name => name) + assert_not new_user.save + assert_includes new_user.errors[:display_name], "has already been taken" + end end def test_email_valid @@@ -60,11 -57,11 +60,11 @@@ user.display_name = "123" assert_predicate user, :valid?, "should allow 3 char name name" user.display_name = "12" - assert_not user.valid?, "should not allow 2 char name" + assert_not_predicate user, :valid?, "should not allow 2 char name" user.display_name = "" - assert_not user.valid?, "should not allow blank/0 char name" + assert_not_predicate user, :valid?, "should not allow blank/0 char name" user.display_name = nil - assert_not user.valid?, "should not allow nil value" + assert_not_predicate user, :valid?, "should not allow nil value" end def test_display_name_valid @@@ -90,10 -87,40 +90,40 @@@ bad.each do |display_name| user = build(:user) user.display_name = display_name - assert_not user.valid?, "#{display_name} is valid when it shouldn't be" + assert_not_predicate user, :valid?, "#{display_name} is valid when it shouldn't be" end end + def test_display_name_user_id_new + existing_user = create(:user) + user = build(:user) + + user.display_name = "user_#{existing_user.id}" + assert_not user.valid?, "user_ name is valid for existing user id when it shouldn't be" + + user.display_name = "user_#{existing_user.id + 1}" + assert_not user.valid?, "user_ name is valid for new user id when it shouldn't be" + end + + def test_display_name_user_id_rename + existing_user = create(:user) + user = create(:user) + + user.display_name = "user_#{existing_user.id}" + assert_not user.valid?, "user_ name is valid for existing user id when it shouldn't be" + + user.display_name = "user_#{user.id}" + assert_predicate user, :valid?, "user_ name is invalid for own id, when it should be" + end + + def test_display_name_user_id_unchanged_is_valid + user = build(:user, :display_name => "user_0") + user.save(:validate => false) + user.reload + + assert_predicate user, :valid?, "user_0 display_name is invalid but it hasn't been changed" + end + def test_friends_with alice = create(:user, :active) bob = create(:user, :active) @@@ -220,25 -247,25 +250,25 @@@ assert_predicate build(:user, :pending), :visible? assert_predicate build(:user, :active), :visible? assert_predicate build(:user, :confirmed), :visible? - assert_not build(:user, :suspended).visible? - assert_not build(:user, :deleted).visible? + assert_not_predicate build(:user, :suspended), :visible? + assert_not_predicate build(:user, :deleted), :visible? end def test_active? - assert_not build(:user, :pending).active? + assert_not_predicate build(:user, :pending), :active? assert_predicate build(:user, :active), :active? assert_predicate build(:user, :confirmed), :active? - assert_not build(:user, :suspended).active? - assert_not build(:user, :deleted).active? + assert_not_predicate build(:user, :suspended), :active? + assert_not_predicate build(:user, :deleted), :active? end def test_moderator? - assert_not create(:user).moderator? + assert_not_predicate create(:user), :moderator? assert_predicate create(:moderator_user), :moderator? end def test_administrator? - assert_not create(:user).administrator? + assert_not_predicate create(:user), :administrator? assert_predicate create(:administrator_user), :administrator? end @@@ -256,10 -283,10 +286,10 @@@ assert_predicate user.description, :blank? assert_nil user.home_lat assert_nil user.home_lon - assert_not user.avatar.attached? + assert_not_predicate user.avatar, :attached? assert_equal "deleted", user.status - assert_not user.visible? - assert_not user.active? + assert_not_predicate user, :visible? + assert_not_predicate user, :active? end def test_soft_destroy_revokes_oauth1_tokens