Deny standalone service template ordering when product setting is enabled #476

eclarizio · 2018-09-24T17:40:20Z

The issue this PR fixes is that currently, you can simply POST to the service_template order endpoint with whatever data you want (ideally, there is sort of a flow, i.e. you request the dialog, you may request any dialog field refreshes, and then you can POST to the service_template order endpoint with the data you've gathered from the previous requests). With any random data, it won't really work, but for example if a dropdown only contains the options "A" or "B", but the API request is submitted with "C", it is accepted.

This change, along with a product setting called allow_api_service_ordering, that the users must set to false first (as it is defaulted to true via this PR), should ensure that only the API calls that come from the UIs will be accepted. Otherwise everything will behave as it does now. This is mostly just a quick fix solution until we can potentially implement RBAC for API users, or add code to the model to validate the values against expected values, but those options are both a lot more involved.

/cc @tinaafitz

https://bugzilla.redhat.com/show_bug.cgi?id=1632416

eclarizio · 2018-09-26T17:40:47Z

@miq-bot add_reviewer @bdunne

@bdunne Can you take a look for me, please? And/or suggest anyone else that can take a look?

bdunne

In general, this feels like a model validation issue more than an api problem

bdunne · 2018-09-26T19:27:46Z

app/controllers/api/mixins/service_templates.rb

@@ -17,6 +19,14 @@ def order_service_template(id, data, scheduled_time = nil)
      def service_template_ident(st)
        "Service Template id:#{st.id} name:'#{st.name}'"
      end
+
+      def api_request_allowed?
+        standalone_api_request? ? !Settings.product.deny_api_service_ordering : true


Can the Setting have a positive name?

The only issue I have with changing it to a positive name is then the users who want it to work the way it works after this PR would be to set the value to "false", which feels weird but I don't know if we have a precedent for this or not.

No, but I think the default should be in here anyway.

bdunne · 2018-09-26T19:31:08Z

spec/requests/service_templates_spec.rb

+      let(:product_settings) { double(:deny_api_service_ordering => true) }
+
+      before do
+        allow(Settings).to receive(:product).and_return(product_settings)


Prefer stub_settings_merge

bdunne · 2018-09-26T19:35:33Z

app/controllers/api/mixins/service_templates.rb

+      end
+
+      def standalone_api_request?
+        request.authorization.present? && request.authorization.to_s.split(" ", 2).first.downcase == "basic"


request.authorization.try(:starts_with?, "basic")

Also, this will break if we allow other authentication types to the api

Agreed, but do you know of another way to distinguish standalone API calls versus API calls made from the UI?

I can't think of anything at the moment

maybe we can leverage the token manager namespace, api, ws, (might need to add param or separate api/ui as 2 namespaces).

Also, I pulled the "basic" check from here: https://github.com/rails/rails/blob/fc5dd0b85189811062c85520fd70de8389b55aeb/actionpack/lib/action_controller/metal/http_authentication.rb#L104.

I suppose we could do .try(:downcase).try(:starts_with?, "basic")? Not sure how else to handle the fact that it could be "Basic", "BASIC", or any other various capitalizations.

eclarizio · 2018-09-26T20:11:28Z

@bdunne I agree, but fixing the validation to take care of ensuring that the values passed in through the API versus actual acceptable values is much more involved because of dynamic dialogs and how we don't want to call into automate when we shouldn't have to (and the potential sequencing of those calls depending on which fields are dependent on which for refreshes). If the dialog has some expensive automate calls, submitting will take way longer than it should since for the majority of the cases the API call will have proper values already.

A PR could build on this one so that instead of just flat out denying the request a product setting is no longer needed and it delegates to the model to verify that the values passed in are acceptable values.

eclarizio · 2018-09-27T18:12:49Z

Not sure what labels to put on this now that we've transitioned into Hammer.

@miq-bot add_label bug

carbonin

I see where @bdunne is coming from and would agree in an ideal world, but if validating the incoming values properly means double automate calls then I think this is a better stop-gap solution.

Generally though, I would like to see something less fragile than checking the auth type to determine if the request came from our UI or not. Given that this bug is of medium priority and not a blocker, I think it would be good to do this for the first pass. Maybe we could implement a special header value to denote API calls originating in the UI and make this information available across the API for all requests. Thoughts @bdunne @abellotti ?

carbonin · 2018-09-27T18:31:24Z

app/controllers/api/mixins/service_templates.rb

+      end
+
+      def standalone_api_request?
+        request.authorization.present? && request.authorization.try(:starts_with?, "basic")


If we're doing the .present? check, why do we need the .try? Do we expect request.authorization not to respond to starts_with??

Also, is the premise here valid @abellotti @jvlcek? Does external auth not work with the API? Or am I misunderstanding this check?

Good question about external auth, I'm not sure. I mentioned before when @bdunne suggested using the try that I (originally) pulled this line from https://github.com/rails/rails/blob/fc5dd0b85189811062c85520fd70de8389b55aeb/actionpack/lib/action_controller/metal/http_authentication.rb#L104, and when I went to change to the try I forgot to take off the .present?.

carbonin · 2018-09-27T18:35:15Z

app/controllers/api/mixins/service_templates.rb

+        standalone_api_request? ? Settings.product.allow_api_service_ordering : true
+      end
+
+      def standalone_api_request?


Can we name this better? I don't understand what it means for an API request to be "standalone", maybe reverse the logic and call it request_from_ui?

That's fair, I was struggling to come up with a good name, but reversing the logic and naming it request_from_ui? sounds better to me

eclarizio · 2018-09-27T19:09:42Z

@carbonin I agree, ultimately the auth type was the only thing I could think of to determine the difference between UI API calls and API calls originating from curl or any other client, but if there is a better way to determine it, it's easy to replace the logic here. I thought of a special header as well, but originally I heard that this was supposed to be some kind of high priority customer issue, so I wanted to get something out relatively quickly. Tina created the BZ just so we would have something to link to this PR, but I have no insight into how high the priority is now for the customer.

abellotti · 2018-09-27T19:18:13Z

I think it's best if we use/extend our UI assigned tokens for this. Since API calls coming in from the UI use their tokens, we already have that information (or easy to add) for differentiation. This way this will work for basic as well as ext-auth requests from the UI.

eclarizio · 2018-09-28T18:21:21Z

@abellotti Is this more what you had in mind? I originally started down this path but couldn't figure out how to authenticate it. Thanks for the push in the right direction with the token_get_info method.

mkanoor · 2018-09-28T18:31:56Z

@eclarizio Isn't this a validation problem, say if you had two drop down items in a service dialog
state = NY
fruit = pick one of ['apple', 'banana', 'orange']
county = pick one of Dynamic content from Automate (e.g. get list of counties based on state)

When the caller sends in
state = NY, fruit = apple, county = Ulster its valid this can be called from the UI and from a CLI making the REST API call and if we made the automate call to get the list of counties we would know that Ulster is a valid county in NY
but when some one passes in

state = NY, fruit = banana, county = bergen this is invalid because Bergen is not a county in NY. When the user is in the UI we will never present Bergen as an option since we would have made the dynamic call

If during validation of the dialog if we fetched the list of the dynamic components we could have rejected that invalid county.

I think the validation is making an assumption that if the field is a drop down the user has selected a valid item so even in the static fruit list if someone passed in kiwi we would still let it thru.

eclarizio · 2018-09-28T20:14:52Z

If during validation of the dialog if we fetched the list of the dynamic components we could have rejected that invalid county.

We could, you're right, but that (to me) is an unnecessary automate call, because we should have already done that when the user selected NY from the UI, which (correct me if you think I'm wrong here) will be a large majority of the time.

Maybe in your example case, the automate call isn't very expensive, but if the dialog has multiple expensive automate calls, I don't think the trade off of always calling automate to validate the values is worth it compared to how often it would actually be useful since again, the majority of the order submit calls will be going through the UIs. Unless someone purposely mucks with data, it is impossible to actually send through values that don't exist.

Even in the event people use the API to POST order submits, in theory they should be doing a GET request on the dialog first, then POST-ing to the refresh endpoint when their data changes so they can get the correct values on the dialog fields, and then finally POST-ing to the order submit endpoint.

mkanoor · 2018-09-28T21:28:47Z

@eclarizio
There are 2 distinct phases here

User interacting with the dialogs making selections, causing data to change, maybe some data input validation happening like checking regular expression, minimum length etc.
The user presses the order button at this point we have a hash of all the selected values, these values have to be validated before the request is created or as the first step after the request is created. @Fryguy has correctly pointed out in the past that we do some validation before the MiqRequest is created which ends up holding the API calls and causing them to hang. We should create a request then validate all the parameters and once validation is successful mark the request as "Ready" for approval or Reject it.

The UI today mostly focuses on (1) but its easy to do (2) by doing some of the same things that were done in (1). That code is already there. It just needs to be used during the ordering phase.

(2) cannot be done in Automate, we cannot have Automate load up a dialog and for each field call all the Automate Methods to create dynamic content, all of that has to be done from the dialog side since there is already code doing this validation.

Granted "Validation" might take a long time but if there is an option that says "Strict Dialog Validation" and this can be done after the MiqRequest is created it would be ideal. We wont he holding the Request API call while we do the validation.

If we dont do this validation all the REST API calls are going in without any validation and we run into the issues reported in this BZ.

miq-bot · 2018-10-15T16:43:40Z

This pull request is not mergeable. Please rebase and repush.

JPrause · 2018-10-18T13:39:08Z

@eclarizio is this will be able to be backported, can you add the hammer/yes and gaprindashvili/yes labels

JPrause · 2018-10-18T13:39:18Z

@miq-bot add_label blocker

bdunne · 2018-10-18T15:34:26Z

app/controllers/api/mixins/service_templates.rb

      def request_from_ui?
        return false if request.headers["x-auth-token"].blank?
-        token_info.present?
+        !token_info.empty?


Aren't these the same?

bdunne · 2018-10-18T15:42:47Z

app/controllers/api/mixins/service_templates.rb

@@ -15,19 +17,24 @@ def order_service_template(id, data, scheduled_time = nil)

      private

+      def service_template_ident(st)


I don't think this move should be in the diff

bdunne · 2018-10-18T15:44:33Z

spec/requests/service_catalogs_spec.rb

@@ -375,6 +375,13 @@ def sc_template_url(id, st_id = nil)
      request_headers["x-auth-token"] = test_token
    end

+    before do
+      stub_settings_merge(:product => double(:allow_api_service_ordering => true))


You shouldn't need a double here, it can be a nested hash

bdunne · 2018-10-18T15:45:49Z

spec/requests/service_templates_spec.rb

@@ -466,6 +466,15 @@

  describe "Service Templates order" do
    let(:service_template) { FactoryGirl.create(:service_template, :with_provision_resource_action_and_dialog, :orderable) }
+    let(:product_settings) { double(:allow_api_service_ordering => allow_api_service_ordering) }


Also, it doesn't feel like this should be in a let

eclarizio · 2018-10-18T16:21:37Z

@bdunne Thanks for the catches, was from the rebase and I was just concerned about fixing the conflicts, haha.

eclarizio · 2018-10-19T18:47:58Z

@miq-bot add_label gaprindashvili/yes, hammer/yes

https://bugzilla.redhat.com/show_bug.cgi?id=1632416

miq-bot · 2018-10-22T15:42:32Z

Checked commits eclarizio/manageiq-api@b46e4c0~...86d0986 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

Deny standalone service template ordering when product setting is enabled (cherry picked from commit 7343ad7) https://bugzilla.redhat.com/show_bug.cgi?id=1632416

simaishi · 2018-10-22T17:29:49Z

Hammer backport details:

$ git log -1
commit 55732b3d20dece9cf4743fa0cd850af5c0c11e82
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Oct 22 11:49:56 2018 -0400

    Merge pull request #476 from eclarizio/dialog_ordering_security_issue
    
    Deny standalone service template ordering when product setting is enabled
    
    (cherry picked from commit 7343ad7cad22f24639a23ff3a9d6c5182d64172d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1632416

Deny standalone service template ordering when product setting is enabled (cherry picked from commit 7343ad7) https://bugzilla.redhat.com/show_bug.cgi?id=1646435

simaishi · 2018-11-05T15:25:23Z

Gaprindashvili backport details:

$ git log -1
commit 068184ec96943dcff4cab92dff2a4c60e2bc10fe
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Oct 22 11:49:56 2018 -0400

    Merge pull request #476 from eclarizio/dialog_ordering_security_issue
    
    Deny standalone service template ordering when product setting is enabled
    
    (cherry picked from commit 7343ad7cad22f24639a23ff3a9d6c5182d64172d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1646435

eclarizio force-pushed the dialog_ordering_security_issue branch from bce5997 to 32b7f04 Compare September 24, 2018 18:17

miq-bot requested a review from bdunne September 26, 2018 17:41

bdunne requested changes Sep 26, 2018

View reviewed changes

eclarizio force-pushed the dialog_ordering_security_issue branch from 32b7f04 to 663ecc8 Compare September 26, 2018 20:01

eclarizio force-pushed the dialog_ordering_security_issue branch from 663ecc8 to 4218444 Compare September 27, 2018 18:09

eclarizio mentioned this pull request Sep 27, 2018

Add product setting default for allowing API service ordering ManageIQ/manageiq#18029

Merged

miq-bot added the bug label Sep 27, 2018

carbonin reviewed Sep 27, 2018

View reviewed changes

eclarizio force-pushed the dialog_ordering_security_issue branch 2 times, most recently from 08779c9 to f6c1828 Compare September 27, 2018 19:10

eclarizio force-pushed the dialog_ordering_security_issue branch 2 times, most recently from 09185ff to 8e470f1 Compare September 27, 2018 20:15

eclarizio mentioned this pull request Oct 11, 2018

Add flag to initialize field default values #485

Merged

miq-bot added the unmergeable label Oct 15, 2018

miq-bot added the blocker label Oct 18, 2018

eclarizio force-pushed the dialog_ordering_security_issue branch 2 times, most recently from be62161 to 3045610 Compare October 18, 2018 15:02

bdunne reviewed Oct 18, 2018

View reviewed changes

eclarizio force-pushed the dialog_ordering_security_issue branch from 3045610 to 417873f Compare October 18, 2018 16:21

miq-bot removed the unmergeable label Oct 18, 2018

bdunne approved these changes Oct 18, 2018

View reviewed changes

bdunne closed this Oct 18, 2018

bdunne reopened this Oct 18, 2018

miq-bot added gaprindashvili/yes labels Oct 19, 2018

Only allow non-UI service ordering when the product setting is enabled

b46e4c0

https://bugzilla.redhat.com/show_bug.cgi?id=1632416

eclarizio force-pushed the dialog_ordering_security_issue branch from 417873f to 364b585 Compare October 22, 2018 15:26

Validate ui request via auth token instead of auth strategy

86d0986

https://bugzilla.redhat.com/show_bug.cgi?id=1632416

eclarizio force-pushed the dialog_ordering_security_issue branch from 364b585 to 86d0986 Compare October 22, 2018 15:40

bdunne merged commit 7343ad7 into ManageIQ:master Oct 22, 2018

bdunne added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 22, 2018

bdunne self-assigned this Oct 22, 2018

simaishi added hammer/backported and removed hammer/yes labels Oct 22, 2018

AparnaKarve mentioned this pull request Oct 22, 2018

provide service_template to orderable? method #498

Merged

eclarizio mentioned this pull request Oct 30, 2018

Ensure ServiceTemplate ordering passes through the submit_workflow flag #504

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 5, 2018

		@@ -15,19 +17,24 @@ def order_service_template(id, data, scheduled_time = nil)

		private

		def service_template_ident(st)

Deny standalone service template ordering when product setting is enabled #476

Deny standalone service template ordering when product setting is enabled #476

Conversation

eclarizio commented Sep 24, 2018 • edited Loading

eclarizio commented Sep 26, 2018

bdunne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclarizio commented Sep 26, 2018

eclarizio commented Sep 27, 2018

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclarizio Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclarizio commented Sep 27, 2018

abellotti commented Sep 27, 2018

eclarizio commented Sep 28, 2018

mkanoor commented Sep 28, 2018

eclarizio commented Sep 28, 2018 • edited Loading

mkanoor commented Sep 28, 2018

miq-bot commented Oct 15, 2018

JPrause commented Oct 18, 2018

JPrause commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclarizio commented Oct 18, 2018

eclarizio commented Oct 19, 2018

miq-bot commented Oct 22, 2018

simaishi commented Oct 22, 2018

simaishi commented Nov 5, 2018

eclarizio commented Sep 24, 2018 •

edited

Loading

eclarizio Sep 27, 2018 •

edited

Loading

eclarizio commented Sep 28, 2018 •

edited

Loading