-
Notifications
You must be signed in to change notification settings - Fork 246
Credentials fix #1090
Credentials fix #1090
Changes from 1 commit
b1064fe
258ab1c
d110575
1634b0c
7f3f5cc
511f774
2f87e29
c6c093b
9a7b70e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # encoding: utf-8 | ||
| # Copyright (c) Microsoft Corporation. All rights reserved. | ||
| # Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| module Azure::Common | ||
| # The Azure::Common::Configurable module provides basic configuration for Azure activities. | ||
| module Configurable | ||
| # @return [String] Azure tenant id (also known as domain). | ||
| attr_accessor :tenant_id | ||
|
|
||
| # @return [String] Azure client id. | ||
| attr_accessor :client_id | ||
|
|
||
| # @return [String] Azure secret key. | ||
| attr_accessor :client_secret | ||
|
|
||
| # @return [String] Azure subscription id. | ||
| attr_accessor :subscription_id | ||
|
|
||
| # @return [MsRestAzure::ActiveDirectoryServiceSettings] Azure active directory service settings. | ||
| attr_accessor :active_directory_settings | ||
|
|
||
| # @return [MsRest::ServiceClientCredentials] credentials to authorize HTTP requests made by the service client. | ||
| attr_accessor :credentials | ||
|
|
||
| class << self | ||
| # | ||
| # List of configurable keys for {Azure::Common::Client}. | ||
| # @return [Array] of option keys. | ||
| # | ||
| def keys | ||
| @keys ||= [:tenant_id, :client_id, :client_secret, :subscription_id, :active_directory_settings] | ||
| end | ||
| end | ||
|
|
||
| # | ||
| # Set configuration options using a block. | ||
| # | ||
| def configure | ||
| yield self | ||
| end | ||
|
|
||
| # | ||
| # Resets the configurable options to provided options or defaults. | ||
| # This will also creates MsRest::TokenCredentials to be used for subsequent Azure Resource Manager clients. | ||
| # | ||
| def reset!(options = {}) | ||
| Azure::Common::Configurable.keys.each do |key| | ||
| default_value = Azure::Common::Default.options[key] | ||
| instance_variable_set(:"@#{key}", options.fetch(key, default_value)) | ||
| end | ||
|
|
||
| default_value = get_credentials(self.tenant_id, self.client_id, self.client_secret, self.active_directory_settings) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this needed?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why we need to get credentials in Configurable and remove it from Default, where it was.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in the previous comment, the user may or may not provide credentials in the options. If he provides it, we will use it. Else, we will derive it from the other values supplied. The 2 fetch are different. The first one (in setup_options) is getting the credentials from the default value and the second one is from the user supplied values (not the environment variables). |
||
| instance_variable_set(:"@credentials", options.fetch(:credentials, default_value)) | ||
|
|
||
| self | ||
| end | ||
|
|
||
| def config | ||
| self | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # | ||
| # configures configurable options to default values | ||
| # | ||
| def setup_options | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be called setup_default_options?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. WIll change the name and regen in a few minutes |
||
| opts = {} | ||
| Azure::Common::Configurable.keys.map do |key| | ||
| opts[key] = Azure::Common::Default.options[key] | ||
| end | ||
| opts[:credentials] = get_credentials(opts[:tenant_id], opts[:client_id], opts[:client_secret], opts[:active_directory_settings]) | ||
| opts | ||
| end | ||
|
|
||
| def get_credentials(tenant_id, client_id, client_secret, active_directory_settings) | ||
| MsRest::TokenCredentials.new( | ||
| MsRestAzure::ApplicationTokenProvider.new( | ||
| tenant_id, client_id, client_secret, active_directory_settings)) | ||
| end | ||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # encoding: utf-8 | ||
| # Copyright (c) Microsoft Corporation. All rights reserved. | ||
| # Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| module Azure::Common | ||
| module Default | ||
| class << self | ||
| # | ||
| # Default Azure Tenant Id. | ||
| # @return [String] Azure Tenant Id. | ||
| # | ||
| def tenant_id | ||
| ENV['AZURE_TENANT_ID'] | ||
| end | ||
|
|
||
| # | ||
| # Default Azure Client Id. | ||
| # @return [String] Azure Client Id. | ||
| # | ||
| def client_id | ||
| ENV['AZURE_CLIENT_ID'] | ||
| end | ||
|
|
||
| # | ||
| # Default Azure Client Secret. | ||
| # @return [String] Azure Client Secret. | ||
| # | ||
| def client_secret | ||
| ENV['AZURE_CLIENT_SECRET'] | ||
| end | ||
|
|
||
| # | ||
| # Default Azure Subscription Id. | ||
| # @return [String] Azure Subscription Id. | ||
| # | ||
| def subscription_id | ||
| ENV['AZURE_SUBSCRIPTION_ID'] | ||
| end | ||
|
|
||
| # | ||
| # Default Azure Active Directory Service Settings. | ||
| # @return [MsRestAzure::ActiveDirectoryServiceSettings] Azure Active Directory Service Settings. | ||
| # | ||
| def active_directory_settings | ||
| MsRestAzure::ActiveDirectoryServiceSettings.get_azure_settings | ||
| end | ||
|
|
||
| # | ||
| # Configuration options. | ||
| # @return [Hash] Configuration options. | ||
| # | ||
| def options | ||
| Hash[Azure::Common::Configurable.keys.map { |key| [key, send(key)]}] | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would our guidelines from before profile changes not work for initializing credentials, I believe we were doing:
If so, should we leave them as they were, just updating the namespace for ResourceManagementClient? so we demonstrate the minimum change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, after this PR is done, we'd need to re-update the samples that were updated with the last release too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The detailed explanation would be that we always want the user (of the SDK) to provide us with the tenant_id, client_id, client_secret and subscription_id. There is no escaping that. The credentials is just a derived value from these. The user may or may not provide it.
But, when you think about it, why would a user want to provide a derived value when he/she is supplying the original values anyway? That is the reason, I am updating the examples in readme and removed the credentials. But, if a user wants to provide it, then he is free to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... Credentials are not a bad thing to provide. This abstraction allows any token provider to be used.