diff --git a/CHANGELOG b/CHANGELOG
index 0d89fca9fc1283104d07915e0726f1f7e1047da4..325a073b5c586c255c186811cc5d947bc17eb8af 100644
--- a/CHANGELOG
+++ b/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)
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index 9f9f9a92f1150439032f213787824428e8f07121..8498efc89d0e2749b279553ffc11b4ed8497fc42 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -1,6 +1,9 @@
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
diff --git a/app/models/ability.rb b/app/models/ability.rb
index b72178fa12608f84058cb356ee71d7c798e8ebc4..ee2f7b5f58bf3ebfea4fe7f92d66e75fc0799e7f 100644
--- a/app/models/ability.rb
+++ b/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
+ []
+ end
+ end
- if group && group.public_profile?
- [:read_group]
- else
- []
- 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
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..e9b823c523c0b5038843a0e9489a4f64e41f3907
--- /dev/null
+++ b/spec/controllers/snippets_controller_spec.rb
@@ -0,0 +1,118 @@
+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
diff --git a/spec/factories.rb b/spec/factories.rb
index 200f18f660d52b4b32e36c7fadff1f3702c66051..4bf93adabe27c31e5977daa3eb85726ce88e2a17 100644
--- a/spec/factories.rb
+++ b/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