Unify authorization pattern in virtual registry GraphQL resolvers

Summary

Currently, virtual registry GraphQL resolvers perform authorization checks in the resolve method instead of using the standard GraphQL authorized? method. We should evaluate whether this pattern should be refactored or if it's semantically correct.

Current Pattern

Most virtual registry resolvers use this pattern:

def resolve(**args)
  return unless ::VirtualRegistries::Packages::Maven.virtual_registry_available?(object, current_user)
  
  # resolver logic
end

Examples:

❓ Question

virtual_registry_available? checks multiple conditions:

group.dependency_proxy_feature_available? &&
  Feature.enabled?(:maven_virtual_registry, group) &&
  group.licensed_feature_available?(:packages_virtual_registry) &&
  Ability.allowed?(current_user, permission, group.virtual_registry_policy_subject) &&
  VirtualRegistries::Setting.find_for_group(group).enabled

Only Ability.allowed? is actually authorization. The rest are feature availability checks.

However, having permission check Ability.allowed? inside resolve method breaks the convention of GraphQL usage, it's raised in the MR discussion, authorize for the class or authorized? method is the recommend way to do permission check which we don't follow now.

Current solution

In the MR, we made a compromise to align with GraphQL best practices.

We added the authorized? method in the resolver to explicitly show authorization checks (code):

  def authorized?(**_args)
    current_user&.can?(:admin_virtual_registry, group.virtual_registry_policy_subject)
  end

Problem

This creates duplication, we do permission check twice: once in authorized? and again in virtual_registry_available?. Goals:

  • Follow GraphQL best practices using authorize or authorized? for authorization
  • Establish a unified authorization pattern across the virtual registries domain

Solution

Continue with the current approach from the MR. We added the authorized? method in the resolver (code):

  def authorized?(**_args)
    current_user&.can?(:admin_virtual_registry, group.virtual_registry_policy_subject)
  end

We need to apply it to all the resolvers in virtual registries scope.

Considered Approaches

Option 1: Split Authorization from Availability

Extract Ability.allowed? from VirtualRegistries::Packages::Maven.virtual_registry_available? since virtual_registry_available? should be a availability check but not permission check.

def authorized?(**args)
  Ability.allowed?(current_user, :admin_virtual_registry, object.virtual_registry_policy_subject)
end

def resolve(**args)
  # Check feature availability, `Ability.allowed?` part should be removed 
  return unless VirtualRegistries::Packages::Maven.virtual_registry_available?(..)
  
  # business logic
end

This option will follow the recommend practice, but it also bring some concerns:

  • Requires refactoring virtual_registry_available? which is used in many places (REST APIs, controllers, other GraphQL resolvers)
  • Every caller would need to add their own permission check, leading to code duplication

Option 2: Put virtual_registry_available? in authorized? method

Follows GraphQL best practices by using the authorized? method for all availability checks. GraphQL Ruby automatically handles authorization failures by returning nil instead of raising errors. However, this approach puts both authorization checks (permissions) and feature availability checks (feature flags, license, settings) in the same method, which may be semantically unclear.

def authorized?(**args)
  VirtualRegistries::Packages::Maven.virtual_registry_available?
end

def resolve(**args)  
  # business logic
end

References

Edited by 🤖 GitLab Bot 🤖