Skip to content

WIP: Add AttributesPermitter to Import/Export for allowlist approach instead of denylist

George Koltsov requested to merge georgekoltsov/add-attribute-permitter into master

What does this MR do?

This MR introduces a different way we handle unintended/undesired Imported/Exported model attributes.

Instead of specifying a list of attributes that are going to be excluded from Import/Export, have an explicit list of allowed included attributes. This way there is less of a risk having unintended attributes being exported/imported and slipped into the system, since current approach allows any attribute (that is not defined in the list of excluded attributes) to be injected into the import and potentially achieve unexpected behaviours.

For export, we already have a perfect tool for the job, which is included_attributes list in import_export.yml file https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/import_export/project/import_export.yml#L105-113 Specified attributes for a model under this section will only include those attributes. ActiveModel JSON serializer is used to achieve that https://api.rubyonrails.org/classes/ActiveModel/Serializers/JSON.html

For import, I have added a new AttributesPermitter class which builds allowed attributes for each model based on import_export.yml contents and includes:

  1. Model attributes from the list of included_attributes
  2. Model relations from relations tree
  3. Model methods from methods list
=> {:project=>
  [:labels,
   :milestones,
   :issues,
   :snippets,
   ...],
:ci_pipelines=>[:notes, :stages, :external_pull_request, :merge_request, :notes],
 :merge_request=>
  [:allow_maintainer_to_push,
   :approvals_before_merge,
   :created_at,
   :description,
   :discussion_locked,
   :iid,
   :in_progress_merge_commit_sha,
   :last_edited_at,
   :lock_version,
   :merge_commit_sha,
   :merge_error,
   :merge_params,
   :merge_status,
   :merge_when_pipeline_succeeds,
...

The only allowed keys in the relation hash are going to be the ones defined in the above list. Any other attributes manually added to the relation hash in project.json are going to be removed.

Introducing this approach does mean that we will have to maintain an explicit list of allowed attributes for every model. As a test, I implemented 2 models issues and merge requests but if we decide to go ahead with this approach then we will have to add every model and its attributes to the list of included_attributes. In this case we can remove excluded_attributes list, as there won't be a need in it anymore.

Our current approach is on Import AttributeCleaner cleans up everything that ends on _id, _ids (and more) as well as excluded model's attributes defined in https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/import_export/project/import_export.yml#L116. This solution is prone to manual injection of additional attributes that are not going to be excluded, which can cause unexpected behaviour.

One potential solution is instead of replacing one approach for another, we can keep both. And have a relation hash that gets cleaned up by AttributeCleaner first, then filtered by AttributesPermitter. This does feel unnecessary though, so perhaps this is a good option for a transition period, if this approach is the one we are going to take.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by 🤖 GitLab Bot 🤖

Merge request reports