[FEATURE] Mask sensitive run arguments #1169
Conversation
b819e27 to
cc1e7c8
Compare
There was a problem hiding this comment.
Hi, thanks for contributing to the gem! I have one comment about where the argument filtering should live, but otherwise I think this feature makes sense, and I like that we're leaning on the AS Parameter Filter.
cc @Shopify/rails if anyone else would like to take a look at this.
| # Mask sensitive information from the arguments of a task. | ||
| # | ||
| # @param run [Run] the Run that contains the arguments to be masked. | ||
| def mask_arguments(run) |
There was a problem hiding this comment.
What do you think about having the Run handle this instead? That would allow us to memoize the parameter filter.
module MaintenanceTasks
class Run < ApplicationRecord
...
def masked_arguments
argument_filter.filter(arguments)
end
private
def argument_filter
@argument_filter ||= ParameterFilter.new(Rails.application.config.filter_parameters + task.masked_attributes)
endThere was a problem hiding this comment.
Thanks a lot @adrianna-chang-shopify for the feedback! I've moved the method from the helper to the Run model as requested.
app/models/maintenance_tasks/task.rb
Outdated
| @@ -28,6 +28,11 @@ class NotFoundError < NameError; end | |||
| # @api private | |||
| class_attribute :collection_builder_strategy, default: NullCollectionBuilder.new | |||
|
|
|||
| # the sensitive attributes that will be filtered when fetching a run. | |||
There was a problem hiding this comment.
| # the sensitive attributes that will be filtered when fetching a run. | |
| # The sensitive attributes that will be filtered when fetching a run. |
README.md
Outdated
| end | ||
| ``` | ||
|
|
||
| If you have any filtered parameters in the global rails parameter filter, they will be |
There was a problem hiding this comment.
What do you think about linking to the docs here?
There was a problem hiding this comment.
Great idea, thanks for flagging it! I added it in the latest commit
There was a problem hiding this comment.
Thank you! Proposed feature makes total sense and implementation looks clean 👌
I'll let @adrianna-chang-shopify to review and we can most likely merge it!
…ents' into feature/mask-sensitive-run-arguments
|
Hey Shopifolk! @adrianna-chang-shopify I have tried to squash the commits as you requested, but I ended up having a strange commit history, but it it squashed anyways. |
In some cases, maintenance tasks require the use of sensitive data in their arguments; however, I noticed that there should be a protection layer to hide these sensitive data from the UI once the task is submitted to run.
This PR introduces a new class method in the maintenance tasks to mask any sensitive attributes and hide them in the arguments list in the UI.
[TBD] The PR also assumes that the Rails global parameter filters contain sensitive keys, which should be automatically taken into account, and redacted globally across maintenance tasks.
The final result should look like the following screenshot:
