WIP: Add AttributesPermitter to Import/Export for allowlist approach instead of denylist
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:
- Model attributes from the list of
included_attributes
- Model relations from relations
tree
- 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
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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