<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=195975) </details> <!--IssueSummary end--> ## 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 https://gitlab.com/gitlab-org/gitlab/issues/39087 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. 1. 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](https://docs.gitlab.com/ee/development/policies.html#specifying-policy-class). 1. Get it right. For instance, calling `demodulized` is not obvious and not done in all examples below. ## Risks Nothing I can foresee. ## Involved components ```ruby # 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 ``` ```ruby # app/models/snippet.rb 236 def to_ability_name 237 'snippet' 238 end ``` ```ruby # app/models/note.rb 369 def noteable_ability_name 370 for_snippet? ? 'snippet' : noteable_type.demodulize.underscore 371 end ``` ```ruby # 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 ``` ```ruby # 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 ``` ```ruby # 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 ``` ```ruby # 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 https://gitlab.com/gitlab-org/gitlab/merge_requests/22500 ## 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.