Merge branch 'eb-visual-review-tools-pat' into 'master'

Support PAT authentication for visual reviews

See merge request gitlab-org/gitlab!29336

doc/ci/review_apps/index.md

@@ -188,9 +188,6 @@ With Visual Reviews, you can provide a feedback form to your Review Apps so
 that reviewers can post comments directly from the app back to the merge request
 that spawned the Review App.
 
-NOTE: **Note:** Visual Reviews currently only work for public projects. Support for private
-and internal projects [is planned](https://gitlab.com/gitlab-org/gitlab/-/issues/42750).
-
 ### Configuring Visual Reviews
 
 Ensure that the `anonymous_visual_review_feedback` feature flag is enabled.
@@ -218,6 +215,7 @@ looks like:
   data-merge-request-id='1'
   data-mr-url='https://gitlab.example.com'
   data-project-path='sarah/review-app-tester'
+  data-require-auth='true'
   id='review-app-toolbar-script'
   src='https://gitlab.example.com/assets/webpack/visual_review_toolbar.js'>
 </script>
@@ -235,6 +233,7 @@ to replace those values at runtime when each review app is created:
 - `data-mr-url` is the URL of the GitLab instance and will be the same for all
   review apps.
 - `data-project-path` is the project's path, which can be found by `CI_PROJECT_PATH`.
+- `data-require-auth` is optional for public projects but required for [private and internal ones](#visual-reviews-in-private-or-internal-projects). If this is set to `true`, the user will be required to enter their [personal access token](../../user/profile/personal_access_tokens.md) instead of their name and email.
 - `id` is always `review-app-toolbar-script`, you don't need to change that.
 - `src` is the source of the review toolbar script, which resides in the
   respective GitLab instance and will be the same for all review apps.
@@ -272,6 +271,15 @@ can supply the ID by either:​​
 - Dynamically adding the `data-merge-request-id` value during the build of the app.
 - Supplying it manually through the visual review form in the app.
 
+### Visual Reviews in private or internal projects
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/42750#note_317271120) in GitLab 12.10.
+
+To enable visual reviews for private and internal projects, set the
+[`data-require-auth` variable](#configuring-visual-reviews) to `true`. When enabled,
+the user must enter a [personal access token](../../user/profile/personal_access_tokens.md)
+with `read_api` scope before submitting feedback.
+
 ### Using Visual Reviews
 
 After Visual Reviews has been [enabled](#configuring-visual-reviews) for the
@@ -285,7 +293,7 @@ To use the feedback form:
 1. Make a comment on the visual review. You can make use of all the
    [Markdown annotations](../../user/markdown.md) that are also available in
    merge request comments.
-1. Submit your feedback anonymously or add your name.
+1. If `data-require-auth` is `true`, you must enter your [personal access token](../../user/profile/personal_access_tokens.md). Otherwise, you must enter your name, and optionally, your email.
 1. Finally, click **Send feedback**.
 
 After you make and submit a comment in the visual review box, it will appear

ee/app/services/notes/create_visual_review_service.rb

+# frozen_string_literal: true
+
+module Notes
+  class CreateVisualReviewService < CreateService
+    def initialize(merge_request, current_user, body:, position: nil)
+      super(
+        merge_request.project,
+        User.visual_review_bot,
+        {
+          note: note_body(current_user, body),
+          position: position,
+          type: 'DiscussionNote',
+          noteable_type: 'MergeRequest',
+          noteable_id: merge_request.id
+        }
+      )
+    end
+
+    private
+
+    def note_body(user, body)
+      if user && body.present?
+        "**Feedback from @#{user.username} (#{user.email})**\n\n#{body}"
+      else
+        body
+      end
+    end
+  end
+end

ee/changelogs/unreleased/eb-visual-review-tools-pat.yml

+---
+title: Support visual reviews on private and internal projects through PAT authentication
+merge_request: 29336
+author:
+type: fixed

ee/lib/api/visual_review_discussions.rb

@@ -36,15 +36,16 @@ module API
           forbidden!('Anonymous visual review feedback is disabled')
         end
 
-        merge_request = find_merge_request_without_permissions_check(params[:merge_request_iid])
+        merge_request = find_merge_request(params[:merge_request_iid])
 
-        note = create_visual_review_note(merge_request, {
-          note: params[:body],
-          type: 'DiscussionNote',
-          noteable_type: 'MergeRequest',
-          position: params[:position],
-          noteable_id: merge_request.id
-        })
+        note = ::Notes::CreateVisualReviewService.new(
+          merge_request,
+          current_user,
+          {
+            body: params[:body],
+            position: params[:position]
+          }
+        ).execute
 
         if note.valid?
           present note.discussion, with: Entities::Discussion

ee/lib/ee/api/helpers/notes_helpers.rb

@@ -23,22 +23,14 @@ module EE
           end
         end
 
-        # Used only for anonymous Visual Review Tools feedback
-        def find_merge_request_without_permissions_check(noteable_id)
-          params = finder_params_by_noteable_type_and_id(::MergeRequest, noteable_id)
-
-          ::NotesFinder.new(current_user, params).target || not_found!(noteable_type)
-        end
-
-        def create_visual_review_note(noteable, opts)
-          unless ::Feature.enabled?(:anonymous_visual_review_feedback)
-            forbidden!('Anonymous visual review feedback is disabled')
-          end
-
-          parent  = noteable_parent(noteable)
-          project = parent if parent.is_a?(Project)
-
-          ::Notes::CreateService.new(project, ::User.visual_review_bot, opts).execute
+        # This is mainly used finding the target MR of the Visual Review note.
+        # If current_user is nil (PAT is not passed), only public merge requests can be found
+        # If current_user is present (PAT is passed), private projects can be found as long as user is a project member.
+        # If current_user is present (PAT is passed), internal projects can be found by any authenticated user.
+        def find_merge_request(merge_request_iid)
+          params = finder_params_by_noteable_type_and_id(::MergeRequest, merge_request_iid)
+
+          ::NotesFinder.new(current_user, params).target || not_found!(::MergeRequest)
         end
       end
     end

ee/spec/requests/api/visual_review_discussions_spec.rb

@@ -3,19 +3,39 @@
 require 'spec_helper'
 
 describe API::VisualReviewDiscussions do
-  let(:user)     { create(:user) }
-  let!(:project) { create(:project, :public, :repository, namespace: user.namespace) }
-
-  context 'when sending merge request feedback from a visual review app without authentication' do
-    let!(:merge_request) do
-      create(:merge_request_with_diffs, source_project: project, target_project: project, author: user)
+  shared_examples_for 'accepting request without authentication' do
+    let(:request) do
+      post api("/projects/#{project_id}/merge_requests/#{merge_request.iid}/visual_review_discussions"), params: note_params
     end
 
+    it_behaves_like 'handling merge request feedback'
+  end
+
+  shared_examples_for 'accepting request with authentication' do
+    let(:token) { create(:personal_access_token) }
+    let(:user) { token.user }
+
     let(:request) do
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/visual_review_discussions"), params: note_params
+      post api("/projects/#{project_id}/merge_requests/#{merge_request.iid}/visual_review_discussions"), params: note_params, headers: { 'Private-Token' => token.token }
+    end
+
+    before do
+      create(:project_member,
+             user: user,
+             project: project,
+             access_level: ProjectMember::DEVELOPER)
+    end
+
+    it_behaves_like 'handling merge request feedback', :with_auth
+  end
+
+  shared_examples_for 'handling merge request feedback' do |with_auth|
+    let!(:merge_request) do
+      create(:merge_request_with_diffs, source_project: project, target_project: project)
     end
 
-    let(:note_params)   { { body: 'hi!' } }
+    let(:project_id) { project.id }
+    let(:note_params) { { body: 'hi!', created_at: 2.weeks.ago } }
     let(:response_note) { json_response['notes'].first }
 
     it 'creates a new note' do
@@ -53,8 +73,14 @@ describe API::VisualReviewDiscussions do
         expect(response).to have_gitlab_http_status(:created)
       end
 
-      it 'returns the persisted note body' do
-        expect(response_note['body']).to eq('hi!')
+      if with_auth
+        it 'returns the persisted note body including user details' do
+          expect(response_note['body']).to eq("**Feedback from @#{user.username} (#{user.email})**\n\nhi!")
+        end
+      else
+        it 'returns the persisted note body' do
+          expect(response_note['body']).to eq('hi!')
+        end
       end
 
       it 'returns the name of the Visual Review Bot assigned as the author' do
@@ -64,6 +90,10 @@ describe API::VisualReviewDiscussions do
       it 'returns the id of the merge request as the parent noteable_id' do
         expect(response_note['noteable_id']).to eq(merge_request.id)
       end
+
+      it 'returns a current time stamp instead of the provided one' do
+        expect(Time.parse(response_note['created_at']) > 1.day.ago).to eq(true)
+      end
     end
 
     context 'with no message body' do
@@ -76,10 +106,26 @@ describe API::VisualReviewDiscussions do
       end
     end
 
+    context 'with an invalid project ID' do
+      let(:project_id) { project.id + 1 }
+
+      it 'does not create a new note' do
+        expect { request }.not_to change(Note, :count)
+      end
+
+      describe 'the API response' do
+        it 'responds with a status 404' do
+          request
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
     context 'with an invalid merge request IID' do
       let(:merge_request) { double(iid: 546574823564) }
 
-      it 'creates a new note' do
+      it 'does not create a new note' do
         expect { request }.not_to change(Note, :count)
       end
 
@@ -115,43 +161,77 @@ describe API::VisualReviewDiscussions do
         end
       end
     end
+  end
+
+  shared_examples_for 'rejecting request without authentication' do
+    let!(:merge_request) do
+      create(:merge_request_with_diffs, source_project: project, target_project: project)
+    end
 
-    context 'when an admin or owner makes an authenticated request' do
-      let(:request) do
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/visual_review_discussions", project.owner), params: note_params
+    let(:project_id) { project.id }
+    let(:note_params) { { body: 'hi!', created_at: 2.weeks.ago } }
+
+    let(:request) do
+      post api("/projects/#{project_id}/merge_requests/#{merge_request.iid}/visual_review_discussions"), params: note_params
+    end
+
+    it 'returns a 404 project not found' do
+      expect { request }.not_to change(merge_request.notes, :count)
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+
+  context 'when project is public' do
+    let!(:project) { create(:project, :public, :repository) }
+
+    it_behaves_like 'accepting request without authentication'
+    it_behaves_like 'accepting request with authentication'
+  end
+
+  context 'when project is private' do
+    let!(:project) { create(:project, :private, :repository) }
+
+    it_behaves_like 'accepting request with authentication'
+    it_behaves_like 'rejecting request without authentication'
+
+    context 'and authenticated user has no project access' do
+      let!(:merge_request) do
+        create(:merge_request_with_diffs, source_project: project, target_project: project)
       end
 
+      let(:token) { create(:personal_access_token) }
+      let(:user) { token.user }
+      let(:project_id) { project.id }
       let(:note_params) { { body: 'hi!', created_at: 2.weeks.ago } }
 
-      it 'creates a new note' do
-        expect { request }.to change(merge_request.notes, :count).by(1)
+      let(:request) do
+        post api("/projects/#{project_id}/merge_requests/#{merge_request.iid}/visual_review_discussions"), params: note_params, headers: { 'Private-Token' => token.token }
       end
 
-      describe 'the API response' do
-        before do
-          request
-        end
+      it 'returns a 404 project not found' do
+        expect { request }.not_to change(merge_request.notes, :count)
 
-        it 'responds with a status 201 Created' do
-          expect(response).to have_gitlab_http_status(:created)
-        end
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
 
-        it 'returns the persisted note body' do
-          expect(response_note['body']).to eq('hi!')
-        end
+  context 'when project is internal' do
+    let!(:project) { create(:project, :internal, :repository) }
 
-        it 'returns the name of the Visual Review Bot assigned as the author' do
-          expect(response_note['author']['username']).to eq(User.visual_review_bot.username)
-        end
+    it_behaves_like 'accepting request with authentication'
+    it_behaves_like 'rejecting request without authentication'
 
-        it 'returns the id of the merge request as the parent noteable_id' do
-          expect(response_note['noteable_id']).to eq(merge_request.id)
-        end
+    context 'and authenticated user has no project access' do
+      let(:token) { create(:personal_access_token) }
+      let(:user) { token.user }
 
-        it 'returns a current time stamp instead of the provided one' do
-          expect(Time.parse(response_note['created_at']) > 1.day.ago).to eq(true)
-        end
+      let(:request) do
+        post api("/projects/#{project_id}/merge_requests/#{merge_request.iid}/visual_review_discussions"), params: note_params, headers: { 'Private-Token' => token.token }
       end
+
+      it_behaves_like 'handling merge request feedback', :with_auth
     end
   end
 end

package.json

@@ -41,7 +41,7 @@
     "@gitlab/at.js": "1.5.5",
     "@gitlab/svgs": "1.119.0",
     "@gitlab/ui": "12.0.0",
-    "@gitlab/visual-review-tools": "1.5.1",
+    "@gitlab/visual-review-tools": "1.6.1",
     "@sentry/browser": "^5.10.2",
     "@sourcegraph/code-host-integration": "0.0.36",
     "apollo-cache-inmemory": "^1.6.3",

yarn.lock

@@ -806,10 +806,10 @@
     vue-loader "^15.4.2"
     vue-runtime-helpers "^1.1.2"
 
-"@gitlab/visual-review-tools@1.5.1":
-  version "1.5.1"
-  resolved "https://registry.yarnpkg.com/@gitlab/visual-review-tools/-/visual-review-tools-1.5.1.tgz#2552927cd7a376f1f06ef3293a69fe2ffcdddb52"
-  integrity sha512-8d6xgK4TsLA5gucd78jzaMyginAMJ8cbu/6ghUGws84zzAEsyJsMTstyt/fA5l4toQXVxtOh90BvDzwxSjZ6hQ==
+"@gitlab/visual-review-tools@1.6.1":
+  version "1.6.1"
+  resolved "https://registry.yarnpkg.com/@gitlab/visual-review-tools/-/visual-review-tools-1.6.1.tgz#0d8f3ff9f51b05f7c80b9a107727703d48997e4e"
+  integrity sha512-vY8K1igwZFoEOmU0h4E7XTLlilsQ4ylPr27O01UsSe6ZTKi6oEMREsRAEpNIUgRlxUARCsf+Opp4pgSFzFkFcw==
 
 "@gitlab/vue-toasted@^1.3.0":
   version "1.3.0"