Added OAuth2 GET and POST to GraphRBAC.json spec #3563

shanepeckham · 2018-08-01T12:48:21Z

Purpose of PR

This is needed so that we can programmatically grant OAuth2 consent to an application in Azure Active Directory - we need this in the Python and Go SDKs for Azure. This is the supported and correct way to do this respecting customer approval workflows. Example use case includes Azure Kubernetes Service RBAC integration with Azure Active Directory.

Added to stable. What has been added:

In paths:

    "/{tenantID}/oauth2PermissionGrants" : {
        "get" : {
         "tags": [
           "OAuth2"
         ],
          "operationId": "OAuth2_Get",
          "produces" : [ "application/json" ],
          "parameters" : [
          {
            "name" : "$filter",
            "in" : "query",
            "required" : false,
            "type" : "string",
            "x-example" : "clientId+eq+'61ed44c3-5a1d-4639-a215-07f25129c6c3'"
          },
          {
            "$ref": "#/parameters/ApiVersionParameter"
          },
          {
            "$ref": "#/parameters/tenantIDInPath"
          }
         ],
          "responses" : {
            "200" : {
              "description" : "OK. The operation was successful.",
              "schema" : {
                "$ref" : "#/definitions/OAuth2"
              }
            }
          }
        },
        "post" : {
         "tags": [
           "OAuth2"
         ],
          "consumes" : [ "application/json" ],
          "operationId": "OAuth2_Post",
          "produces" : [ "application/json" ],
          "parameters" : [ 
            {
            "in" : "body",
            "name" : "body",
            "required" : false,
            "schema" : {
              "$ref" : "#/definitions/OAuth2"
                },
                "x-examples" : {
                    "application/json" : "{\n\t\"odata.type\": \"Microsoft.DirectoryServices.OAuth2PermissionGrant\",\n\t\"clientId\": \"39afbaa2-4a5c-4f5b-9ee3-2c83f09bbc87\", \n\t\"consentType\": \"AllPrincipals\",\n\t\"principalId\": null,\n\t\"resourceId\": \"d3247842-c517-4520-80a7-332690ae2fe4\",\n\t\"scope\": \"user_impersonation\",\n    \"startTime\": \"0001-01-01T00:00:00\",\n    \"expiryTime\": \"9000-01-01T00:00:00\"\n}"
                  }
            },
            {
            "$ref": "#/parameters/ApiVersionParameter"
            },
            {
                "$ref": "#/parameters/tenantIDInPath"
            }
          ],
          "responses" : {
            "201" : {
              "description" : "OK. The operation was successful.",
              "schema" : {
                "$ref" : "#/definitions/OAuth2"
              }
            }
          }
        }
      }

And in definitions:

    "OAuth2" : {
        "properties" : {
          "odata.type" : {
            "type" : "string",
            "description": "Microsoft.DirectoryServices.OAuth2PermissionGrant"
          },
          "clientId" : {
            "type" : "string",
            "description": "The objectId of the Service Principal associated with the app"
          },
          "consentType" : {
            "type" : "string",
            "description": "Typically set to AllPrincipals"
          },
          "principalId" : {
            "type" : "object",
            "description": "Set to null if AllPrincipals is set"
          },
          "resourceId" : {
            "type" : "string",
            "description" : "Service Principal Id of the resource you want to grant"
          },
          "scope" : {
            "type" : "string",
            "description": "Typically set to user_impersonation"
          },
          "startTime" : {
            "type" : "string",
            "description" : "Start time for TTL"
          },
          "expiryTime" : {
            "type" : "string",
            "description" : "Expiry time for TTL"
          }
        },
        "example" : {
          "odata.type" : "odata.type",
          "resourceId" : "resourceId",
          "clientId" : "clientId",
          "scope" : "scope",
          "expiryTime" : "expiryTime",
          "consentType" : "consentType",
          "principalId" : "{}",
          "startTime" : "startTime"
        }
      }

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.

azuresdkci · 2018-08-01T12:48:23Z

Can one of the admins verify this patch?

AutorestCI · 2018-08-01T12:48:23Z

Automation for azure-sdk-for-ruby

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

AutorestCI · 2018-08-01T12:48:23Z

Automation for azure-sdk-for-node

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

AutorestCI · 2018-08-01T12:48:23Z

Automation for azure-sdk-for-python

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

AutorestCI · 2018-08-01T12:48:23Z

Automation for azure-sdk-for-java

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

AutorestCI · 2018-08-01T12:48:24Z

Automation for azure-sdk-for-go

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

marstr

Thanks for being mindful of deployment as you submitted your change.

There are a couple of ways forward:

Changes are getting deployed relatively soon: you can just make these changes to the existing stable definition of graphrbac. We'll just throw the DoNotMerge label on, and set a timeline for when it should get merged.
If it's going to be a while, and you'd like to publish preview packages in the Go & Python SDKs, move these changes to specification/graphrbac/data-plane/preview/1.6/graphrbac.json.
It's going to be a while, but there's no use publishing preview packages publicly: Create a branch in this repository by joining the GitHub team azure-rest-api-writers. Instructions on joining that team here. We can update this PR to target that branch and make the changes to the existing stable file. Then I'll review/approve the changes. When the changes are deployed server side, you can submit a new PR from the staging branch to master. Give this PR as context so the automatically assigned reviewer knows they don't need to do any further review.

shanepeckham · 2018-08-01T21:12:54Z

Thanks @marstr , I went for option 1

jhendrixMSFT · 2018-08-01T22:41:10Z

@AutorestCI regenerate azure-sdk-for-go

shanepeckham · 2018-08-02T06:28:26Z

@marstr , the Linter is failing on legacy items not related to my changes, please advise on how you would like me to proceed.

jhendrixMSFT · 2018-08-02T15:37:22Z

@AutorestCI regenerate azure-sdk-for-go

shanepeckham · 2018-08-07T13:48:08Z

A downstream PR for az cli has a dependency on this PR:

Add new az ad app permissions grant and list commands for OAuth2 permissions for AAD registered apps azure-cli#6975

marstr

Sorry for the delay on this, @shanepeckham.

In regards to the new ARM errors, I glanced through and validated that none of them will be required by us. Though, I would personally recommend adding examples to help us validate/test your API.

marstr · 2018-08-07T17:40:34Z

specification/graphrbac/data-plane/stable/1.6/graphrbac.json

@@ -2985,6 +3054,52 @@
        }
      },
      "description": "Server response for Get tenant domains API call."
+    },
+    "OAuth2" : {


As per the linter, having this share a name with the Operation Group will create a naming conflict in the generated SDKs. Left unchanged, AutoRest will automatically rename this "OAuth2Model". Given context, I'm not sure )Auth2 is a very expressive name for this anyway. Maybe something like "GrantRequest" or "GrantPermission"?

@marstr , I will change this to Permissions in anticipation that we can add a revoke in the future which does not seem to exist at this stage

marstr · 2018-08-07T17:42:18Z

specification/graphrbac/data-plane/stable/1.6/graphrbac.json

+              "x-examples" : {
+                  "application/json" : "{\n\t\"odata.type\": \"Microsoft.DirectoryServices.OAuth2PermissionGrant\",\n\t\"clientId\": \"39afbaa2-4a5c-4f5b-9ee3-2c83f09bbc87\", \n\t\"consentType\": \"AllPrincipals\",\n\t\"principalId\": null,\n\t\"resourceId\": \"d3247842-c517-4520-80a7-332690ae2fe4\",\n\t\"scope\": \"user_impersonation\",\n    \"startTime\": \"0001-01-01T00:00:00\",\n    \"expiryTime\": \"9000-01-01T00:00:00\"\n}"
+                }
+          },


The linter is also complaining about not having a "description" field here.

Added a description

marstr · 2018-08-07T17:43:03Z

specification/graphrbac/data-plane/stable/1.6/graphrbac.json

+          "in" : "query",
+          "required" : false,
+          "type" : "string",
+          "x-example" : "clientId+eq+'61ed44c3-5a1d-4639-a215-07f25129c6c3'"


The linter is complaining about not having a "description" property here.

Added a description

shanepeckham · 2018-08-08T10:31:04Z

@marstr All requested changes applied

marstr

Other than my one comment, this PR LGTM.

shanepeckham · 2018-08-09T20:34:04Z

@marstr we have another PR dependant on this one which has also been approved Azure/azure-cli#6975

shanepeckham · 2018-08-09T20:41:04Z

@marstr Sample for granting permission i.e. POST to https://graph.windows.net/{{tenantId}}/oauth2PermissionGrants:

{
	"odata.type": "Microsoft.DirectoryServices.OAuth2PermissionGrant",
	"clientId": "a417c97d-2569-4bad-bd0c-bf4b187cad00", // App A - This is the service principal objectId associated with your app you want to grant permissions to
	"consentType": "AllPrincipals",
	"principalId": null,
	"resourceId": "b8415521-9724-457e-88f7-c97448afe42c", // App B - This is the service principal objectId associated with the app you want to grant permissions to App A for
	"scope": "user_impersonation",
    "startTime": "0001-01-01T00:00:00",
    "expiryTime": "9000-01-01T00:00:00"
}

To test if it worked GET:

https://graph.windows.net/{{tenantId}}/oauth2PermissionGrants?api-version=1.6&$filter=clientId+eq+'App A Service Principal objectId'

yugangw-msft · 2018-08-13T23:11:23Z

@marstr can correct me, but @shanepeckham you might need to fix one lint error by change Permission to permission

Property named: 'odata.type', for definition: 'Permissions' must follow camelCase style. Example: 'odata.type'.

shanepeckham · 2018-08-15T09:32:11Z

@yugangw-msft , @marstr , the linter error reported (R3016 - DefinitionsPropertiesNamesCamelCase) appears throughout the document on legacy items and to me seems related to the '.' (full stop) in the property name, not the case. Is this correct?

yugangw-msft · 2018-08-16T20:20:08Z

the linter error reported (R3016 - DefinitionsPropertiesNamesCamelCase) appears throughout the document on legacy items and to me seems related to the '.' (full stop) in the property name, not the case. Is this correct?

@marstr are you able to clarify?

marstr

Clarifying my expectations from the linter :)

Many of the errors that are reported aren't actually new.

specification/graphrbac/data-plane/stable/1.6/graphrbac.json

marstr

The linter errors that are being reported are all now ARM specific requirements, therefor this data-plane library is not beholden to them. (Though for the record, it may help you to add more examples)

marstr · 2018-08-17T21:44:14Z

Let me know when you're ready for me to merge this from the server side. Our expectation is that it is ready for public consumption in at least one Azure region.

shanepeckham · 2018-08-18T05:51:59Z

Thank you @marstr , we are good to go.

* Adding Microsoft.Solutions 2021-02-01-preview Same state as RPSaaSDev * fix swagger LintDiff errors * Fix LilntDiff and add more examples * Fix examples and ModelValidation * Fix jitscheduling policy * another attempt to fix ModelValidation * nit * Update updateJitRequest.json

* Revert "Adding Microsoft.Solutions 2021-02-01-preview (#3563)" This reverts commit ff3bc45. * Revert "Revert "Adding Microsoft.Solutions 2021-02-01-preview (#3563)"" This reverts commit 722eafe10cfb14fcc7aae8a620fcc020e46139cb. * Adding new APIs for VM extension * adding agent version * adding suppression for secrets * supressing api key * corrected api version * fixed typo * updating the summary * updating description * resolving warnings * Update logz.json

shanepeckham changed the title ~~Added OAuth2 to GraphRBAC spec~~ Added OAuth2 GET and POST to GraphRBAC.json spec Aug 1, 2018

azuresdkci assigned marstr Aug 1, 2018

azuresdkci requested a review from marstr August 1, 2018 12:49

marstr requested changes Aug 1, 2018

View reviewed changes

AutorestCI mentioned this pull request Aug 1, 2018

[AutoPR graphrbac/data-plane] Added OAuth2 GET and POST to GraphRBAC.json spec Azure/azure-sdk-for-go#2371

Closed

AutorestCI mentioned this pull request Aug 7, 2018

[AutoPR graphrbac/data-plane] Added OAuth2 GET and POST to GraphRBAC.json spec Azure/azure-sdk-for-go#2407

Merged

shanepeckham mentioned this pull request Aug 7, 2018

Add new az ad app permissions grant and list commands for OAuth2 permissions for AAD registered apps Azure/azure-cli#6975

Closed

marstr requested changes Aug 7, 2018

View reviewed changes

Cleaned up graphrbac.json and rebase

1387884

shanepeckham force-pushed the oauth2permissions branch from d2bcb30 to 1387884 Compare August 8, 2018 13:09

marstr requested changes Aug 9, 2018

View reviewed changes

marstr requested changes Aug 16, 2018

View reviewed changes

specification/graphrbac/data-plane/stable/1.6/graphrbac.json Show resolved Hide resolved

specification/graphrbac/data-plane/stable/1.6/graphrbac.json Show resolved Hide resolved

Add description to post body for OAuth2 Permissions

3482509

AutorestCI mentioned this pull request Aug 17, 2018

[AutoPR graphrbac/data-plane] Added OAuth2 GET and POST to GraphRBAC.json spec Azure/azure-sdk-for-ruby#1583

Merged

marstr approved these changes Aug 17, 2018

View reviewed changes

marstr merged commit 146bc8c into Azure:master Aug 22, 2018

shanepeckham deleted the oauth2permissions branch August 22, 2018 20:59

jhendrixMSFT added a commit to Azure/azure-sdk-for-go that referenced this pull request Aug 22, 2018

Created to sync Azure/azure-rest-api-specs#3563

6850280

jhendrixMSFT pushed a commit to Azure/azure-sdk-for-go that referenced this pull request Aug 22, 2018

Created to sync Azure/azure-rest-api-specs#3563 (#2512)

d82257e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OAuth2 GET and POST to GraphRBAC.json spec #3563

Added OAuth2 GET and POST to GraphRBAC.json spec #3563

shanepeckham commented Aug 1, 2018 •

edited

Loading

azuresdkci commented Aug 1, 2018

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

marstr left a comment

shanepeckham commented Aug 1, 2018

jhendrixMSFT commented Aug 1, 2018

shanepeckham commented Aug 2, 2018

jhendrixMSFT commented Aug 2, 2018

shanepeckham commented Aug 7, 2018 •

edited

Loading

marstr left a comment

marstr Aug 7, 2018

shanepeckham Aug 8, 2018

marstr Aug 7, 2018

shanepeckham Aug 8, 2018 •

edited

Loading

marstr Aug 7, 2018

shanepeckham Aug 8, 2018

shanepeckham commented Aug 8, 2018

marstr left a comment

shanepeckham commented Aug 9, 2018

shanepeckham commented Aug 9, 2018 •

edited

Loading

yugangw-msft commented Aug 13, 2018

shanepeckham commented Aug 15, 2018

yugangw-msft commented Aug 16, 2018

marstr left a comment

marstr left a comment

marstr commented Aug 17, 2018

shanepeckham commented Aug 18, 2018

Added OAuth2 GET and POST to GraphRBAC.json spec #3563

Added OAuth2 GET and POST to GraphRBAC.json spec #3563

Conversation

shanepeckham commented Aug 1, 2018 • edited Loading

Purpose of PR

PR information

Quality of Swagger

azuresdkci commented Aug 1, 2018

AutorestCI commented Aug 1, 2018 • edited Loading

Automation for azure-sdk-for-ruby

AutorestCI commented Aug 1, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Aug 1, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Aug 1, 2018 • edited Loading

Automation for azure-sdk-for-java

AutorestCI commented Aug 1, 2018 • edited Loading

Automation for azure-sdk-for-go

marstr left a comment

Choose a reason for hiding this comment

shanepeckham commented Aug 1, 2018

jhendrixMSFT commented Aug 1, 2018

shanepeckham commented Aug 2, 2018

jhendrixMSFT commented Aug 2, 2018

shanepeckham commented Aug 7, 2018 • edited Loading

marstr left a comment

Choose a reason for hiding this comment

marstr Aug 7, 2018

Choose a reason for hiding this comment

shanepeckham Aug 8, 2018

Choose a reason for hiding this comment

marstr Aug 7, 2018

Choose a reason for hiding this comment

shanepeckham Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

marstr Aug 7, 2018

Choose a reason for hiding this comment

shanepeckham Aug 8, 2018

Choose a reason for hiding this comment

shanepeckham commented Aug 8, 2018

marstr left a comment

Choose a reason for hiding this comment

shanepeckham commented Aug 9, 2018

shanepeckham commented Aug 9, 2018 • edited Loading

yugangw-msft commented Aug 13, 2018

shanepeckham commented Aug 15, 2018

yugangw-msft commented Aug 16, 2018

marstr left a comment

Choose a reason for hiding this comment

marstr left a comment

Choose a reason for hiding this comment

marstr commented Aug 17, 2018

shanepeckham commented Aug 18, 2018

shanepeckham commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

AutorestCI commented Aug 1, 2018 •

edited

Loading

shanepeckham commented Aug 7, 2018 •

edited

Loading

shanepeckham Aug 8, 2018 •

edited

Loading

shanepeckham commented Aug 9, 2018 •

edited

Loading