Skip to content

Centralize model type to ability name mapping

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

Summary

There are two distinct parts to this refactor, both closely related and probably easier to fix together.

  1. The logic to check abilities on Snippet and ProjectSnippet objects is duplicated across multiple locations. This should be centralised. Now that *_project_snippet and *_personal_snippet abilities are merged to *_snippet after closing #39087 (closed) we can no longer straight map to ability names via object.demodulize.underscore.

  2. There is an implied ontology within the ability names that starts with an action such as create or read and ends with the subject type such as user or project. For example, create_project or read_project. "Subject type" is very closely related to the name of the policy, though not exactly in cases of STI and modules. Throughout the code base there is the pattern of translating from an object type to some derivative of it's demodulized underscored string variant to produce the subject type. For instance going from Ci::PipelineSchedule to 'pipeline_schedule'.

Improvements

  1. Centralize the mapping logic in one place.
  2. Formalize the action_subject convention. The naming is only loosely defined by policy naming convention, as evidenced by the code examples below like noteable_type.demodulize.underscore. Worth noting is there exists a formal definition of going from Model to Policy name via .declarative_policy_class.
  3. Get it right. For instance, calling demodulized is not obvious and not done in all examples below.

Risks

Nothing I can foresee.

Involved components

# lib/api/helpers/notes_helpers.rb
74       def noteable_read_ability_name(noteable)
75         "read_#{ability_name(noteable)}".to_sym
76       end
77
78       def ability_name(noteable)
79         if noteable.respond_to?(:to_ability_name)
80           noteable.to_ability_name
81         else
82           noteable.class.to_s.underscore
83         end
84       end
# app/models/snippet.rb
236   def to_ability_name
237     'snippet'
238   end
# app/models/note.rb
369   def noteable_ability_name
370     for_snippet? ? 'snippet' : noteable_type.demodulize.underscore
371   end
# lib/api/award_emoji.rb
126       def read_ability(awardable)
127         case awardable
128         when Note
129           read_ability(awardable.noteable)
130         when Snippet, ProjectSnippet
131           :read_snippet
132         else
133           :"read_#{awardable.class.to_s.underscore}"
134         end
135       end
# app/controllers/uploads_controller.rb
 37   def authorize_access!
 38     return unless model
 39
 40     authorized =
 41       case model
 42       when Note
 43         can?(current_user, :read_project, model.project)
 44       when User
 45         # We validate the current user has enough (writing)
 46         # access to itself when a secret is given.
 47         # For instance, user avatars are readable by anyone,
 48         # while temporary, user snippet uploads are not.
 49         !secret? || can?(current_user, :update_user, model)
 50       when Appearance
 51         true
 52       else
 53         permission = "read_#{model.class.underscore}".to_sym
 54
 55         can?(current_user, permission, model)
 56       end
 57
 58     render_unauthorized unless authorized
 59   end
# app/services/notification_service.rb
291   def send_new_note_notifications(note)
292     notify_method = "note_#{note.noteable_ability_name}_email".to_sym
293
294     recipients = NotificationRecipientService.build_new_note_recipients(note)
295     recipients.each do |recipient|
296       mailer.send(notify_method, recipient.user.id, note.id, recipient.reason).deliver_later
297     end
298   end
# app/policies/note_policy.rb
 12   condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") }

There could be more. The above is what I came across while working on !22500 (merged)

Optional: Missing test coverage

There will need to be specs created for the method that returns the ability subject type. This will need to test for each subject/policy type. Something like it_behaves_like 'model with policy', 'pipeline_schedule' is what comes to mind.

Edited by 🤖 GitLab Bot 🤖