Removing exceptions thrown for AccessToken authentication #5135

tiffanyachen · 2017-12-12T23:23:50Z

Removing exceptions thrown for AccessToken authentication - further work likely needed post update on the AuthenticationFactory side.

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

RandalliLama · 2017-12-13T00:06:26Z

src/ResourceManager/KeyVault/Commands.KeyVault/Models/DataServiceCredential.cs

-            if (context.Account.Type != AzureAccount.AccountType.User &&
-                context.Account.Type != AzureAccount.AccountType.ServicePrincipal)
-                throw new ArgumentException(string.Format(KeyVaultProperties.Resources.UnsupportedAccountType, context.Account.Type));
-
            if (context.Subscription != null && context.Account != null)
                tenantId = context.Subscription.GetPropertyAsArray(AzureSubscription.Property.Tenants)
                       .Intersect(context.Account.GetPropertyAsArray(AzureAccount.Property.Tenants))


@markcowl This code retrieves looks for the tenant ID in two different places. The code around this line is intersecting subscription tenants with account tenants. If we don't find the tenant here, then we retrieve the tenant from the context Tenant property. Do you know of a reason that we shouldn't just retrieve the tenant directly from the context rather than doing the whole intersecting thing?

Also, are we guaranteed to be able to get a tenant id regardless of Account.Type? In other words, can we/should we throw if we don't find a tenant id at all?

@RandalliLama It is best practice to get the tenant from the context

markcowl · 2017-12-16T00:03:00Z

src/ResourceManager/KeyVault/Commands.KeyVault/Models/DataServiceCredential.cs

@@ -94,8 +90,6 @@ private static string GetTenantId(IAzureContext context)

        private static Tuple<IAccessToken, string> GetTokenInternal(string tenantId, IAuthenticationFactory authFactory, IAzureContext context, string resourceIdEndpoint)
        {
-            if (string.IsNullOrWhiteSpace(tenantId))


Why is this being removed?

@markcowl This was removed because in the case that we don't get the tenant (with Account.Type being User or ServicePrincipal), I didn't want it to fail.

However, looks like we'd want to grab the tenant from the context in this case - does that sound reasonable? If so I'll go ahead and change it up that way.

@tiffanyachen Correct, that would eb the correct thing to do here.

Deepthi1111 · 2017-12-16T00:06:00Z

Could someone please unsubscribe me from the mails starting with subject: [Azure/azure I am no longer with the project and can’t login to github to unsubscribe myself. Thanks, Deepthi.

…

Sent from my iPhone On Dec 12, 2017, at 3:24 PM, tiffanyachen <[email protected]<mailto:[email protected]>> wrote: Removing exceptions thrown for AccessToken authentication - further work likely needed post update on the AuthenticationFactory side. Description

________________________________ This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here<https://msdn.microsoft.com/en-us/library/dd878270(v=vs.85).aspx>. * I have read the contribution guidelines<https://github.com/Azure/azure-powershell/blob/preview/CONTRIBUTING.md>. * If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module<https://github.com/Azure/azure-powershell/blob/preview/documentation/help-generation.md>. * If any large changes are made to a service, they are reflected in the respective change log<https://github.com/Azure/azure-powershell/blob/preview/CONTRIBUTING.md#updating-the-change-log>. General Guidelines<https://github.com/Azure/azure-powershell/blob/preview/CONTRIBUTING.md#general-guidelines> * Title of the pull request 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 more information on cleaning up the commits in your PR, see this page<https://github.com/Azure/azure-powershell/blob/preview/documentation/cleaning-up-commits.md>. * The pull request does not introduce breaking changes<https://github.com/Azure/azure-powershell/blob/preview/documentation/breaking-changes/breaking-changes-definition.md> (unless a major version change occurs in the assembly and module). Testing Guidelines<https://github.com/Azure/azure-powershell/blob/preview/CONTRIBUTING.md#testing-guidelines> * Pull request includes test coverage for the included changes. * PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources. Cmdlet Signature Guidelines<https://github.com/Azure/azure-powershell/blob/preview/CONTRIBUTING.md#cmdlet-signature-guidelines> * New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here<https://github.com/Azure/azure-powershell/wiki/PowerShell-Cmdlet-Design-Guidelines#supportsshouldprocess>. * Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter. Cmdlet Parameter Guidelines<https://github.com/Azure/azure-powershell/blob/preview/CONTRIBUTING.md#cmdlet-parameter-guidelines> * Parameter types should not expose types from the management library - complex parameter types should be defined in the module. * Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet. * Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

________________________________ You can view, comment on, or merge this pull request online at: #5135 Commit Summary * Removed check throwing exception for AccessToken authentication * Removed check for empty tenantId File Changes * M src/ResourceManager/KeyVault/Commands.KeyVault/Models/DataServiceCredential.cs<https://github.com/Azure/azure-powershell/pull/5135/files#diff-0> (6) Patch Links: * https://github.com/Azure/azure-powershell/pull/5135.patch * https://github.com/Azure/azure-powershell/pull/5135.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#5135>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AO7uFVjphF0BVDWq3nWWwTf60QW0rSPWks5s_ws4gaJpZM4Q_yQW>.

markcowl · 2017-12-20T00:43:41Z

src/ResourceManager/KeyVault/Commands.KeyVault/Models/DataServiceCredential.cs

@@ -94,8 +90,6 @@ private static string GetTenantId(IAzureContext context)

        private static Tuple<IAccessToken, string> GetTokenInternal(string tenantId, IAuthenticationFactory authFactory, IAzureContext context, string resourceIdEndpoint)
        {
-            if (string.IsNullOrWhiteSpace(tenantId))


@tiffanyachen Correct, that would eb the correct thing to do here.

markcowl · 2017-12-28T20:09:05Z

Closing in favor of #5158

tiffanyachen added 2 commits December 11, 2017 17:19

Removed check throwing exception for AccessToken authentication

64eeb93

Removed check for empty tenantId

0e82142

tiffanyachen requested a review from RandalliLama December 12, 2017 23:23

azuresdkci added the needs-review label Dec 12, 2017

azuresdkci assigned shahabhijeet Dec 12, 2017

RandalliLama reviewed Dec 13, 2017

View reviewed changes

markcowl requested changes Dec 16, 2017

View reviewed changes

markcowl assigned tiffanyachen and unassigned shahabhijeet Dec 16, 2017

markcowl added needs-revision and removed needs-review labels Dec 16, 2017

markcowl requested changes Dec 20, 2017

View reviewed changes

markcowl closed this Dec 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Removing exceptions thrown for AccessToken authentication #5135

Removing exceptions thrown for AccessToken authentication #5135

Uh oh!

tiffanyachen commented Dec 12, 2017 •

edited by azuresdkci

Loading

Uh oh!

RandalliLama Dec 13, 2017

Uh oh!

markcowl Dec 16, 2017

Uh oh!

markcowl Dec 16, 2017

Uh oh!

tiffanyachen Dec 19, 2017

Uh oh!

markcowl Dec 20, 2017

Uh oh!

Deepthi1111 commented Dec 16, 2017 via email

Uh oh!

markcowl Dec 20, 2017

Uh oh!

markcowl commented Dec 28, 2017

Uh oh!

Uh oh!

Removing exceptions thrown for AccessToken authentication #5135

Removing exceptions thrown for AccessToken authentication #5135

Uh oh!

Conversation

tiffanyachen commented Dec 12, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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!

Deepthi1111 commented Dec 16, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl commented Dec 28, 2017

Uh oh!

Uh oh!

tiffanyachen commented Dec 12, 2017 •

edited by azuresdkci

Loading