Raises on INSUFFICIENT_PERMISSION error from NetSuite response #405

aom · 2018-08-16T08:30:25Z

We're handling a NetSuite installation with multiple Subsidiaries and during development we're seeing a lot of INSUFFICIENT_PERMISSION errors from Subsidiary restrictions. Especially when searching for subsidiaries from NetSuite.

In development these are easy to spot from XML responses but in production they would get lost without verbose logging. We consider these the kind of errors to raise because they won't heal themselves automatically.

Example SOAP request and response:

<env:Envelope xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:platformMsgs="urn:messages_2017_2.platform.webservices.netsuite.com" xmlns:env="http://schemas.xmlsoap.org/soap/envelope/" xmlns:platformCore="urn:core_2017_2.platform.webservices.netsuite.com" xmlns:platformCommon="urn:common_2017_2.platform.webservices.netsuite.com" xmlns:listRel="urn:relationships_2017_2.lists.webservices.netsuite.com" xmlns:tranSales="urn:sales_2017_2.transactions.webservices.netsuite.com" xmlns:tranPurch="urn:purchases_2017_2.transactions.webservices.netsuite.com" xmlns:actSched="urn:scheduling_2017_2.activities.webservices.netsuite.com" xmlns:setupCustom="urn:customization_2017_2.setup.webservices.netsuite.com" xmlns:listAcct="urn:accounting_2017_2.lists.webservices.netsuite.com" xmlns:tranBank="urn:bank_2017_2.transactions.webservices.netsuite.com" xmlns:tranCust="urn:customers_2017_2.transactions.webservices.netsuite.com" xmlns:tranEmp="urn:employees_2017_2.transactions.webservices.netsuite.com" xmlns:tranInvt="urn:inventory_2017_2.transactions.webservices.netsuite.com" xmlns:listSupport="urn:support_2017_2.lists.webservices.netsuite.com" xmlns:tranGeneral="urn:general_2017_2.transactions.webservices.netsuite.com" xmlns:commGeneral="urn:communication_2017_2.general.webservices.netsuite.com" xmlns:listMkt="urn:marketing_2017_2.lists.webservices.netsuite.com" xmlns:listWebsite="urn:website_2017_2.lists.webservices.netsuite.com" xmlns:fileCabinet="urn:filecabinet_2017_2.documents.webservices.netsuite.com" xmlns:listEmp="urn:employees_2017_2.lists.webservices.netsuite.com">
  <env:Header>
    ..
  </env:Header>
  <env:Body>
    <platformMsgs:search>
      <platformMsgs:searchRecord xsi:type="listAcct:SubsidiarySearch">
        <listAcct:basic/>
      </platformMsgs:searchRecord>
    </platformMsgs:search>
  </env:Body>
</env:Envelope>

Response

<soapenv:Envelope xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <soapenv:Header>
    <platformMsgs:documentInfo xmlns:platformMsgs="urn:messages_2017_2.platform.webservices.netsuite.com">
      <platformMsgs:nsId>..</platformMsgs:nsId>
    </platformMsgs:documentInfo>
  </soapenv:Header>
  <soapenv:Body>
    <searchResponse xmlns="urn:messages_2017_2.platform.webservices.netsuite.com">
      <platformCore:searchResult xmlns:platformCore="urn:core_2017_2.platform.webservices.netsuite.com">
        <platformCore:status isSuccess="false">
          <platformCore:statusDetail type="ERROR">
            <platformCore:code>INSUFFICIENT_PERMISSION</platformCore:code>
            <platformCore:message>Permission Violation: The subsidiary restrictions on your role prevent you from seeing this record.</platformCore:message>
          </platformCore:statusDetail>
        </platformCore:status>
      </platformCore:searchResult>
    </searchResponse>
  </soapenv:Body>
</soapenv:Envelope>

freezepl · 2019-02-04T21:34:20Z

Hey @iloveitaly , could you comment or merge this PR? I have similar issue and I think more people are affected by this..

iloveitaly · 2019-02-05T22:23:49Z

@freezepl @aom I like this idea! Going to need some more time to think on it and see if it any unintended side effects. Short on time right now!

freezepl · 2019-02-11T08:55:22Z

@iloveitaly thanks! we could add feature flag, so it would not break old implementations.

JakubKopys · 2019-02-27T12:32:40Z

@iloveitaly any updates? Would love to see this merged!

iloveitaly · 2019-02-27T23:41:18Z

@JakubKopys not yet :( Pretty busy with the day-job at the moment.

aom · 2019-03-05T13:35:34Z

Feature flag is probably a good idea as this might cause unexpected exceptions for people using this gem. I'm happy to implement the flag!

One option is to have simple boolean config to NetSuite.configure block. Another would be to allow developer to configure list of errors to raise. Ie.

NetSuite.configure do
  raise_on_response_errors ['INSUFFICIENT_PERMISSION']
end

iloveitaly · 2019-03-08T03:29:31Z

lib/netsuite/response.rb

+      case response_error_code
+      when 'INSUFFICIENT_PERMISSION'
+        raise NetSuite::PermissionError, status_detail[:message]
+      end


@aom what other error types do you think would be added to this switch statement?

@iloveitaly I don't know yet :) By implementing switch-statement I wanted to embrace the possibility of handling more errors that should raise.

https://netsuite.custhelp.com/app/answers/detail/a_id/11613 I see a lot of error types, but I don't know which will be useful here.

It seems my user account doesn't have access to that link.

Nevermind. I had to open SuiteAnswers from NetSuite Help so it authenticated me and I was able to open the link directly.

Included list to the documentation as a comment in raise_on_response_errors method.

In my use case I've burned myself with INSUFFICIENT_PERMISSION errors often enough that I wanted it to raise instead of fail silently. I can't say whether other error codes should raise but I assume someone might have a good case to add more of them to this list in future!

@iloveitaly @aom This makes good sense to me. Though I would think that we wouldn't want to fail silently for any unsuccessful requests.

Would it be useful to have a "generic" ResponseError raised for all other error types and then we could add to the list of specific errors as needed?

def raise_on_response_errors return unless response_error_code case response_error_code when 'INSUFFICIENT_PERMISSION' raise NetSuite::PermissionError, status_detail[:message] else raise NetSuite::UnsuccessfulResponseError, "#{status_detail[:code]}: #{status_detail[:message]}" end end

I don't know if it would be awkward to have have a mix of specific and generic errors raised

Also, I realize that this PR has been open for a while. Adding the generic error could definitely be done in a second phase so it doesn't hold this one up any longer.

lib/netsuite/response.rb

freezepl · 2019-03-08T09:33:51Z

Feature flag is probably a good idea as this might cause unexpected exceptions for people using this gem. I'm happy to implement the flag!

One option is to have simple boolean config to NetSuite.configure block. Another would be to allow developer to configure list of errors to raise. Ie.
NetSuite.configure do
  raise_on_response_errors ['INSUFFICIENT_PERMISSION']
end

I think this is right approach. Although I would use only

NetSuite.configure do
  raise_on_response_errors true
end

Update fork

aom · 2019-03-08T10:06:53Z

@freezepl Agreed. As long as we're only handling one response error it should not be any more complicated to turn the feature on. I did the gate now.

I think I'll need to update this to README.md next if this is what we'll want to merge?

freezepl · 2019-03-08T10:20:19Z

Thanks @aom this looks awesome!

Ideally this option should be a default one, but it is a breaking change. It could be released under v 1.X version in the future.

@iloveitaly what do you think about this PR now?
I would give 🚢

iloveitaly · 2019-03-08T15:01:01Z

Why should we gate this behind a feature flag? Given that this is less than < 1.0 I don't see any reason why we can't introduce breaking changes, especially if we document them in the release notes.

I think raising an exception on these sort of hard NetSuite failures feels like the correct default behavior and I can't imagine a world where folks wouldn't want an exception to be raised.

Let me know what I'm missing here!

freezepl · 2019-03-08T15:03:00Z

@iloveitaly you commented here about unintended side effects, so we thought it can be turn on in the configuration..

Maybe we can do it other way round ? Turn it off in configuration?

aom · 2019-03-11T09:51:50Z

How about this:

With raise_on_response_errors being false by default it would not be a breaking change and could already be merged to master.

Then at 1.0 you could change the default to true and add it as a breaking change to release notes? There could be a simple PR labeled with 1.0 for this change.

aom · 2019-03-11T09:55:02Z

I agree with @iloveitaly that these sort of hard NetSuite failures should raise instead fail silently. It would've been especially helpful when we we're setting up the access management for our project :)

But including it in README.md's configuration example might do the trick without breaking changes.

freezepl · 2019-03-18T11:48:27Z

@iloveitaly any updates on this PR? Can we merge it?

freezepl · 2019-03-27T10:13:11Z

Bump! Guys would like to use this feature and I don't want to use fork. Can we merge it? @iloveitaly

freezepl · 2019-04-05T09:51:04Z

@iloveitaly any thoughts ?

freezepl · 2019-04-23T12:06:34Z

@iloveitaly can we merge this PR ?

iloveitaly · 2019-04-23T18:49:21Z

Sorry, haven't had time to look at this! I need to run some tests on other systems first. Not a priority at the moment, unfortunately :(

freezepl · 2019-09-23T11:40:01Z

5 moths later @iloveitaly could we do something about this PR ? (after fixing conflicts)

aom · 2019-10-07T08:41:40Z

@freezepl @iloveitaly Rebased with master to resolve conflicts.

aom · 2021-09-27T06:24:57Z

I'm no longer maintaining the fork. Best people to have a chat about changes to this PR are @beyondcompute and @edimossilva

aom force-pushed the raise-on-response-permission-errors branch from b0c8481 to dcdbaa4 Compare August 16, 2018 08:39

freezepl mentioned this pull request Jan 7, 2019

Action get_all return errors #423

Open

iloveitaly reviewed Mar 8, 2019

View reviewed changes

Merge pull request #1 from NetSweet/master

2c973a7

Update fork

freezepl approved these changes Mar 11, 2019

View reviewed changes

iloveitaly force-pushed the master branch 2 times, most recently from 7185ffc to df796ee Compare May 28, 2019 15:55

Merge remote-tracking branch 'upstream/master'

18bb31a

aom added 2 commits October 7, 2019 11:39

Merge remote-tracking branch 'upstream/master'

979f6d1

Raises on INSUFFICIENT_PERMISSION error from NetSuite response

940278f

aom force-pushed the raise-on-response-permission-errors branch from 8deebe2 to 940278f Compare October 7, 2019 08:40

iloveitaly mentioned this pull request Aug 17, 2021

return-netsuite-error-for-search-action-failures #485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raises on INSUFFICIENT_PERMISSION error from NetSuite response #405

Raises on INSUFFICIENT_PERMISSION error from NetSuite response #405

aom commented Aug 16, 2018

freezepl commented Feb 4, 2019

iloveitaly commented Feb 5, 2019

freezepl commented Feb 11, 2019

JakubKopys commented Feb 27, 2019

iloveitaly commented Feb 27, 2019

aom commented Mar 5, 2019

iloveitaly Mar 8, 2019

aom Mar 8, 2019

freezepl Mar 8, 2019

aom Mar 8, 2019

aom Mar 8, 2019

aom Mar 8, 2019

Timothyjb Aug 18, 2021

Timothyjb Aug 18, 2021

freezepl commented Mar 8, 2019

aom commented Mar 8, 2019

freezepl commented Mar 8, 2019

iloveitaly commented Mar 8, 2019

freezepl commented Mar 8, 2019

aom commented Mar 11, 2019

aom commented Mar 11, 2019

freezepl commented Mar 18, 2019

freezepl commented Mar 27, 2019

freezepl commented Apr 5, 2019

freezepl commented Apr 23, 2019

iloveitaly commented Apr 23, 2019

freezepl commented Sep 23, 2019

aom commented Oct 7, 2019

aom commented Sep 27, 2021

Raises on INSUFFICIENT_PERMISSION error from NetSuite response #405

Are you sure you want to change the base?

Raises on INSUFFICIENT_PERMISSION error from NetSuite response #405

Conversation

aom commented Aug 16, 2018

freezepl commented Feb 4, 2019

iloveitaly commented Feb 5, 2019

freezepl commented Feb 11, 2019

JakubKopys commented Feb 27, 2019

iloveitaly commented Feb 27, 2019

aom commented Mar 5, 2019

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

freezepl commented Mar 8, 2019

aom commented Mar 8, 2019

freezepl commented Mar 8, 2019

iloveitaly commented Mar 8, 2019

freezepl commented Mar 8, 2019

aom commented Mar 11, 2019

aom commented Mar 11, 2019

freezepl commented Mar 18, 2019

freezepl commented Mar 27, 2019

freezepl commented Apr 5, 2019

freezepl commented Apr 23, 2019

iloveitaly commented Apr 23, 2019

freezepl commented Sep 23, 2019

aom commented Oct 7, 2019

aom commented Sep 27, 2021