SQL blob auditing and threat detection specs and examples by jaredmoo · Pull Request #1012 · Azure/azure-rest-api-specs

jaredmoo · 2017-03-08T19:54:22Z

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.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

msftclas · 2017-03-08T19:54:26Z

@jaredmoo,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

jaredmoo · 2017-03-08T19:56:10Z

arm-sql/2015-05-01-preview/examples/DatabaseBlobAuditingCreateMax.json

+        "kind": "V12",
+          "properties": {
+            "state":"Enabled",
+            "storageEndpoint": "https://mystorage.blob.core.windows.net",


Comment by @anudeepsharma :

ARM's recommendation of using another resource is by ID, and as first party application you can always query access keys. Now you are putting responsibility of updating the storage account key whenever user recycle the storage account keys. I am putting this comment specially here as these APIs are in preview.

This is applicable for SecurityAlert as well (you can open a backlog item for the same for next API version)

In case you ask for storageAccount ID, you don't need to ask subscription id as well. So in total StorageEndpoint, AccessKeys, StorageSubscription, isStorageSecondaryKeyInUse will be replaced by one entity StorageAccountId

anudeepsharma · 2017-03-09T18:45:39Z

arm-sql/2015-05-01-preview/examples/ServerSecurityAlertCreateMax.json

+            "state": "Enabled",
+            "emailAccountAdmins": "Enabled",
+            "emailAddresses": "testSecurityAlert@microsoft.com",
+            "disabledAlerts": "Access_Anomaly;Usage_Anomaly",


Array here would have been better for disabledAlerts and emailAddresses

anudeepsharma

https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209097807

Linter errors on PR. Please fix the same.

For comments on APIs please create backlog item in RP to fix and get it reviewed for next API version.

amarzavery · 2017-03-09T18:50:13Z

arm-sql/2015-05-01-preview/examples/ServerSecurityAlertCreateMax.json

+        "properties": {
+            "state": "Enabled",
+            "emailAccountAdmins": "Enabled",
+            "emailAddresses": "testSecurityAlert@microsoft.com",


The property name is in plural form "emailAddress es" and it only accepts one email address as a string. An array would have been better.

@ayeletshpigelman please make sure it's noted as a bug.

amarzavery · 2017-03-09T18:57:36Z

arm-sql/2014-04-01/examples/DatabaseSecurityAlertCreateMin.json

+            "state": "Enabled",
+            "emailAccountAdmins": "Enabled",
+            "emailAddresses": "",
+            "disabledAlerts": "",


Is it OK to send an empty string back in the response (if the property was not set in the request body OR due to any other reason)? An optional property not being present if not set or it being present always with an empty string are two different things. Looks like the latter is true. If that is indeed the case then should such properties have "default": "".
I have seen this for "storageAccountAccessKey" and "storageEndpoint" as well.

If "state": "Disabled" then everything else is optional. If "state": "Enabled" then "storageEndpoint" and "storageAccountAccessKey" are required. I'll split these out into 2 different examples.

Also emailAddresses etc can be set to null, so empty string is not default.

Actually the min/max examples are already accurate (when enabled=true). I just have to make state required.

Fixed in 94e1a24

jaredmoo · 2017-03-14T02:55:06Z

@anudeepsharma @amarzavery can you please review?

Build failed due to compositeAutomation.json . Not my fault
For ENUM_MISMATCH warnings in example validation: This warning is legit, but I'm not sure how to best document this value. My thinking was to have modelAsString enum so that I can at least list the possible values, even though it's a comma-separated list so it doesn't really work as an enum. (Yes this has design issues, but we are documenting the API as it exists in production). Should I just make this a regular string without enum?
The remaining linter warnings I have discussed with @veronicagg and are false positives

anudeepsharma

LGTM

jaredmoo · 2017-03-14T17:00:49Z

Thanks Anudeep! From: Anudeep Sharma [mailto:notifications@github.com] Sent: Tuesday, March 14, 2017 9:59 AM To: Azure/azure-rest-api-specs <azure-rest-api-specs@noreply.github.com> Cc: Jared Moore <Jared.Moore@microsoft.com>; Mention <mention@noreply.github.com> Subject: Re: [Azure/azure-rest-api-specs] SQL blob auditing and threat detection specs and examples (#1012) Merged #1012<#1012>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1012 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ADGS935zAWBsVgnkiEDBadGGiL0xh58hks5rlsdngaJpZM4MXO42>.

AutorestCI · 2017-03-14T17:02:27Z

No modification for NodeJS

Rebasing with master 11/26

ayeletshpigelman and others added 15 commits February 15, 2017 15:59

Merge remote-tracking branch 'refs/remotes/Azure/master'

a600be3

Merge remote-tracking branch 'refs/remotes/Azure/master'

35a560a

add blob auditing to compositeSql

d3775f4

Merge remote-tracking branch 'refs/remotes/Azure/master'

8565a8a

Add blob auditing and threat detection swagger files

4917829

correct id at DatabaseBlobAuditingGet

7ea1349

update specs and examples according to code review comments

2b74082

Remove deprecated field

56d8c39

Add description to properties in specs

9a67b41

fix identation

2a0f6a5

Moved data security specs into 2015-05-01-preview folder

8cd7ff3

Use common ServerNameParameter

57c3990

Use x-ms-client-flatten (fixes warnings)

149ae5e

Added missing descriptions

673be0c

Fixed indentation

7e0d5d4

msftclas added the cla-not-required label Mar 8, 2017

jaredmoo mentioned this pull request Mar 8, 2017

SQL blob auditing and threat detection specs and examples #1011

Closed

10 tasks

jaredmoo commented Mar 8, 2017

View reviewed changes

azuresdkci assigned anudeepsharma Mar 8, 2017

anudeepsharma reviewed Mar 9, 2017

View reviewed changes

anudeepsharma suggested changes Mar 9, 2017

View reviewed changes

amarzavery reviewed Mar 9, 2017

View reviewed changes

jaredmoo added 7 commits March 9, 2017 15:18

Merge remote-tracking branch 'azure/master' into datasecurity

3499df8

Fixed anonymous type warnings

265a07b

Removed "Azure SQL" from param descriptions.

a249e5b

Turned auditing & threat policy names into enums

ed328c8

Edits to security policy specs

d149b09

More updates to data security specs

1c88c9e

Cut operation results apis. These are not part of public api.

96c0cc5

jaredmoo added 2 commits March 10, 2017 18:19

Removed policy name param. Hardcode instead.

cc6f4a8

Remove server audit and threat detection specs

c6b1c4b

jaredmoo mentioned this pull request Mar 11, 2017

SQL database auditing and threat detection Azure/azure-sdk-for-net#2925

Closed

8 tasks

jaredmoo added 4 commits March 11, 2017 18:06

Make state required

3df9729

Document storage property requirements

94e1a24

"Get" -> "Gets"

35c08ed

Fixed missing response schema

9ac0a2f

anudeepsharma approved these changes Mar 14, 2017

View reviewed changes

anudeepsharma merged commit 0d62469 into Azure:master Mar 14, 2017

jaredmoo mentioned this pull request Mar 15, 2017

Removed no longer supported SQL resource types and renamed several operations #1005

Merged

10 tasks

jaredmoo deleted the datasecurity branch July 8, 2017 17:22

mccleanp pushed a commit that referenced this pull request Mar 23, 2022

Merge pull request #1012 from Azure/user/sanjaiga/rebase1126

0a630cc

Rebasing with master 11/26

Comments

Conversation

jaredmoo commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented Mar 8, 2017

Uh oh!

jaredmoo Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

anudeepsharma Mar 9, 2017 • 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!

anudeepsharma left a comment

Choose a reason for hiding this comment

Uh oh!

amarzavery Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

jaredmoo Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

amarzavery Mar 9, 2017 • 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!

jaredmoo Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

jaredmoo Mar 12, 2017

Choose a reason for hiding this comment

Uh oh!

jaredmoo Mar 12, 2017

Choose a reason for hiding this comment

Uh oh!

jaredmoo Mar 12, 2017

Choose a reason for hiding this comment

Uh oh!

jaredmoo commented Mar 14, 2017

Uh oh!

anudeepsharma left a comment

Choose a reason for hiding this comment

Uh oh!

jaredmoo commented Mar 14, 2017 via email

Uh oh!

AutorestCI commented Mar 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jaredmoo commented Mar 8, 2017 •

edited

Loading

anudeepsharma Mar 9, 2017 •

edited

Loading

amarzavery Mar 9, 2017 •

edited

Loading