Merge branch '240920-img-resizing-add-ff-on-avatar-owner' into 'master'

Add image resizing FF based on avatar owner

See merge request gitlab-org/gitlab!40606

app/controllers/concerns/send_file_upload.rb

@@ -53,8 +53,10 @@ module SendFileUpload
   private
 
   def image_scaling_request?(file_upload)
-    Feature.enabled?(:dynamic_image_resizing, current_user) &&
-      avatar_safe_for_scaling?(file_upload) && valid_image_scaling_width? && current_user
+    avatar_safe_for_scaling?(file_upload) &&
+      scaling_allowed_by_feature_flags?(file_upload) &&
+      valid_image_scaling_width? &&
+      current_user
   end
 
   def avatar_safe_for_scaling?(file_upload)
@@ -68,4 +70,17 @@ module SendFileUpload
   def valid_image_scaling_width?
     Avatarable::ALLOWED_IMAGE_SCALER_WIDTHS.include?(params[:width]&.to_i)
   end
+
+  # We use two separate feature gates to allow image resizing.
+  # The first, `:dynamic_image_resizing_requester`, based on the content requester.
+  # Enabling it for the user would allow that user to send resizing requests for any avatar.
+  # The second, `:dynamic_image_resizing_owner`, based on the content owner.
+  # Enabling it for the user would allow anyone to send resizing requests against the mentioned user avatar only.
+  # This flag allows us to operate on trusted data only, more in https://gitlab.com/gitlab-org/gitlab/-/issues/241533.
+  # Because of this, you need to enable BOTH to serve resized image,
+  # as you would need at least one allowed requester and at least one allowed avatar.
+  def scaling_allowed_by_feature_flags?(file_upload)
+    Feature.enabled?(:dynamic_image_resizing_requester, current_user) &&
+      Feature.enabled?(:dynamic_image_resizing_owner, file_upload.model)
+  end
 end

config/feature_flags/development/dynamic_image_resizing_owner.yml

+---
+name: dynamic_image_resizing_owner
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40606
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/241533
+group: group::memory
+type: development
+default_enabled: false

config/feature_flags/development/dynamic_image_resizing.yml → config/feature_flags/development/dynamic_image_resizing_requester.yml

 ---
-name: dynamic_image_resizing
+name: dynamic_image_resizing_requester
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37342
 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/233704
 group: group::memory
 type: development
-default_enabled: false
\ No newline at end of file
+default_enabled: false

spec/controllers/concerns/send_file_upload_spec.rb

@@ -49,12 +49,13 @@ RSpec.describe SendFileUpload do
     end
 
     shared_examples 'handles image resize requests' do
+      let(:headers) { double }
+      let(:image_requester) { build(:user) }
+      let(:image_owner) { build(:user) }
       let(:params) do
         { attachment: 'avatar.png' }
       end
 
-      let(:headers) { double }
-
       before do
         allow(uploader).to receive(:image_safe_for_scaling?).and_return(true)
         allow(uploader).to receive(:mounted_as).and_return(:avatar)
@@ -64,81 +65,110 @@ RSpec.describe SendFileUpload do
         # local or remote files
         allow(controller).to receive(:send_file)
         allow(controller).to receive(:redirect_to)
+
+        allow(controller).to receive(:current_user).and_return(image_requester)
+        allow(uploader).to receive(:model).and_return(image_owner)
       end
 
-      context 'when feature is enabled for current user' do
-        let(:user) { build(:user) }
+      context 'when boths FFs are enabled' do
+        before do
+          stub_feature_flags(dynamic_image_resizing_requester: image_requester)
+          stub_feature_flags(dynamic_image_resizing_owner: image_owner)
+        end
 
+        it_behaves_like 'handles image resize requests allowed by FFs'
+      end
+
+      context 'when only FF based on content requester is enabled for current user' do
         before do
-          stub_feature_flags(dynamic_image_resizing: user)
-          allow(controller).to receive(:current_user).and_return(user)
+          stub_feature_flags(dynamic_image_resizing_requester: image_requester)
+          stub_feature_flags(dynamic_image_resizing_owner: false)
         end
 
-        context 'with valid width parameter' do
-          it 'renders OK with workhorse command header' do
-            expect(controller).not_to receive(:send_file)
-            expect(controller).to receive(:params).at_least(:once).and_return(width: '64')
-            expect(controller).to receive(:head).with(:ok)
-            expect(Gitlab::Workhorse).to receive(:send_scaled_image).with(a_string_matching('^(/.+|https://.+)'), 64, 'image/png').and_return([
-              Gitlab::Workhorse::SEND_DATA_HEADER, "send-scaled-img:faux"
-            ])
-            expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, "send-scaled-img:faux")
-
-            subject
-          end
+        it_behaves_like 'bypasses image resize requests not allowed by FFs'
+      end
+
+      context 'when only FF based on content owner is enabled for requested avatar owner' do
+        before do
+          stub_feature_flags(dynamic_image_resizing_requester: false)
+          stub_feature_flags(dynamic_image_resizing_owner: image_owner)
         end
 
-        context 'with missing width parameter' do
-          it 'does not write workhorse command header' do
-            expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+        it_behaves_like 'bypasses image resize requests not allowed by FFs'
+      end
 
-            subject
-          end
+      context 'when both FFs are disabled' do
+        before do
+          stub_feature_flags(dynamic_image_resizing_requester: false)
+          stub_feature_flags(dynamic_image_resizing_owner: false)
         end
 
-        context 'with invalid width parameter' do
-          it 'does not write workhorse command header' do
-            expect(controller).to receive(:params).at_least(:once).and_return(width: 'not a number')
-            expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+        it_behaves_like 'bypasses image resize requests not allowed by FFs'
+      end
+    end
+
+    shared_examples 'bypasses image resize requests not allowed by FFs' do
+      it 'does not write workhorse command header' do
+        expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
 
-            subject
-          end
+        subject
+      end
+    end
+
+    shared_examples 'handles image resize requests allowed by FFs' do
+      context 'with valid width parameter' do
+        it 'renders OK with workhorse command header' do
+          expect(controller).not_to receive(:send_file)
+          expect(controller).to receive(:params).at_least(:once).and_return(width: '64')
+          expect(controller).to receive(:head).with(:ok)
+
+          expect(Gitlab::Workhorse).to receive(:send_scaled_image).with(a_string_matching('^(/.+|https://.+)'), 64, 'image/png').and_return([
+            Gitlab::Workhorse::SEND_DATA_HEADER, "send-scaled-img:faux"
+          ])
+          expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, "send-scaled-img:faux")
+
+          subject
         end
+      end
 
-        context 'with width that is not allowed' do
-          it 'does not write workhorse command header' do
-            expect(controller).to receive(:params).at_least(:once).and_return(width: '63')
-            expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+      context 'with missing width parameter' do
+        it 'does not write workhorse command header' do
+          expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
 
-            subject
-          end
+          subject
         end
+      end
 
-        context 'when image file is not an avatar' do
-          it 'does not write workhorse command header' do
-            expect(uploader).to receive(:mounted_as).and_return(nil) # FileUploader is not mounted
-            expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+      context 'with invalid width parameter' do
+        it 'does not write workhorse command header' do
+          expect(controller).to receive(:params).at_least(:once).and_return(width: 'not a number')
+          expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
 
-            subject
-          end
+          subject
         end
+      end
 
-        context 'when image file type is not considered safe for scaling' do
-          it 'does not write workhorse command header' do
-            expect(uploader).to receive(:image_safe_for_scaling?).and_return(false)
-            expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+      context 'with width that is not allowed' do
+        it 'does not write workhorse command header' do
+          expect(controller).to receive(:params).at_least(:once).and_return(width: '63')
+          expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
 
-            subject
-          end
+          subject
         end
       end
 
-      context 'when feature is disabled' do
-        before do
-          stub_feature_flags(dynamic_image_resizing: false)
+      context 'when image file is not an avatar' do
+        it 'does not write workhorse command header' do
+          expect(uploader).to receive(:mounted_as).and_return(nil) # FileUploader is not mounted
+          expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+
+          subject
         end
+      end
 
+      context 'when image file type is not considered safe for scaling' do
         it 'does not write workhorse command header' do
+          expect(uploader).to receive(:image_safe_for_scaling?).and_return(false)
           expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
 
           subject