forem icon indicating copy to clipboard operation
forem copied to clipboard
verified publisher icon forem

Analysis and remediation [likely] required for creating a Role system

Open jeremyf opened this issue 4 years ago • 5 comments

Introduction

One of the refactoring chores necessary for a role system is to eschew the usage of method calls such as current_user.admin? in the view (or controller). These method calls are short-hand for implicit understandings of the code base. It's easy to fall into the habit of writing code that looks like the following:

if current_user.has_role?(:editor)
  # render edit button
end

The above is a quick shortcut that sits as a proxy for the more robust implementation:

if can?(agent: current_user, action: edit, subject: @article)
  # render edit button
end

What does remediation look like? It means assessing all of the places in which we ask something of the user. In the Places in Code to Explore and Remediate, I've identified candidates for consideration.

Places in Code to Explore and Remediate

app/controllers

app/helpers

app/models

app/policies

app/services

app/views

References

For Forem core members see the following posts:

Generating the list

I wrote the following Ruby script to populate the Places in Code to Explore and Remediate:

app_dirs = `ls #{ENV['HOME']}/git/forem/app`.split("\n")
app_dirs.each do |dir|
  results = `cd #{ENV['HOME']}/git/forem;  rg "user\\.\\w+\\?(\\([^\\)]\+\\))?" app/#{dir} --only-matching --line-number | awk -F":" '{ print "- [ ] [" $1 ":" $2 "](https://github.com/forem/forem/blob/50a73e035e2278bd0d52c2ab377822042028b3e3/" $1 "#L" $2 ") calls \`" $3 $4"\`"}'`

  next if results.size.zero?
  puts "### app/#{dir}\n\n"
  puts results
  puts "\n\n"
end

This used ripgrep (e.g., rg) and awk

jeremyf avatar Dec 01 '21 15:12 jeremyf

Thanks for the issue, we will take it into consideration! Our team of engineers is busy working on many types of features, please give us time to get back to you.

Feature requests that require more discussion may be closed. Read more about our feature request process on forem.dev.

To our amazing contributors: issues labeled type: bug are always up for grabs, but for feature requests, please wait until we add a ready for dev before starting to work on it.

To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem/oss. The OSS Community Manager or the engineers on OSS rotation will follow up.

For full info on how to contribute, please check out our contributors guide.

github-actions[bot] avatar Dec 01 '21 15:12 github-actions[bot]

So are you going to replace the use of current_user.posseses_or_can_do(some_role_or_ability)? with this:

if can?(agent: current_user, action: edit, subject: @article)
  # render edit button
end

that part wasn't as clear to me

msarit avatar Dec 02 '21 15:12 msarit

I don't yet know what I'm going to replace this with. I know we use Pundit, so there's a consideration of syntax.

jeremyf avatar Dec 03 '21 04:12 jeremyf

I know we use Pundit, so there's a consideration of syntax.

I actually had typed up a comment to this effect yesterday, then decided to not post it because I thought it was too early in the process to discuss on that level of detail yet.

citizen428 avatar Dec 03 '21 06:12 citizen428

@citizen428 I got an email and was like "Yes! Remember to account for the existing system." My language comes from long time experience in CanCanCan, but I'm framing lots of things based on my work on robust roles/permission system (https://github.com/ndlib/sipity/blob/master/app/services/sipity/services/authorization_layer.rb)

jeremyf avatar Dec 03 '21 16:12 jeremyf