Gotcha with guard clause in overridden methods
The proper way of handling anonymous rules should be by overriding #anonymous_rules
in BasePolicy
subclasses. The default for #anonymous_rules
is #rules
.
As mentioned in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/822#note_19898741, an early return
does not return from the "overrider" method (be it via inheritance or via prepend
).
For instance:
# app/policies/group_policy.rb
class GroupMemberPolicy < BasePolicy
def rules
return unless @user
target_user = @subject.user
group = @subject.group
return if group.last_owner?(target_user)
can_manage = Ability.allowed?(@user, :admin_group_member, group)
if can_manage
can! :update_group_member
can! :destroy_group_member
elsif @user == target_user
can! :destroy_group_member
end
end
end
# app/policies/ee/group_policy.rb
module EE
module GroupPolicy
def rules
raise NotImplementedError unless defined?(super)
super
owner = @user.admin? || @subject.has_owner?(@user)
master = owner || @subject.has_master?(@user)
if @subject.ldap_synced?
cannot! :admin_group_member
can! :override_group_member if master
end
end
end
end
=> NoMethodError - undefined method `admin?' for nil:NilClass:
app/policies/ee/group_policy.rb:8:in `rules
So there's two thing here:
-
Audit our current usage of prepend and make sure we don't have code that don't behave as we'd expect
-
When guard clauses are used in an overridden method, another strategy (documented by https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/963/diffs) should be used:
create a "hook" method that is empty in CE, and with the EE-specific implementation in EE