policies: fixed destroy for repositories/tags

Repositories and tags that belonged to a global namespace was not being
able to be deleted. The regression was introduced in the 2.4 release.

Signed-off-by: Vítor Avelino <[email protected]>

app/policies/namespace_policy.rb

@@ -55,10 +55,8 @@ def create?
   def destroy?
     raise Pundit::NotAuthorizedError, "must be logged in" unless user
 
-    is_owner               = @namespace.team.owners.exists?(user.id)
-    can_contributor_delete = APP_CONFIG["delete"]["contributors"] &&
-                             @namespace.team.contributors.exists?(user.id)
-    delete_enabled? && (@user.admin? || is_owner || can_contributor_delete)
+    can_contributor_delete = APP_CONFIG["delete"]["contributors"] && contributor?
+    delete_enabled? && (@user.admin? || owner? || can_contributor_delete)
   end
 
   def update?

app/policies/repository_policy.rb

@@ -1,25 +1,31 @@
 # frozen_string_literal: true
 
 class RepositoryPolicy
-  attr_reader :user, :repository
+  attr_reader :user, :repository, :namespace
 
   def initialize(user, repository)
     @user = user
     @repository = repository
+    @namespace = repository.namespace
   end
 
   def show?
-    return @repository.namespace.visibility_public? unless @user
+    return namespace.visibility_public? unless @user
 
     @user.admin? ||
-      @repository.namespace.visibility_public? ||
-      @repository.namespace.visibility_protected? ||
-      @repository.namespace.team.users.exists?(user.id)
+      namespace.visibility_public? ||
+      namespace.visibility_protected? ||
+      namespace.team.users.exists?(user.id)
   end
 
   # Returns true if the repository can be destroyed.
   def destroy?
-    NamespacePolicy.new(@user, @repository.namespace).destroy?
+    raise Pundit::NotAuthorizedError, "must be logged in" unless user
+
+    is_owner               = namespace.team.owners.exists?(user.id)
+    is_contributor         = namespace.team.contributors.exists?(user.id)
+    can_contributor_delete = APP_CONFIG["delete"]["contributors"] && is_contributor
+    delete_enabled? && (@user.admin? || is_owner || can_contributor_delete)
   end
 
   class Scope
@@ -52,4 +58,11 @@ def resolve
       end
     end
   end
+
+  protected
+
+  # Returns true if delete is enabled
+  def delete_enabled?
+    APP_CONFIG.enabled?("delete")
+  end
 end

spec/policies/repository_policy_spec.rb

@@ -73,7 +73,10 @@
   permissions :destroy? do
     before do
       namespace = create(:namespace, team: team2, registry: registry)
+      global_team = registry.global_namespace.team
+      global_namespace = create(:namespace, team: global_team, registry: registry)
       @repository = create(:repository, namespace: namespace)
+      @global_repository = create(:repository, namespace: global_namespace)
     end
 
     context "delete disabled" do
@@ -84,24 +87,28 @@
       it "denies access to admin" do
         admin = create(:admin)
         expect(subject).not_to permit(admin, @repository)
+        expect(subject).not_to permit(admin, @global_repository)
       end
 
       it "denies access to owner" do
         owner = create(:user)
         TeamUser.create(team: team2, user: owner, role: TeamUser.roles["owner"])
 
         expect(subject).not_to permit(owner, @repository)
+        expect(subject).not_to permit(owner, @global_repository)
       end
 
       it "denies access to contributor" do
         contributor = create(:user)
         TeamUser.create(team: team2, user: contributor, role: TeamUser.roles["contributor"])
 
         expect(subject).not_to permit(contributor, @repository)
+        expect(subject).not_to permit(contributor, @global_repository)
       end
 
       it "denies access to non-member" do
         expect(subject).not_to permit(user, @repository)
+        expect(subject).not_to permit(user, @global_repository)
       end
     end
 
@@ -113,24 +120,32 @@
       it "grants access to admin" do
         admin = create(:admin)
         expect(subject).to permit(admin, @repository)
+        expect(subject).to permit(admin, @global_repository)
       end
 
       it "grants access to owner" do
         owner = create(:user)
+        global_team = @global_repository.namespace.team
         TeamUser.create(team: team2, user: owner, role: TeamUser.roles["owner"])
+        TeamUser.create(team: global_team, user: owner, role: TeamUser.roles["owner"])
 
         expect(subject).to permit(owner, @repository)
+        expect(subject).to permit(owner, @global_repository)
       end
 
       it "denies access to contributor" do
         contributor = create(:user)
+        global_team = @global_repository.namespace.team
         TeamUser.create(team: team2, user: contributor, role: TeamUser.roles["contributor"])
+        TeamUser.create(team: global_team, user: contributor, role: TeamUser.roles["contributor"])
 
         expect(subject).not_to permit(contributor, @repository)
+        expect(subject).not_to permit(contributor, @global_repository)
       end
 
       it "denies access to non-member" do
         expect(subject).not_to permit(user, @repository)
+        expect(subject).not_to permit(user, @global_repository)
       end
     end
 
@@ -144,9 +159,12 @@
 
       it "grants access to contributor" do
         contributor = create(:user)
+        global_team = @global_repository.namespace.team
         TeamUser.create(team: team2, user: contributor, role: TeamUser.roles["contributor"])
+        TeamUser.create(team: global_team, user: contributor, role: TeamUser.roles["contributor"])
 
         expect(subject).to permit(contributor, @repository)
+        expect(subject).to permit(contributor, @global_repository)
       end
     end
   end