From 18b5d3c605196ff65e06dc02a3713ede1ac7b5bb Mon Sep 17 00:00:00 2001
From: Rub21 <rub2106@gmail.com>
Date: Wed, 15 Jul 2026 10:09:09 -0500
Subject: [PATCH] Limit new trace uploads to trackable and identifiable

---
 .rubocop_todo.yml                             |  2 +-
 app/controllers/api/traces_controller.rb      |  8 --
 app/controllers/traces_controller.rb          | 14 +---
 app/helpers/trace_helper.rb                   |  6 ++
 app/models/trace.rb                           | 35 ++++++++-
 app/models/user.rb                            |  9 +++
 app/views/traces/edit.html.erb                |  6 +-
 app/views/traces/new.html.erb                 |  6 +-
 config/locales/en.yml                         |  4 +
 .../api/tracepoints_controller_test.rb        |  4 +-
 .../api/traces/data_controller_test.rb        |  6 +-
 .../controllers/api/traces_controller_test.rb | 77 ++++++++++---------
 test/controllers/site_controller_test.rb      |  2 +-
 .../traces/data_controller_test.rb            |  8 +-
 .../traces/feeds_controller_test.rb           | 14 ++--
 .../traces/icons_controller_test.rb           |  6 +-
 .../traces/pictures_controller_test.rb        |  6 +-
 test/controllers/traces_controller_test.rb    | 66 +++++++++++-----
 test/factories/traces.rb                      |  6 ++
 test/helpers/trace_helper_test.rb             | 25 ++++++
 test/models/trace_test.rb                     | 52 ++++++++++---
 test/models/user_test.rb                      | 17 ++++
 22 files changed, 257 insertions(+), 122 deletions(-)
 create mode 100644 test/helpers/trace_helper_test.rb

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 892e927c0..3c4f8c675 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -56,7 +56,7 @@ Metrics/BlockNesting:
 # Offense count: 23
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 343
+  Max: 351
 
 # Offense count: 72
 # Configuration parameters: AllowedMethods, AllowedPatterns.
diff --git a/app/controllers/api/traces_controller.rb b/app/controllers/api/traces_controller.rb
index 757984cfb..a0892ed1c 100644
--- a/app/controllers/api/traces_controller.rb
+++ b/app/controllers/api/traces_controller.rb
@@ -28,14 +28,6 @@ module Api
       description = params[:description] || ""
       visibility = params[:visibility]
 
-      if visibility.nil?
-        visibility = if params.fetch(:public, "0").to_i.nonzero?
-                       "public"
-                     else
-                       "private"
-                     end
-      end
-
       if params.expect(:file).respond_to?(:read)
         trace = do_create(params[:file], tags, description, visibility)
 
diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb
index 140e61b56..2e180d20d 100644
--- a/app/controllers/traces_controller.rb
+++ b/app/controllers/traces_controller.rb
@@ -85,7 +85,7 @@ class TracesController < ApplicationController
 
   def new
     @title = t ".upload_trace"
-    @trace = Trace.new(:visibility => default_visibility)
+    @trace = Trace.new(:visibility => current_user.default_trace_visibility)
   end
 
   def edit
@@ -210,18 +210,6 @@ class TracesController < ApplicationController
     render :action => :offline if Settings.status == "gpx_offline"
   end
 
-  def default_visibility
-    visibility = current_user.preferences.find_by(:k => "gps.trace.visibility")
-
-    if visibility
-      visibility.v
-    elsif current_user.preferences.find_by(:k => "gps.trace.public", :v => "default").nil?
-      "private"
-    else
-      "public"
-    end
-  end
-
   def trace_params
     params.expect(:trace => [:description, :tagstring, :visibility])
   end
diff --git a/app/helpers/trace_helper.rb b/app/helpers/trace_helper.rb
index e1902c331..2b68b7034 100644
--- a/app/helpers/trace_helper.rb
+++ b/app/helpers/trace_helper.rb
@@ -18,4 +18,10 @@ module TraceHelper
   def trace_picture(trace, options = {})
     trace_image(trace, :animated => true, :size => 250, **options)
   end
+
+  def trace_visibility_options(trace)
+    trace.selectable_visibilities.map do |visibility|
+      [t("traces.visibility.#{visibility}"), visibility]
+    end
+  end
 end
diff --git a/app/models/trace.rb b/app/models/trace.rb
index b8071f960..216eb3735 100644
--- a/app/models/trace.rb
+++ b/app/models/trace.rb
@@ -44,19 +44,44 @@ class Trace < ApplicationRecord
   has_one_attached :image, :service => Settings.trace_image_storage
   has_one_attached :icon, :service => Settings.trace_icon_storage
 
+  # Visibility values for new uploads, and the old ones kept for existing traces.
+  VISIBILITIES = %w[trackable identifiable].freeze
+  LEGACY_VISIBILITIES = %w[private public].freeze
+
   validates :user, :associated => true
   validates :name, :presence => true, :length => 1..255, :characters => true
   validates :description, :presence => { :on => :create }, :length => 1..255, :characters => true
   validates :timestamp, :presence => true
-  validates :visibility, :inclusion => %w[private public trackable identifiable]
+  validates :visibility, :inclusion => VISIBILITIES + LEGACY_VISIBILITIES
+  validates :visibility, :inclusion => { :in => VISIBILITIES }, :on => :create
+  validate :visibility_not_changed_to_legacy, :on => :update
 
   after_save :set_filename
 
+  # True if a new upload can use this visibility.
+  def self.valid_visibility?(visibility)
+    VISIBILITIES.include?(visibility)
+  end
+
+  # True if this is an old visibility (private or public) that new uploads no longer use.
+  def self.legacy_visibility?(visibility)
+    LEGACY_VISIBILITIES.include?(visibility)
+  end
+
   # Visibility for new uploads when the user has no preference.
   def self.default_visibility
     "trackable"
   end
 
+  # Visibilities this trace can use, including its legacy one if it has one.
+  def selectable_visibilities
+    if Trace.legacy_visibility?(visibility)
+      [visibility] + VISIBILITIES
+    else
+      VISIBILITIES
+    end
+  end
+
   def tagstring
     tags.collect(&:tag).join(", ")
   end
@@ -303,4 +328,12 @@ class Trace < ApplicationRecord
   def set_filename
     file.blob.update(:filename => "#{id}#{extension_name}") if file.attached?
   end
+
+  # Prevent an existing trace from changing its visibility from a current value back to a legacy one.
+  def visibility_not_changed_to_legacy
+    return unless visibility_changed?
+    return unless Trace.legacy_visibility?(visibility) && Trace.valid_visibility?(visibility_was)
+
+    errors.add(:visibility, :cannot_change_to_legacy)
+  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 15f7f9fa6..74323bd48 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -296,6 +296,15 @@ class User < ApplicationRecord
     preference.update!(:v => language)
   end
 
+  def default_trace_visibility
+    visibility = preferences.find_by(:k => "gps.trace.visibility")&.v
+    if Trace.valid_visibility?(visibility)
+      visibility
+    else
+      Trace.default_visibility
+    end
+  end
+
   def notification_preferences
     @notification_preferences ||= UserNotificationPreferences.new(self)
   end
diff --git a/app/views/traces/edit.html.erb b/app/views/traces/edit.html.erb
index 1c440adf2..074e97ca6 100644
--- a/app/views/traces/edit.html.erb
+++ b/app/views/traces/edit.html.erb
@@ -8,11 +8,7 @@
   <%= f.text_field :name, :disabled => true %>
   <%= f.text_field :description, :maxlength => 255 %>
   <%= f.text_field :tagstring %>
-  <%= f.select :visibility,
-               [[t("traces.visibility.private"), "private"],
-                [t("traces.visibility.public"), "public"],
-                [t("traces.visibility.trackable"), "trackable"],
-                [t("traces.visibility.identifiable"), "identifiable"]],
+  <%= f.select :visibility, trace_visibility_options(@trace),
                :help => link_to(t(".visibility_help"), t(".visibility_help_url")) %>
   <%= f.primary %>
   <%= link_to t(".cancel"), show_trace_path(@trace.user, @trace), :class => "btn btn-link" %>
diff --git a/app/views/traces/new.html.erb b/app/views/traces/new.html.erb
index ae5ffe565..9af986354 100644
--- a/app/views/traces/new.html.erb
+++ b/app/views/traces/new.html.erb
@@ -7,11 +7,7 @@
                               :help => ".gpx, .tar.gz, .tar.bz2, .tar, .zip, .gpx.gz, .gpx.bz2" %>
   <%= f.text_field :description, :maxlength => 255 %>
   <%= f.text_field :tagstring %>
-  <%= f.select :visibility,
-               [[t("traces.visibility.private"), "private"],
-                [t("traces.visibility.public"), "public"],
-                [t("traces.visibility.trackable"), "trackable"],
-                [t("traces.visibility.identifiable"), "identifiable"]],
+  <%= f.select :visibility, trace_visibility_options(@trace),
                :help => link_to(t(".visibility_help"), t(".visibility_help_url")) %>
   <%= f.primary %>
   <%= link_to t(".help"), t(".help_url"), :class => "btn btn-link" %>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 6bdb37549..1d838e341 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -49,6 +49,10 @@ en:
           attributes:
             url:
               http_parse_error: Url should start with http:// or https://
+        trace:
+          attributes:
+            visibility:
+              cannot_change_to_legacy: "can't be changed back to a legacy value"
     # Translates all the model names, which is used in error handling on the website
     models:
       acl: "Access Control List"
diff --git a/test/controllers/api/tracepoints_controller_test.rb b/test/controllers/api/tracepoints_controller_test.rb
index fc4d01588..e1235eb20 100644
--- a/test/controllers/api/tracepoints_controller_test.rb
+++ b/test/controllers/api/tracepoints_controller_test.rb
@@ -27,8 +27,8 @@ module Api
       )
     end
 
-    def test_tracepoints
-      point = create(:trace, :visibility => "public", :latitude => 1, :longitude => 1) do |trace|
+    def test_tracepoints_public
+      point = create(:trace, :without_validations, :visibility => "public", :latitude => 1, :longitude => 1) do |trace|
         create(:tracepoint, :trace => trace, :latitude => 1 * GeoRecord::SCALE, :longitude => 1 * GeoRecord::SCALE)
       end
       minlon = point.longitude - 0.001
diff --git a/test/controllers/api/traces/data_controller_test.rb b/test/controllers/api/traces/data_controller_test.rb
index 155b934a1..d7076ab33 100644
--- a/test/controllers/api/traces/data_controller_test.rb
+++ b/test/controllers/api/traces/data_controller_test.rb
@@ -20,7 +20,7 @@ module Api
 
       # Test downloading a trace through the api
       def test_show
-        public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+        public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
 
         # First with no auth
         get api_trace_data_path(public_trace_file)
@@ -65,7 +65,7 @@ module Api
 
       # Check an anonymous trace can't be downloaded by another user through the api
       def test_data_anon
-        anon_trace_file = create(:trace, :visibility => "private", :fixture => "b")
+        anon_trace_file = create(:trace, :visibility => "trackable", :fixture => "b")
 
         # First with no auth
         get api_trace_data_path(anon_trace_file)
@@ -105,7 +105,7 @@ module Api
 
       # Check that trace data can't be downloaded through the api when the traces feature is disabled
       def test_show_disabled
-        public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+        public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
         auth_header = bearer_authorization_header public_trace_file.user
 
         with_settings(:traces_disabled => true) do
diff --git a/test/controllers/api/traces_controller_test.rb b/test/controllers/api/traces_controller_test.rb
index 59a6f976e..71ce695f6 100644
--- a/test/controllers/api/traces_controller_test.rb
+++ b/test/controllers/api/traces_controller_test.rb
@@ -39,7 +39,7 @@ module Api
 
     # Check getting a specific trace through the api
     def test_show
-      public_trace_file = create(:trace, :visibility => "public")
+      public_trace_file = create(:trace, :visibility => "identifiable")
 
       # First with no auth
       get api_trace_path(public_trace_file)
@@ -69,7 +69,7 @@ module Api
 
     # Check an anonymous trace can't be specifically fetched by another user
     def test_show_anon
-      anon_trace_file = create(:trace, :visibility => "private")
+      anon_trace_file = create(:trace, :visibility => "trackable")
 
       # First with no auth
       get api_trace_path(anon_trace_file)
@@ -107,7 +107,7 @@ module Api
 
     # Check that getting a trace through the api is not possible when the traces feature is disabled
     def test_show_disabled
-      public_trace_file = create(:trace, :visibility => "public")
+      public_trace_file = create(:trace, :visibility => "identifiable")
       auth_header = bearer_authorization_header public_trace_file.user
 
       with_settings(:traces_disabled => true) do
@@ -163,50 +163,53 @@ module Api
 
       trace.destroy
       assert_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
+    end
+
+    # The legacy public flag is no longer supported and returns bad request
+    def test_create_legacy_public_flag
+      fixture = Rails.root.join("test/gpx/fixtures/a.gpx")
+      file = Rack::Test::UploadedFile.new(fixture, "application/gpx+xml")
+      user = create(:user)
+      auth_header = bearer_authorization_header user
+
+      assert_no_difference "Trace.count" do
+        post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 1 }, :headers => auth_header
+      end
+      assert_response :bad_request
 
-      # Rewind the file
       file.rewind
 
-      # Now authenticated, with the legacy public flag
-      assert_not_equal "public", user.preferences.find_by(:k => "gps.trace.visibility").v
+      assert_no_difference "Trace.count" do
+        post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 0 }, :headers => auth_header
+      end
+      assert_response :bad_request
+    end
+
+    # A legacy visibility (public or private) is no longer accepted on upload
+    def test_create_legacy_visibility
+      fixture = Rails.root.join("test/gpx/fixtures/a.gpx")
+      file = Rack::Test::UploadedFile.new(fixture, "application/gpx+xml")
+      user = create(:user)
       auth_header = bearer_authorization_header user
-      post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 1 }, :headers => auth_header
-      assert_response :success
-      trace = Trace.find(response.body.to_i)
-      assert_equal "a.gpx", trace.name
-      assert_equal "New Trace", trace.description
-      assert_equal %w[new trace], trace.tags.order(:tag).collect(&:tag)
-      assert_equal "public", trace.visibility
-      assert_not trace.inserted
-      assert_equal File.new(fixture).read, trace.file.blob.download
-      trace.destroy
-      assert_equal "public", user.preferences.find_by(:k => "gps.trace.visibility").v
 
-      # Rewind the file
+      assert_no_difference "Trace.count" do
+        post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :visibility => "public" }, :headers => auth_header
+      end
+      assert_response :bad_request
+
       file.rewind
 
-      # Now authenticated, with the legacy private flag
-      second_user = create(:user)
-      assert_nil second_user.preferences.find_by(:k => "gps.trace.visibility")
-      auth_header = bearer_authorization_header second_user
-      post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 0 }, :headers => auth_header
-      assert_response :success
-      trace = Trace.find(response.body.to_i)
-      assert_equal "a.gpx", trace.name
-      assert_equal "New Trace", trace.description
-      assert_equal %w[new trace], trace.tags.order(:tag).collect(&:tag)
-      assert_equal "private", trace.visibility
-      assert_not trace.inserted
-      assert_equal File.new(fixture).read, trace.file.blob.download
-      trace.destroy
-      assert_equal "private", second_user.preferences.find_by(:k => "gps.trace.visibility").v
+      assert_no_difference "Trace.count" do
+        post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :visibility => "private" }, :headers => auth_header
+      end
+      assert_response :bad_request
     end
 
     # Check updating a trace through the api
     def test_update
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
       deleted_trace_file = create(:trace, :deleted)
-      anon_trace_file = create(:trace, :visibility => "private")
+      anon_trace_file = create(:trace, :visibility => "trackable")
 
       # First with no auth
       put api_trace_path(public_trace_file), :params => create_trace_xml(public_trace_file)
@@ -237,7 +240,7 @@ module Api
       auth_header = bearer_authorization_header public_trace_file.user
       t = public_trace_file
       t.description = "Changed description"
-      t.visibility = "private"
+      t.visibility = "trackable"
       put api_trace_path(t), :params => create_trace_xml(t), :headers => auth_header
       assert_response :success
       nt = Trace.find(t.id)
@@ -263,7 +266,7 @@ module Api
 
     # Check deleting a trace through the api
     def test_destroy
-      public_trace_file = create(:trace, :visibility => "public")
+      public_trace_file = create(:trace, :visibility => "identifiable")
 
       # First with no auth
       delete api_trace_path(public_trace_file)
diff --git a/test/controllers/site_controller_test.rb b/test/controllers/site_controller_test.rb
index 7a3812d5e..9fe319b3c 100644
--- a/test/controllers/site_controller_test.rb
+++ b/test/controllers/site_controller_test.rb
@@ -360,7 +360,7 @@ class SiteControllerTest < ActionDispatch::IntegrationTest
   def test_edit_with_inaccessible_gpxes
     user = create(:user)
     deleted_gpx = create(:trace, :deleted, :latitude => 1, :longitude => 1)
-    private_gpx = create(:trace, :latitude => 1, :longitude => 1, :visibility => "private")
+    private_gpx = create(:trace, :latitude => 1, :longitude => 1, :visibility => "trackable")
     session_for(user)
 
     get edit_path(:gpx => 99999)
diff --git a/test/controllers/traces/data_controller_test.rb b/test/controllers/traces/data_controller_test.rb
index a28579c06..1be591499 100644
--- a/test/controllers/traces/data_controller_test.rb
+++ b/test/controllers/traces/data_controller_test.rb
@@ -25,7 +25,7 @@ module Traces
 
     # Test downloading a trace
     def test_show
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
 
       # First with no auth, which should work since the trace is public
       get trace_data_path(public_trace_file)
@@ -69,7 +69,7 @@ module Traces
 
     # Check an anonymous trace can't be downloaded by another user
     def test_show_anon
-      anon_trace_file = create(:trace, :visibility => "private", :fixture => "b")
+      anon_trace_file = create(:trace, :visibility => "trackable", :fixture => "b")
 
       # First with no auth
       get trace_data_path(anon_trace_file)
@@ -103,7 +103,7 @@ module Traces
     end
 
     def test_show_offline
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
       with_settings(:status => "gpx_offline") do
         get trace_data_path(public_trace_file)
         assert_response :success
@@ -113,7 +113,7 @@ module Traces
 
     # Check that trace data can't be downloaded when the traces feature is disabled
     def test_show_disabled
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
 
       with_settings(:traces_disabled => true) do
         get trace_data_path(public_trace_file)
diff --git a/test/controllers/traces/feeds_controller_test.rb b/test/controllers/traces/feeds_controller_test.rb
index 3e7bc4a12..405482f98 100644
--- a/test/controllers/traces/feeds_controller_test.rb
+++ b/test/controllers/traces/feeds_controller_test.rb
@@ -28,16 +28,16 @@ module Traces
     def test_show
       user = create(:user)
       # The fourth test below is surprisingly sensitive to timestamp ordering when the timestamps are equal.
-      trace_a = create(:trace, :visibility => "public", :timestamp => 4.seconds.ago) do |trace|
+      trace_a = create(:trace, :visibility => "identifiable", :timestamp => 4.seconds.ago) do |trace|
         create(:tracetag, :trace => trace, :tag => "London")
       end
-      trace_b = create(:trace, :visibility => "public", :timestamp => 3.seconds.ago) do |trace|
+      trace_b = create(:trace, :visibility => "identifiable", :timestamp => 3.seconds.ago) do |trace|
         create(:tracetag, :trace => trace, :tag => "Birmingham")
       end
-      create(:trace, :visibility => "private", :user => user, :timestamp => 2.seconds.ago) do |trace|
+      create(:trace, :visibility => "trackable", :user => user, :timestamp => 2.seconds.ago) do |trace|
         create(:tracetag, :trace => trace, :tag => "London")
       end
-      create(:trace, :visibility => "private", :user => user, :timestamp => 1.second.ago) do |trace|
+      create(:trace, :visibility => "trackable", :user => user, :timestamp => 1.second.ago) do |trace|
         create(:tracetag, :trace => trace, :tag => "Birmingham")
       end
 
@@ -55,11 +55,11 @@ module Traces
       second_user = create(:user)
       create(:user)
       create(:trace)
-      trace_b = create(:trace, :visibility => "public", :timestamp => 4.seconds.ago, :user => user)
-      trace_c = create(:trace, :visibility => "public", :timestamp => 3.seconds.ago, :user => user) do |trace|
+      trace_b = create(:trace, :visibility => "identifiable", :timestamp => 4.seconds.ago, :user => user)
+      trace_c = create(:trace, :visibility => "identifiable", :timestamp => 3.seconds.ago, :user => user) do |trace|
         create(:tracetag, :trace => trace, :tag => "London")
       end
-      create(:trace, :visibility => "private")
+      create(:trace, :visibility => "trackable")
 
       # Test a user with no traces
       get traces_feed_path(:display_name => second_user)
diff --git a/test/controllers/traces/icons_controller_test.rb b/test/controllers/traces/icons_controller_test.rb
index 53865ae29..8de51f83a 100644
--- a/test/controllers/traces/icons_controller_test.rb
+++ b/test/controllers/traces/icons_controller_test.rb
@@ -15,7 +15,7 @@ module Traces
 
     # Test downloading the icon for a trace
     def test_show
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
 
       # First with no auth, which should work since the trace is public
       get trace_icon_path(public_trace_file.user, public_trace_file)
@@ -34,7 +34,7 @@ module Traces
 
     # Check the icon for an anonymous trace can't be downloaded by another user
     def test_show_anon
-      anon_trace_file = create(:trace, :visibility => "private", :fixture => "b")
+      anon_trace_file = create(:trace, :visibility => "trackable", :fixture => "b")
 
       # First with no auth
       get trace_icon_path(anon_trace_file.user, anon_trace_file)
@@ -67,7 +67,7 @@ module Traces
 
     # Check that trace icons can't be downloaded when the traces feature is disabled
     def test_show_disabled
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
 
       with_settings(:traces_disabled => true) do
         get trace_icon_path(public_trace_file.user, public_trace_file)
diff --git a/test/controllers/traces/pictures_controller_test.rb b/test/controllers/traces/pictures_controller_test.rb
index dc3f60b7d..979fc6ff7 100644
--- a/test/controllers/traces/pictures_controller_test.rb
+++ b/test/controllers/traces/pictures_controller_test.rb
@@ -15,7 +15,7 @@ module Traces
 
     # Test downloading the picture for a trace
     def test_show
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
 
       # First with no auth, which should work since the trace is public
       get trace_picture_path(public_trace_file.user, public_trace_file)
@@ -34,7 +34,7 @@ module Traces
 
     # Check the picture for an anonymous trace can't be downloaded by another user
     def test_show_anon
-      anon_trace_file = create(:trace, :visibility => "private", :fixture => "b")
+      anon_trace_file = create(:trace, :visibility => "trackable", :fixture => "b")
 
       # First with no auth
       get trace_picture_path(anon_trace_file.user, anon_trace_file)
@@ -67,7 +67,7 @@ module Traces
 
     # Check that trace pictures can't be downloaded when the traces feature is disabled
     def test_show_disabled
-      public_trace_file = create(:trace, :visibility => "public", :fixture => "a")
+      public_trace_file = create(:trace, :visibility => "identifiable", :fixture => "a")
 
       with_settings(:traces_disabled => true) do
         get trace_picture_path(public_trace_file.user, public_trace_file)
diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb
index 6674a216f..0ba8a0acf 100644
--- a/test/controllers/traces_controller_test.rb
+++ b/test/controllers/traces_controller_test.rb
@@ -81,16 +81,16 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
   def test_index
     user = create(:user)
     # The fourth test below is surprisingly sensitive to timestamp ordering when the timestamps are equal.
-    trace_a = create(:trace, :visibility => "public", :timestamp => 4.seconds.ago) do |trace|
+    trace_a = create(:trace, :visibility => "identifiable", :timestamp => 4.seconds.ago) do |trace|
       create(:tracetag, :trace => trace, :tag => "London")
     end
-    trace_b = create(:trace, :visibility => "public", :timestamp => 3.seconds.ago) do |trace|
+    trace_b = create(:trace, :visibility => "identifiable", :timestamp => 3.seconds.ago) do |trace|
       create(:tracetag, :trace => trace, :tag => "Birmingham")
     end
-    trace_c = create(:trace, :visibility => "private", :user => user, :timestamp => 2.seconds.ago) do |trace|
+    trace_c = create(:trace, :visibility => "trackable", :user => user, :timestamp => 2.seconds.ago) do |trace|
       create(:tracetag, :trace => trace, :tag => "London")
     end
-    trace_d = create(:trace, :visibility => "private", :user => user, :timestamp => 1.second.ago) do |trace|
+    trace_d = create(:trace, :visibility => "trackable", :user => user, :timestamp => 1.second.ago) do |trace|
       create(:tracetag, :trace => trace, :tag => "Birmingham")
     end
 
@@ -116,10 +116,10 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
   # Check that I can get mine
   def test_index_mine
     user = create(:user)
-    create(:trace, :visibility => "public") do |trace|
+    create(:trace, :visibility => "identifiable") do |trace|
       create(:tracetag, :trace => trace, :tag => "Birmingham")
     end
-    trace_b = create(:trace, :visibility => "private", :user => user) do |trace|
+    trace_b = create(:trace, :visibility => "trackable", :user => user) do |trace|
       create(:tracetag, :trace => trace, :tag => "London")
     end
 
@@ -145,8 +145,8 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     second_user = create(:user)
     third_user = create(:user)
     create(:trace)
-    trace_b = create(:trace, :visibility => "public", :user => user)
-    trace_c = create(:trace, :visibility => "private", :user => user) do |trace|
+    trace_b = create(:trace, :visibility => "identifiable", :user => user)
+    trace_c = create(:trace, :visibility => "trackable", :user => user) do |trace|
       create(:tracetag, :trace => trace, :tag => "London")
     end
 
@@ -322,7 +322,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
   # Test showing a trace
   def test_show
-    public_trace_file = create(:trace, :visibility => "public")
+    public_trace_file = create(:trace, :visibility => "identifiable")
 
     # First with no auth, which should work since the trace is public
     get show_trace_path(public_trace_file.user, public_trace_file)
@@ -341,7 +341,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
   # Check an anonymous trace can't be viewed by another user
   def test_show_anon
-    anon_trace_file = create(:trace, :visibility => "private")
+    anon_trace_file = create(:trace, :visibility => "trackable")
 
     # First with no auth
     get show_trace_path(anon_trace_file.user, anon_trace_file)
@@ -387,22 +387,25 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     assert_template :new
     assert_select "select#trace_visibility option[value=identifiable][selected]", 1
 
-    # Now authenticated as a user with gps.trace.public set
+    # Now authenticated as a user with the legacy gps.trace.public preference,
+    # which is no longer supported and falls back to trackable
     second_user = create(:user)
     create(:user_preference, :user => second_user, :k => "gps.trace.public", :v => "default")
     session_for(second_user)
     get new_trace_path
     assert_response :success
     assert_template :new
-    assert_select "select#trace_visibility option[value=public][selected]", 1
+    assert_select "select#trace_visibility option[value=trackable][selected]", 1
 
-    # Now authenticated as a user with no preferences
+    # Now authenticated as a user with no preferences, defaults to trackable
     third_user = create(:user)
     session_for(third_user)
     get new_trace_path
     assert_response :success
     assert_template :new
-    assert_select "select#trace_visibility option[value=private][selected]", 1
+    assert_select "select#trace_visibility option[value=trackable][selected]", 1
+    # New uploads only offer the two supported visibilities
+    assert_select "select#trace_visibility option", 2
   end
 
   # Test creating a trace
@@ -437,6 +440,19 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     assert_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
   end
 
+  # A legacy visibility (public or private) is no longer accepted on upload
+  def test_create_post_with_legacy_visibility
+    fixture = Rails.root.join("test/gpx/fixtures/a.gpx")
+    file = Rack::Test::UploadedFile.new(fixture, "application/gpx+xml")
+    user = create(:user)
+    session_for(user)
+
+    assert_no_difference "Trace.count" do
+      post traces_path, :params => { :trace => { :gpx_file => file, :description => "New Trace", :tagstring => "new,trace", :visibility => "public" } }
+    end
+    assert_template :new
+  end
+
   # Test creating a trace with validation errors
   def test_create_post_with_validation_errors
     # Get file to use
@@ -455,7 +471,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
   # Test fetching the edit page for a trace using GET
   def test_edit_get
-    public_trace_file = create(:trace, :visibility => "public")
+    public_trace_file = create(:trace, :visibility => "identifiable")
     deleted_trace_file = create(:trace, :deleted)
 
     # First with no auth
@@ -483,13 +499,25 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
+  # An old visibility stays selected in the edit form, so changing other fields
+  # does not change it by mistake.
+  def test_edit_get_keeps_legacy_visibility
+    trace = create(:trace, :without_validations, :visibility => "private")
+
+    session_for(trace.user)
+    get edit_trace_path(:display_name => trace.user.display_name, :id => trace)
+    assert_response :success
+    assert_select "select#trace_visibility option", 3
+    assert_select "select#trace_visibility option[value=private][selected]", 1
+  end
+
   # Test saving edits to a trace
   def test_update
-    public_trace_file = create(:trace, :visibility => "public")
+    public_trace_file = create(:trace, :visibility => "identifiable")
     deleted_trace_file = create(:trace, :deleted)
 
     # New details
-    new_details = { :description => "Changed description", :tagstring => "new_tag", :visibility => "private" }
+    new_details = { :description => "Changed description", :tagstring => "new_tag", :visibility => "trackable" }
 
     # First with no auth
     put trace_path(:display_name => public_trace_file.user.display_name, :id => public_trace_file, :trace => new_details)
@@ -533,7 +561,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
 
   # Test destroying a trace
   def test_destroy
-    public_trace_file = create(:trace, :visibility => "public")
+    public_trace_file = create(:trace, :visibility => "identifiable")
     deleted_trace_file = create(:trace, :deleted)
 
     # First with no auth
@@ -563,7 +591,7 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     assert_not trace.visible
 
     # Finally with a trace that is destroyed by an admin
-    public_trace_file = create(:trace, :visibility => "public")
+    public_trace_file = create(:trace, :visibility => "identifiable")
     admin = create(:administrator_user)
     session_for(admin)
     delete trace_path(:display_name => public_trace_file.user.display_name, :id => public_trace_file)
diff --git a/test/factories/traces.rb b/test/factories/traces.rb
index c4845be3b..53684c61d 100644
--- a/test/factories/traces.rb
+++ b/test/factories/traces.rb
@@ -15,6 +15,12 @@ FactoryBot.define do
       visible { false }
     end
 
+    # Insert a trace with a legacy visibility (public/private) by skipping
+    # validations, so tests can check that old traces still work.
+    trait :without_validations do
+      to_create { |instance| instance.save(:validate => false) }
+    end
+
     transient do
       fixture { nil }
     end
diff --git a/test/helpers/trace_helper_test.rb b/test/helpers/trace_helper_test.rb
new file mode 100644
index 000000000..dbc90a226
--- /dev/null
+++ b/test/helpers/trace_helper_test.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class TraceHelperTest < ActionView::TestCase
+  test "trace_visibility_options includes a legacy option for traces that have it" do
+    public_trace = build(:trace, :visibility => "public")
+    options = trace_visibility_options(public_trace)
+    assert_equal %w[public trackable identifiable], options.map(&:last)
+
+    private_trace = build(:trace, :visibility => "private")
+    options = trace_visibility_options(private_trace)
+    assert_equal %w[private trackable identifiable], options.map(&:last)
+  end
+
+  test "trace_visibility_options only includes current visibilities for traces without a legacy one" do
+    trackable_trace = build(:trace, :visibility => "trackable")
+    options = trace_visibility_options(trackable_trace)
+    assert_equal %w[trackable identifiable], options.map(&:last)
+
+    identifiable_trace = build(:trace, :visibility => "identifiable")
+    options = trace_visibility_options(identifiable_trace)
+    assert_equal %w[trackable identifiable], options.map(&:last)
+  end
+end
diff --git a/test/models/trace_test.rb b/test/models/trace_test.rb
index 4415803b1..59fb62bf4 100644
--- a/test/models/trace_test.rb
+++ b/test/models/trace_test.rb
@@ -15,12 +15,12 @@ class TraceTest < ActiveSupport::TestCase
     second_user = create(:user)
     third_user = create(:user)
     fourth_user = create(:user)
-    public_trace_file = create(:trace, :visibility => "public", :user => first_user)
-    anon_trace_file = create(:trace, :visibility => "private", :user => second_user)
+    public_trace_file = create(:trace, :without_validations, :visibility => "public", :user => first_user)
+    anon_trace_file = create(:trace, :without_validations, :visibility => "private", :user => second_user)
     identifiable_trace_file = create(:trace, :visibility => "identifiable", :user => first_user)
-    pending_trace_file = create(:trace, :visibility => "public", :user => second_user, :inserted => false)
+    pending_trace_file = create(:trace, :without_validations, :visibility => "public", :user => second_user, :inserted => false)
     trackable_trace_file = create(:trace, :visibility => "trackable", :user => second_user)
-    _other_trace_file = create(:trace, :visibility => "private", :user => third_user)
+    _other_trace_file = create(:trace, :without_validations, :visibility => "private", :user => third_user)
 
     check_query(Trace.visible_to(first_user), [
                   public_trace_file, identifiable_trace_file, pending_trace_file
@@ -35,12 +35,12 @@ class TraceTest < ActiveSupport::TestCase
   end
 
   def test_visible_to_all
-    public_trace_file = create(:trace, :visibility => "public")
-    _private_trace_file = create(:trace, :visibility => "private")
+    public_trace_file = create(:trace, :without_validations, :visibility => "public")
+    _private_trace_file = create(:trace, :without_validations, :visibility => "private")
     identifiable_trace_file = create(:trace, :visibility => "identifiable")
     _trackable_trace_file = create(:trace, :visibility => "trackable")
-    deleted_trace_file = create(:trace, :deleted, :visibility => "public")
-    pending_trace_file = create(:trace, :visibility => "public", :inserted => false)
+    deleted_trace_file = create(:trace, :deleted, :without_validations, :visibility => "public")
+    pending_trace_file = create(:trace, :without_validations, :visibility => "public", :inserted => false)
 
     check_query(Trace.visible_to_all, [
                   public_trace_file, identifiable_trace_file,
@@ -68,13 +68,45 @@ class TraceTest < ActiveSupport::TestCase
     trace_valid({ :description => nil }, :valid => false)
     trace_valid({ :description => "a" * 255 })
     trace_valid({ :description => "a" * 256 }, :valid => false)
-    trace_valid({ :visibility => "private" })
-    trace_valid({ :visibility => "public" })
     trace_valid({ :visibility => "trackable" })
     trace_valid({ :visibility => "identifiable" })
+    # Legacy visibilities are no longer allowed on new traces
+    trace_valid({ :visibility => "private" }, :valid => false)
+    trace_valid({ :visibility => "public" }, :valid => false)
     trace_valid({ :visibility => "foo" }, :valid => false)
   end
 
+  def test_visibility_cannot_change_to_legacy
+    trace = create(:trace, :visibility => "trackable")
+    trace.visibility = "public"
+    assert_not_predicate trace, :valid?
+
+    trace = create(:trace, :visibility => "identifiable")
+    trace.visibility = "private"
+    assert_not_predicate trace, :valid?
+  end
+
+  def test_visibility_can_change_from_legacy
+    trace = create(:trace, :without_validations, :visibility => "public")
+    trace.visibility = "trackable"
+    assert_predicate trace, :valid?
+
+    trace = create(:trace, :without_validations, :visibility => "private")
+    trace.visibility = "public"
+    assert_predicate trace, :valid?
+  end
+
+  def test_selectable_visibilities
+    trace = build(:trace, :visibility => "identifiable")
+    assert_equal %w[trackable identifiable], trace.selectable_visibilities
+
+    trace = build(:trace, :visibility => "public")
+    assert_equal %w[public trackable identifiable], trace.selectable_visibilities
+
+    trace = build(:trace, :visibility => "private")
+    assert_equal %w[private trackable identifiable], trace.selectable_visibilities
+  end
+
   def test_tagstring_handles_space_separated_tags
     trace = build(:trace)
     trace.tagstring = "foo bar baz"
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 42f69b6a3..4757c2e13 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -396,6 +396,23 @@ class UserTest < ActiveSupport::TestCase
     assert_equal "fr", preference.v
   end
 
+  def test_default_trace_visibility_undefined
+    user = create(:user)
+    assert_equal "trackable", user.default_trace_visibility
+  end
+
+  def test_default_trace_visibility_known
+    user = create(:user)
+    create(:user_preference, :user => user, :k => "gps.trace.visibility", :v => "identifiable")
+    assert_equal "identifiable", user.default_trace_visibility
+  end
+
+  def test_default_trace_visibility_unsupported
+    user = create(:user)
+    create(:user_preference, :user => user, :k => "gps.trace.visibility", :v => "public")
+    assert_equal "trackable", user.default_trace_visibility
+  end
+
   def test_visible?
     assert_predicate build(:user, :pending), :visible?
     assert_predicate build(:user, :active), :visible?
-- 
2.47.3