Consider adding additional ability/security checks to another layer of abstraction
We recently had some security issues related to code that we forgot to add additional can?
check in services, controller etc.
Recent issues: https://gitlab.com/gitlab-org/gitlab-ce/issues/15330, https://gitlab.com/gitlab-org/gitlab-ce/issues/14899
I think that there were quite a few security issues in the past also. I remember @DouweM mentioned it in one of the merge requests of mine.
I was thinking how can we prevent such problems in the future, and I would like to ask people, about opinion on adding additional permissions-checks in another layer of abstractions. This is quite difficult problem to solve, but we can discuss things like doing some Aspect Oriented Programming here, that would add pointcuts to models and prevent unauthorized saves, finds, updates etc. Other ideas are also welcome!
So we can think about code like:
class Issue
include PermissionsAspects
# [...]
end
in app/permissions/issue.rb
:
class Permissions::Issue < Aspects::Permissions
include Ability
def initialize(user, project, issue)
@user, @project = user, project
end
permit :find do
check { @user.can?(:read_issue, @project)) }
check { } # additional checks
except { } # exceptions
end
permit :save do
check { @project.public? }
end
permit :update do
end
end
Code like this can add pointcuts to ActiveRecord
model around methods that save object, update object, find object, etc.
Of course this is merely an idea I would like to ask other people about. It would be also awesome to discuss other ideas. I think that this may be good approach to think about solving similar security problems, like we recently had, near a place it originates from, to prevent this from happening in the future.
What do you think @stanhu @DouweM @dzaporozhets @rspeicher @rymai @pcarranza @tmaczukin?