Private/Public filters

[antulik]

I'd like to open a discussion about separating filters into public and private, or external/internal.

Background and defaults

To simplify interaction object testing we expose defaults and class dependences as interaction inputs. e.g.

class SendSms < ActiveInteraction::Base
  object :sms_service, default -> { SmsService.new }
  boolean :require_verification, default: false
  object :user

  string :text
  string :number
  
  def execute
    #...
  end
end

SendSms.run(text: 'Hi', number: '123', user: current_user)

As mentioned, that's great for testing and reusability of the interaction class. Two very important features.

Controller

Almost all our use cases we have include parameters from the request and additional internal data. It looks like this:

outcome = SendSms.run(params.merge(user: current_user))

Problem

Now the problem that require_verification input is vulnerable and not secure. User can pass any data and overwrite its default.

Proposal

I'd like to have a clear distinction between external (unsanitized) and internal inputs. So if user passes random data, form only accepts inputs that marked as external.

Interface could look like this:

class SendSms < ActiveInteraction::Base
  with_options internal: true do
    object :sms_service, default -> { SmsService.new }
    boolean :require_verification, default: true
    object :user
  end

  string :text
  string :number
  
  def execute
    #...
  end
end

params = { text: 'Hi', number: '123', require_verification: false }
outcome = SendSms.set(user: current_user).run(params)
outcome.require_verification 
# => true   # value comes from defaults and not from params hash

Why to care?

That's already a security vulnerability if you use inputs and don't overwrite them. With the new record filter it becomes even more risky.

[ngan]

I feel like this is mixing in Rails' controller responsibilities into AI. I mean, isn't Strong Parameters in charge of this kind of security? And the whole reason why attr_protected was removed was because Rails wanted to push the knowledge and responsibility of param filtering to the controller level--which allows models to not care about various permissions (to set certain attributes) from the potentially many controllers it is used in. In your example, what if I had an Admin controller that allows require_verification to be passed in; I now have to know that my interaction considers the require_verification attribute as "private" and have to move it over to the set call.

All that said, I do think the problem you brought up is a problem. Perhaps AI can work with ActionController::Parameter...

params = ActionController::Parameters
SendSms.run(params)

AI can check if a plain Hash is passed in (in which it would behave as normal) or if an ActionController::Parameters object was passed in. If so, it would check if it's permitted?

[AaronLasseigne]

I agree with what @ngan said here.

As for ActionController::Parameters, we already accept that as the input to an interaction but we don't currently check for whether permitted? has been set. I'm not super familiar with permitted? but isn't that going to just be set to true when you use permit?

[antulik]

@ngan My hesitation to use Rails Strong Params is that it requires duplication of all form's (interaction) inputs. With interaction it's pretty much clear what it needs and what params are safe.

But you've raised a good point about roles. In the case where you need different params I'd say it's better to create another form and compose it. It's more clear flow as there is only one place where the params are whitelisted (form).

Otherwise with Form+StrongParams you need to whitelist params in two places (form+controller) and keep them in sync.

After couple of years of using this gem i'm yet to see a use case where I could reuse the the interaction for user and admin but with different inputs. I'm sure it exists, but optimising for 1% use case makes it worse for other 99% use cases.

[skunkworker]

@antulik I agree about the security risk of record.

At first I really liked the record filter but after reading this I've gone back and changed them to be object instead and pushed the validation of the scope on the object outside of the active interaction object.

Not having a scope that can be passed puts too much power inside of the interaction, instead of having a find_foo! method in a controller that can restrict the object, once inside you could potentially iterate through object ids and try every single combination.

If a or scope from an object could be passed to record that could help, but without it, record is too vague at the current moment.

[AaronLasseigne]

I see what you mean. I'm thinking it might make sense to allow the finder option to take a proc. In the proc you could use a scope to limit things. Still a thought in progress though.

[blackst0ne]

Any update here?
Or any advice how to split internal and external params without duplicating code, e.g. using strong params?

[antulik]

@blackst0ne I've wrote the extension which integrates strong params. https://github.com/antulik/active_interaction-extras#strongparams

We are using it in production, but I would probably change the interface if I was to write it today.