Introducing new API for Advanced Threat Protection by tomerlmsft · Pull Request #3306 · Azure/azure-rest-api-specs

tomerlmsft · 2018-06-26T14:58:05Z

API form -
{resourceUri}/providers/Microsoft.Security/advancedThreatProtectionSettings/{settingName}

Suppporting verbs -
GET, PUT and Delete.

URI example -
/subscriptions/20ff7fc3-e762-44dd-bd96-b71116dcdc23/resourceGroups/SampleRG/providers/Microsoft.Storage/storageAccounts/samplestorageaccount/providers/Microsoft.Security/advancedThreatProtectionSettings/current

Design doc for ATP -
https://microsoft.sharepoint.com/teams/Data_Security/Documents/Dev%20Specs/Storage%20Threat%20Detection/Threat%20Detection%20for%20Azure%20Storage%20-%20Design%20Document.docx?d=w996c55b5a160468c9a9bd0434418a4d0

VSTS item -
#247253, #257790

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

API form - {resourceUri}/providers/Microsoft.Security/advancedThreatProtectionSettings/{settingName} Suppporting verbs - GET, PUT and Delete. URI example - /subscriptions/20ff7fc3-e762-44dd-bd96-b71116dcdc23/resourceGroups/SampleRG/providers/Microsoft.Storage/storageAccounts/samplestorageaccount/providers/Microsoft.Security/advancedThreatProtectionSettings/current Design doc for ATP - https://microsoft.sharepoint.com/teams/Data_Security/Documents/Dev%20Specs/Storage%20Threat%20Detection/Threat%20Detection%20for%20Azure%20Storage%20-%20Design%20Document.docx?d=w996c55b5a160468c9a9bd0434418a4d0

AutorestCI · 2018-06-26T14:58:33Z

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

AutorestCI · 2018-06-26T14:59:13Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2710

AutorestCI · 2018-06-26T14:59:56Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-06-26T14:59:58Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

AutorestCI · 2018-06-26T15:00:36Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

chlahav · 2018-06-27T07:54:17Z

...cation/security/resource-manager/Microsoft.Security/preview/2017-08-01-preview/security.json

+              "200": {
+                "description": "Successful request to get Advanced Threat Protection settings.",
+                "schema": {
+                  "$ref": "#/definitions/AdvancedThreatProtectionSettings"


"AdvancedThreatProtectionSetting" (singular)

I would also rename this to "AdvancedThreatProtection" so it will make more sense

resource type is always plural across all azure -
diagnosticSettings, servers, databases, geoBackupPolicies, operationResults, extendedDiagnosticSettings, etc...

It's plural also in ASC - securityContacts, workspaceSettings, pricings, etc...

Regarding AdvancedThreatProtection vs AdvancedThreatProtectionSettings -
We reviewed it internally and I changed to AdvancedThreatProtectionSettings following a feedback I got.
The purpose of these type is indeed settings so it make sense to me more than AdvancedThreatProtection which doesn't really imply on the real intent.
Also here, all previous features we released followed this pattern -
VulnerabilityAssessmentSettings, auditingSettings.
and in ASC - workspaceSettings, autoProvisioningSettings, etc...

I talked about the object name and not the resource type
the object that you return is only one setting and not settings

in workspaceSettings etc. it's the same

chlahav · 2018-06-27T07:55:36Z

...cation/security/resource-manager/Microsoft.Security/preview/2017-08-01-preview/security.json

+            },
+            "tags": [ "AdvancedThreatProtection" ],
+            "description": "Creates or updates the Advanced Threat Protection settings on a specified resource.",
+            "operationId": "AdvancedThreatProtection_Put",


AdvancedThreatProtection_Create

chlahav · 2018-06-27T08:03:12Z

...cation/security/resource-manager/Microsoft.Security/preview/2017-08-01-preview/security.json

+              }
+            }
+          },
+          "delete": {


delete doesn't have a lot of meaning here
what's the difference between this and "isEnabled": false?

please pick one of them (is enabled property or create delete a resource)
I suggest the property since it will allow you to use policies

make sense, I removed the DELETE operation

chlahav · 2018-06-27T08:04:24Z

...review/examples/AdvancedThreatProtection/DeleteAdvancedThreatProtectionSettings_example.json

+    "settingName": "current"
+  },
+  "responses": {
+    "200": {


missing 204 example (copy from the JIT delete)

you also have a test that fails on that

I'm removing the DELETE operation and stays only with GET and PUT.
When no settings are exists, GET will return "isEnalbed : false" value.
Using PUT, we'll be able to turn the settings ON or OFF.

chlahav · 2018-06-27T08:04:53Z

...cation/security/resource-manager/Microsoft.Security/preview/2017-08-01-preview/security.json

+        },
+        "AdvancedThreatProtectionProperties": {
+          "properties": {
+            "isEnabled": {


rename to "enabled"

chlahav · 2018-06-27T08:07:30Z

...cation/security/resource-manager/Microsoft.Security/preview/2017-08-01-preview/security.json

            "description": "name of the Compliance",
            "x-ms-parameter-location": "method"
+        },
+        "ResourceUri": {


change to "resourceId" to use the same standard as other RPs

chlahav · 2018-06-27T08:23:50Z

please make the "MODE=model PR_ONLY=true" test in Travis pass

marstr · 2018-06-27T16:56:19Z

To @chlahav's point, there are some missing and inconsistent examples. You can find details caught by our linter here: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/396930169#L748

marstr · 2018-06-27T17:19:42Z

...cation/security/resource-manager/Microsoft.Security/preview/2017-08-01-preview/security.json

+              "200": {
+                "description": "Successful request to remove Advanced Threat Protection setting."
+              },
+              "204": {


Just out of curiosity, is there any scenario where this operation would return a 404?

For example:
200 -> Deleted a resource, it's former contents are returned in this response
204 -> This resource was just deleted by another request.
404 -> There was never a resource by this name.

I'm just not sure what the right REST idiom is here.

I removed the DELETE operation and stayed only with PUT (following Chen feedback, see above)

*Removed DELETE operation, stayed only with GET and PUT. *Changed ResourceUri to ResourceId *Changed isEnabled to enabled

…atProtectionSetting

marstr · 2018-07-03T16:41:04Z

@tomerlmsft, this looks good from my perspective. Given that we've waited for more than a three business days without the ARM team responding, we can go ahead and merge it. However, doing so means you'll potentially have ARM feedback be reported on a production service. This can cause complications. However, it is up to you. Would you like to proceed and have me merge this PR?

ravbhatnagar · 2018-07-03T22:53:42Z

...cation/security/resource-manager/Microsoft.Security/preview/2017-08-01-preview/security.json

+          "description": "The identifier of the resource.",
+          "x-ms-parameter-location": "method"
+        },
+        "AdvancedThreatProtectionSettingName": {


if current is the only value supported for the setting name we should make an enum with one value.

Also do you support a list API?

I changed it to enum with one value, thanks for the comment.

We don't support LIST in our API, the only available type of settings name right now is "current".

ravbhatnagar · 2018-07-05T20:25:33Z

Looks good!

marstr · 2018-07-05T23:25:38Z

I see you've named this "[Do Not Merge]". When are you expecting this PR to be mergeable, @tomerlmsft?

tomerlmsft · 2018-07-06T20:29:38Z

Hi Mark,
We are still waiting with the merge until the arm manifest will be updated.

marstr · 2018-07-11T16:21:38Z

Added the [DoMergeLabel] to indicate, without opening the PR, that we are waiting for the ARM manifest to be updated.

marstr · 2018-07-25T23:00:12Z

I'm going to be on vacation for the next two days (until Monday, July 30th). If this PR needs attention in the mean time, please contact my boss, @salameer.

azuresdkci · 2018-08-02T15:30:47Z

Can one of the admins verify this patch?

marstr · 2018-08-02T15:49:44Z

Any updates on this, @tomerlmsft?

marstr · 2018-08-13T17:12:56Z

Closing as stale.

marstr · 2018-08-14T16:25:44Z

After an offline request by @tomerlmsft, I'm going to reopen this.

marstr · 2018-08-17T21:41:06Z

Restarted CI because it looks like the failure on the most recent commit was unrelated to your change.

marstr · 2018-08-24T20:58:39Z

Just as a heads up, I'm going to be OOF all next week. However, since this PR is just waiting for the service to be available, I don't think there's any need to reassign while I'm gone. If you need a swagger reviewer to take a look at this, try getting a hold of @salameer or the swagger reviewer email alias.

marstr · 2018-09-10T23:06:04Z

@tomerlmsft, heads up on this conflict that has emerged.

marstr · 2018-09-17T16:22:27Z

I was asked offline to merge this PR by @tomerlmsft.

azuresdkci assigned marstr Jun 26, 2018

azuresdkci requested a review from marstr June 26, 2018 14:59

chlahav suggested changes Jun 27, 2018

View reviewed changes

chlahav self-requested a review June 27, 2018 08:29

marstr reviewed Jun 27, 2018

View reviewed changes

marstr requested review from marstr and ravbhatnagar June 27, 2018 17:19

marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 27, 2018

tomerlmsft changed the title ~~Introducing new API for Advanced Threat Protection~~ [Do Not Merge] Introducing new API for Advanced Threat Protection Jun 28, 2018

tomerlmsft added 4 commits June 28, 2018 19:39

Addressing pull request comments

eb87b43

*Removed DELETE operation, stayed only with GET and PUT. *Changed ResourceUri to ResourceId *Changed isEnabled to enabled

Renaming obejct from AdvancedThreatProtectionSettings to AdvancedThre…

f0fc406

…atProtectionSetting

Removed delete operation from the swagger definitions

782c68f

Fixed test failures

256d582

chlahav approved these changes Jul 2, 2018

View reviewed changes

marstr approved these changes Jul 3, 2018

View reviewed changes

ravbhatnagar reviewed Jul 3, 2018

View reviewed changes

Changing "AdvancedThreatProtectionSettingName" parameter to enum type

5932dae

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jul 5, 2018

marstr added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Jul 11, 2018

marstr closed this Aug 13, 2018

AutorestCI mentioned this pull request Aug 13, 2018

[AutoPR security/resource-manager] [Do Not Merge] Introducing new API for Advanced Threat Protection Azure/azure-sdk-for-go#2448

Closed

marstr reopened this Aug 14, 2018

AutorestCI mentioned this pull request Aug 14, 2018

[AutoPR security/resource-manager] [Do Not Merge] Introducing new API for Advanced Threat Protection Azure/azure-sdk-for-go#2457

Closed

Changed param name from 'enabled' to 'isEnabled'

3382416

tomerlmsft changed the title ~~[Do Not Merge] Introducing new API for Advanced Threat Protection~~ [Do Not Merge Until September 15th] Introducing new API for Advanced Threat Protection Aug 18, 2018

marstr mentioned this pull request Sep 13, 2018

Edit Azure Security Center settings descriptions #3890

Closed

10 tasks

Merge branch 'master' into master

ec16963

AutorestCI mentioned this pull request Sep 17, 2018

[AutoPR security/resource-manager] [Do Not Merge Until September 15th] Introducing new API for Advanced Threat Protection Azure/azure-sdk-for-go#2709

Merged

marstr changed the title ~~[Do Not Merge Until September 15th] Introducing new API for Advanced Threat Protection~~ Introducing new API for Advanced Threat Protection Sep 17, 2018

marstr removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label Sep 17, 2018

marstr merged commit 1b21e70 into Azure:master Sep 17, 2018

Conversation

tomerlmsft commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

AutorestCI commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-ruby

Uh oh!

AutorestCI commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-java

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chlahav Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chlahav commented Jun 27, 2018

Uh oh!

marstr commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marstr commented Jul 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar commented Jul 5, 2018

Uh oh!

tomerlmsft commented Jun 26, 2018 •

edited

Loading

AutorestCI commented Jun 26, 2018 •

edited

Loading

AutorestCI commented Jun 26, 2018 •

edited

Loading

AutorestCI commented Jun 26, 2018 •

edited

Loading

AutorestCI commented Jun 26, 2018 •

edited

Loading

AutorestCI commented Jun 26, 2018 •

edited

Loading

chlahav Jul 2, 2018 •

edited

Loading

marstr commented Jul 5, 2018 •

edited

Loading