Improve personal snippet access workflow. Fixes #3258

CHANGELOG

@@ -14,6 +14,7 @@ v 8.2.0 (unreleased)
   - Fix: 500 error returned if destroy request without HTTP referer (Kazuki Shimizu)
   - Remove deprecated CI events from project settings page
   - Use issue editor as cross reference comment author when issue is edited with a new mention.
+  - Improve personal snippet access workflow
 v 8.1.1
   - Fix cloning Wiki repositories via HTTP (Stan Hu)

app/controllers/snippets_controller.rb

 class SnippetsController < ApplicationController
   before_action :snippet, only: [:show, :edit, :destroy, :update, :raw]
+  # Allow read snippet
+  before_action :authorize_show_snippet!, only: [:show]
+
   # Allow modify snippet
   before_action :authorize_update_snippet!, only: [:edit, :update]
@@ -79,10 +82,14 @@ class SnippetsController < ApplicationController
                      [Snippet::PUBLIC, Snippet::INTERNAL]).
                      find(params[:id])
                  else
-                   PersonalSnippet.are_public.find(params[:id])
+                   PersonalSnippet.find(params[:id])
                  end
   end
+  def authorize_show_snippet!
+    authenticate_user! unless can?(current_user, :read_personal_snippet, @snippet)
+  end
+
   def authorize_update_snippet!
     return render_404 unless can?(current_user, :update_personal_snippet, @snippet)
   end

app/models/ability.rb

@@ -22,12 +22,17 @@ class Ability
     # List of possible abilities
     # for non-authenticated user
     def not_auth_abilities(user, subject)
+      return not_auth_personal_snippet_abilities(subject) if subject.kind_of?(PersonalSnippet)
+      return not_auth_project_abilities(subject) if subject.kind_of?(Project) || subject.respond_to?(:project)
+      return not_auth_group_abilities(subject) if subject.kind_of?(Group) || subject.respond_to?(:group)
+      []
+    end
+
+    def not_auth_project_abilities(subject)
       project = if subject.kind_of?(Project)
                   subject
-                elsif subject.respond_to?(:project)
-                  subject.project
                 else
-                  nil
+                  subject.project
                 end
       if project && project.public?
@@ -47,19 +52,29 @@ class Ability
         rules - project_disabled_features_rules(project)
       else
-        group = if subject.kind_of?(Group)
-                  subject
-                elsif subject.respond_to?(:group)
-                  subject.group
-                else
-                  nil
-                end
-        if group && group.public_profile?
-          [:read_group]
-        else
-          []
-        end
+        []
+      end
+    end
+    def not_auth_group_abilities(subject)
+      group = if subject.kind_of?(Group)
+                subject
+              else
+                subject.group
+              end
+
+      if group && group.public_profile?
+        [:read_group]
+      else
+        []
+      end
+    end
+
+    def not_auth_personal_snippet_abilities(snippet)
+      if snippet.public?
+        [:read_personal_snippet]
+      else
+        []
       end
     end
@@ -278,7 +293,7 @@ class Ability
       end
     end
-    [:note, :project_snippet, :personal_snippet].each do |name|
+    [:note, :project_snippet].each do |name|
       define_method "#{name}_abilities" do |user, subject|
         rules = []
@@ -298,6 +313,24 @@ class Ability
       end
     end
+    def personal_snippet_abilities(user, snippet)
+      rules = []
+
+      if snippet.author == user
+        rules += [
+          :read_personal_snippet,
+          :update_personal_snippet,
+          :admin_personal_snippet
+        ]
+      end
+
+      if snippet.public? || snippet.internal?
+        rules.push(:read_snippet)
+      end
+
+      rules
+    end
+
     def group_member_abilities(user, subject)
       rules = []
       target_user = subject.user

spec/controllers/snippets_controller_spec.rb

+require 'spec_helper'
+
+describe SnippetsController do
+  describe 'GET #show' do
+    let(:user) { create(:user) }
+
+    context 'when the personal snippet is private' do
+      let(:personal_snippet) { create(:personal_snippet, :private, author: user) }
+
+      context 'when signed in' do
+        before do
+          sign_in(user)
+        end
+
+        context 'when signed in user is not the author' do
+          let(:other_author) { create(:author) }
+          let(:other_personal_snippet) { create(:personal_snippet, :private, author: other_author) }
+
+          it 'responds with status 404' do
+            get :show, id: other_personal_snippet.to_param
+
+            expect(response.status).to eq(404)
+          end
+        end
+
+        context 'when signed in user is the author' do
+          it 'renders the snippet' do
+            get :show, id: personal_snippet.to_param
+
+            expect(assigns(:snippet)).to eq(personal_snippet)
+            expect(response.status).to eq(200)
+          end
+        end
+      end
+
+      context 'when not signed in' do
+        it 'redirects to the sign in page' do
+          get :show, id: personal_snippet.to_param
+
+          expect(response).to redirect_to(new_user_session_path)
+        end
+      end
+    end
+
+    context 'when the personal snippet is internal' do
+      let(:personal_snippet) { create(:personal_snippet, :internal, author: user) }
+
+      context 'when signed in' do
+        before do
+          sign_in(user)
+        end
+
+        it 'renders the snippet' do
+          get :show, id: personal_snippet.to_param
+
+          expect(assigns(:snippet)).to eq(personal_snippet)
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context 'when not signed in' do
+        it 'redirects to the sign in page' do
+          get :show, id: personal_snippet.to_param
+
+          expect(response).to redirect_to(new_user_session_path)
+        end
+      end
+    end
+
+    context 'when the personal snippet is public' do
+      let(:personal_snippet) { create(:personal_snippet, :public, author: user) }
+
+      context 'when signed in' do
+        before do
+          sign_in(user)
+        end
+
+        it 'renders the snippet' do
+          get :show, id: personal_snippet.to_param
+
+          expect(assigns(:snippet)).to eq(personal_snippet)
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context 'when not signed in' do
+        it 'renders the snippet' do
+          get :show, id: personal_snippet.to_param
+
+          expect(assigns(:snippet)).to eq(personal_snippet)
+          expect(response.status).to eq(200)
+        end
+      end
+    end
+
+    context 'when the personal snippet does not exist' do
+      context 'when signed in' do
+        before do
+          sign_in(user)
+        end
+
+        it 'responds with status 404' do
+          get :show, id: 'doesntexist'
+
+          expect(response.status).to eq(404)
+        end
+      end
+
+      context 'when not signed in' do
+        it 'responds with status 404' do
+          get :show, id: 'doesntexist'
+
+          expect(response.status).to eq(404)
+        end
+      end
+    end
+  end
+end

spec/factories.rb

@@ -165,6 +165,18 @@ FactoryGirl.define do
     title
     content
     file_name
+
+    trait :public do
+      visibility_level Gitlab::VisibilityLevel::PUBLIC
+    end
+
+    trait :internal do
+      visibility_level Gitlab::VisibilityLevel::INTERNAL
+    end
+
+    trait :private do
+      visibility_level Gitlab::VisibilityLevel::PRIVATE
+    end
   end
   factory :snippet do