Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AuthV2] Address Bug Bash feedback #3782

Merged
merged 12 commits into from
Aug 30, 2021
Merged

[AuthV2] Address Bug Bash feedback #3782

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
035dd97
first lot of feedback
mkarmark Aug 17, 2021
43699e8
get tests to pass
mkarmark Aug 17, 2021
71c4a3c
fix static analysis errors
mkarmark Aug 17, 2021
833da57
remaining fixes
mkarmark Aug 17, 2021
8c934d2
fix static analysis errors
mkarmark Aug 17, 2021
bb74aca
address pr comments
mkarmark Aug 20, 2021
c2c570c
add comment
mkarmark Aug 20, 2021
e8c8e93
try to pass static analysis
mkarmark Aug 20, 2021
bae77a4
make all settings slot sticky
mkarmark Aug 20, 2021
547de53
done
mkarmark Aug 23, 2021
7a33e01
Address Sisira's comments
mkarmark Aug 27, 2021
fd40d25
Update custom.py
mkarmark Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/authV2/azext_authV2/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@
get_three_state_flag, get_enum_type)
from azure.cli.command_modules.appservice._params import AUTH_TYPES
from azure.cli.core.local_context import LocalContextAttribute, LocalContextAction
from azure.cli.core.cloud import AZURE_PUBLIC_CLOUD, AZURE_CHINA_CLOUD, AZURE_US_GOV_CLOUD, AZURE_GERMAN_CLOUD

UNAUTHENTICATED_CLIENT_ACTION = ['RedirectToLoginPage', 'AllowAnonymous', 'RejectWith401', 'RejectWith404']
FORWARD_PROXY_CONVENTION = ['NoProxy', 'Standard', 'Custom']
CLOUD_NAMES = [AZURE_PUBLIC_CLOUD.name, AZURE_CHINA_CLOUD.name, AZURE_US_GOV_CLOUD.name, AZURE_GERMAN_CLOUD.name]
AAD_VERSIONS = ['v1.0', 'v2.0']


def load_arguments(self, _):
Expand Down Expand Up @@ -76,6 +79,12 @@ def load_arguments(self, _):
c.argument('allowed_token_audiences', options_list=['--allowed-token-audiences', '--allowed-audiences'],
help='The configuration settings of the allowed list of audiences from which to validate the JWT token.')
c.argument('yes', options_list=['--yes', '-y'], help='Do not prompt for confirmation.', action='store_true')
c.argument('tenant_id', options_list=['--tenant-id'],
help='The tenant id of the application.')
c.argument('cloud', options_list=['--cloud'], arg_type=get_enum_type(CLOUD_NAMES),
mkarmark marked this conversation as resolved.
Show resolved Hide resolved
help='The name of the cloud that the application belongs to.')
c.argument('aad_version', options_list=['--aad-version'], arg_type=get_enum_type(AAD_VERSIONS),
mkarmark marked this conversation as resolved.
Show resolved Hide resolved
help='The version of the Azure Active Directory endpoint to use for the OpenID Issuer.')
mkarmark marked this conversation as resolved.
Show resolved Hide resolved

with self.argument_context('webapp auth facebook update') as c:
c.argument('app_id', options_list=['--app-id'],
Expand Down Expand Up @@ -149,6 +158,9 @@ def load_arguments(self, _):
help='The endpoint that contains all the configuration endpoints for the provider.')
c.argument('scopes', options_list=['--scopes'],
help='A list of the scopes that should be requested while authenticating.')
c.argument('client_secret', options_list=['--client-secret'],
help='The application secret of the app used for login.')
c.argument('yes', options_list=['--yes', '-y'], help='Do not prompt for confirmation.', action='store_true')

with self.argument_context('webapp auth openid-connect update') as c:
c.argument('provider_name', options_list=['--provider-name'], required=True,
Expand All @@ -161,6 +173,9 @@ def load_arguments(self, _):
help='The endpoint that contains all the configuration endpoints for the provider.')
c.argument('scopes', options_list=['--scopes'],
help='A list of the scopes that should be requested while authenticating.')
c.argument('client_secret', options_list=['--client-secret'],
help='The application secret of the app used for login.')
c.argument('yes', options_list=['--yes', '-y'], help='Do not prompt for confirmation.', action='store_true')

with self.argument_context('webapp auth openid-connect remove') as c:
c.argument('provider_name', options_list=['--provider-name'], required=True,
Expand Down
136 changes: 121 additions & 15 deletions src/authV2/azext_authV2/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,25 @@
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------
import json
from re import A
import re
from knack.prompting import prompt_y_n
from knack.util import CLIError
from azure.cli.core.util import send_raw_request
from azure.cli.command_modules.appservice._appservice_utils import _generic_site_operation
from azure.cli.command_modules.appservice.custom import update_app_settings
from azure.cli.core.commands.client_factory import get_subscription_id
from azure.cli.command_modules.appservice._params import AUTH_TYPES
from azure.cli.core.cloud import AZURE_PUBLIC_CLOUD, AZURE_CHINA_CLOUD, AZURE_US_GOV_CLOUD, AZURE_GERMAN_CLOUD

MICROSOFT_SECRET_SETTING_NAME = "MICROSOFT_PROVIDER_AUTHENTICATION_SECRET"
FACEBOOK_SECRET_SETTING_NAME = "FACEBOOK_PROVIDER_AUTHENTICATION_SECRET"
GITHUB_SECRET_SETTING_NAME = "GITHUB_PROVIDER_AUTHENTICATION_SECRET"
GOOGLE_SECRET_SETTING_NAME = "GOOGLE_PROVIDER_AUTHENTICATION_SECRET"
MSA_SECRET_SETTING_NAME = "MSA_PROVIDER_AUTHENTICATION_SECRET"
TWITTER_SECRET_SETTING_NAME = "TWITTER_PROVIDER_AUTHENTICATION_SECRET"
TRUE_STRING = "true"
FALSE_STRING = "false"


# region rest calls
Expand All @@ -31,7 +36,7 @@ def get_resource_id(cmd, resource_group_name, name, slot):
resource_group_name,
name)
if slot is not None:
resource_id = resource_id + "/slots" + slot
resource_id = resource_id + "/slots/" + slot
return resource_id


Expand All @@ -49,7 +54,24 @@ def get_auth_settings_v2(cmd, resource_group_name, name, slot=None):
return r.json()


def update_auth_settings_v2_rest_call(cmd, resource_group_name, name, site_auth_settings_v2, slot=None): # pylint: disable=unused-argument
def update_auth_settings_v2_rest_call(cmd, resource_group_name, name, site_auth_settings_v2,
slot=None, overwrite_settings=False, is_upgrade=False): # pylint: disable=unused-argument
is_using_v1 = get_config_version(cmd, resource_group_name, name, slot)["configVersion"] == 'v1'
is_new_auth_app = is_app_new_to_auth(cmd, resource_group_name, name, slot)

if not is_upgrade and is_using_v1 and not is_new_auth_app:
msg = 'Usage Error: Cannot use auth v2 commands when the app is using auth v1. ' \
'Update the auth settings using the az webapp auth-classic command group.'
raise CLIError(msg)

if not overwrite_settings: # if no auth v2 settings set, then default token store to true
if is_new_auth_app:
if "login" not in site_auth_settings_v2.keys():
site_auth_settings_v2["login"] = {}
if "tokenStore" not in site_auth_settings_v2["login"].keys():
site_auth_settings_v2["login"]["tokenStore"] = {}
site_auth_settings_v2["login"]["tokenStore"]["enabled"] = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this prevent customers from explicitly setting the login.tokenStore.enabled to False when enabling auth for the first time? If so, maybe we should default it to True only if login.tokenStore isn't explicitly configured?


final_json = {
"properties": site_auth_settings_v2
}
Expand Down Expand Up @@ -89,7 +111,8 @@ def set_auth_settings_v2(cmd, resource_group_name, name, body=None, slot=None):
json_object = None
else:
json_object = json.loads(body)
return update_auth_settings_v2_rest_call(cmd, resource_group_name, name, json_object, slot)
return update_auth_settings_v2_rest_call(cmd, resource_group_name, name, json_object,
slot, overwrite_settings=True)


def update_auth_settings_v2(cmd, resource_group_name, name, set_string=None, enabled=None, # pylint: disable=unused-argument
Expand Down Expand Up @@ -148,7 +171,8 @@ def upgrade_to_auth_settings_v2(cmd, resource_group_name, name, slot=None): # p
raise CLIError('Usage Error: Cannot use command az webapp auth upgrade when the app is using auth v2.')
prep_auth_settings_for_v2(cmd, resource_group_name, name, slot)
site_auth_settings_v2 = get_auth_settings_v2(cmd, resource_group_name, name, slot)["properties"]
return update_auth_settings_v2_rest_call(cmd, resource_group_name, name, site_auth_settings_v2, slot)
return update_auth_settings_v2_rest_call(cmd, resource_group_name, name,
site_auth_settings_v2, slot, is_upgrade=True)


def get_config_version(cmd, resource_group_name, name, slot=None): # pylint: disable=unused-argument
Expand All @@ -166,7 +190,24 @@ def revert_to_auth_settings(cmd, resource_group_name, name, slot=None): # pylin
raise CLIError('Usage Error: Cannot use command az webapp auth revert when the app is using auth v1.')
site_auth_settings = get_auth_settings(cmd, resource_group_name, name, slot)
set_auth_settings_v2(cmd, resource_group_name, name, None, slot)
update_auth_classic_settings(cmd, resource_group_name, name, site_auth_settings.enabled, None,
site_auth_settings.enabled = TRUE_STRING if site_auth_settings.enabled else FALSE_STRING
site_auth_settings.token_store_enabled = TRUE_STRING if site_auth_settings.token_store_enabled else FALSE_STRING
mkarmark marked this conversation as resolved.
Show resolved Hide resolved
action = None
if site_auth_settings.unauthenticated_client_action == "AllowAnonymous":
action = "AllowAnonymous"
elif site_auth_settings.unauthenticated_client_action == "RedirectToLoginPage":
if site_auth_settings.default_provider == "AzureActiveDirectory":
action = "LoginWithAzureActiveDirectory"
elif site_auth_settings.default_provider == "Facebook":
action = "LoginWithFacebook"
elif site_auth_settings.default_provider == "Google":
action = "LoginWithGoogle"
elif site_auth_settings.default_provider == "MicrosoftAccount":
action = "LoginWithMicrosoftAccount"
elif site_auth_settings.default_provider == "Twitter":
action = "LoginWithTwitter"
mkarmark marked this conversation as resolved.
Show resolved Hide resolved

update_auth_classic_settings(cmd, resource_group_name, name, site_auth_settings.enabled, action,
site_auth_settings.client_id, site_auth_settings.token_store_enabled,
site_auth_settings.runtime_version,
site_auth_settings.token_refresh_extension_hours,
Expand Down Expand Up @@ -194,6 +235,11 @@ def revert_to_auth_settings(cmd, resource_group_name, name, slot=None): # pylin
# region helper methods


def is_app_new_to_auth(cmd, resource_group_name, name, slot):
existing_site_auth_settings_v2 = get_auth_settings_v2(cmd, resource_group_name, name, slot)
return json.dumps(existing_site_auth_settings_v2["properties"]) == "{}"


def set_field_in_auth_settings_recursive(field_name_split, field_value, auth_settings):
if len(field_name_split) == 1:
auth_settings[field_name_split[0]] = field_value
Expand Down Expand Up @@ -396,7 +442,8 @@ def get_aad_settings(cmd, resource_group_name, name, slot=None):

def update_aad_settings(cmd, resource_group_name, name, slot=None, # pylint: disable=unused-argument
client_id=None, client_secret_setting_name=None, # pylint: disable=unused-argument
issuer=None, allowed_token_audiences=None, client_secret=None, yes=False): # pylint: disable=unused-argument
issuer=None, allowed_token_audiences=None, client_secret=None, # pylint: disable=unused-argument
yes=False, tenant_id=None, cloud=None, aad_version=None): # pylint: disable=unused-argument
if client_secret is not None and client_secret_setting_name is not None:
raise CLIError('Usage Error: --client-secret and --client-secret-setting-name cannot both be '
'configured to non empty strings')
Expand All @@ -407,6 +454,26 @@ def update_aad_settings(cmd, resource_group_name, name, slot=None, # pylint: di
raise CLIError('Usage Error: --client-secret cannot be used without agreeing to add app settings '
'to the web app.')

openid_issuer = issuer
if openid_issuer is None:
authority = cmd.cli_ctx.cloud.endpoints.active_directory
mkarmark marked this conversation as resolved.
Show resolved Hide resolved
if cloud is not None:
mkarmark marked this conversation as resolved.
Show resolved Hide resolved
if cloud == AZURE_PUBLIC_CLOUD.name:
authority = AZURE_PUBLIC_CLOUD.endpoints.active_directory
elif cloud == AZURE_CHINA_CLOUD.name:
authority = AZURE_CHINA_CLOUD.endpoints.active_directory
elif cloud == AZURE_GERMAN_CLOUD.name:
authority = AZURE_GERMAN_CLOUD.endpoints.active_directory
elif cloud == AZURE_US_GOV_CLOUD:
authority = AZURE_US_GOV_CLOUD.endpoints.active_directory

if tenant_id is not None:
openid_issuer = authority + "/" + tenant_id
if aad_version != "v2.0":
openid_issuer = openid_issuer + "/" + aad_version
else:
openid_issuer = authority + "/common/v2.0"
mkarmark marked this conversation as resolved.
Show resolved Hide resolved

existing_auth = get_auth_settings_v2(cmd, resource_group_name, name, slot)["properties"]
registration = {}
validation = {}
Expand All @@ -415,7 +482,7 @@ def update_aad_settings(cmd, resource_group_name, name, slot=None, # pylint: di
if "azureActiveDirectory" not in existing_auth["identityProviders"].keys():
existing_auth["identityProviders"]["azureActiveDirectory"] = {}
if (client_id is not None or client_secret is not None or
client_secret_setting_name is not None or issuer is not None):
client_secret_setting_name is not None or openid_issuer is not None):
if "registration" not in existing_auth["identityProviders"]["azureActiveDirectory"].keys():
existing_auth["identityProviders"]["azureActiveDirectory"]["registration"] = {}
registration = existing_auth["identityProviders"]["azureActiveDirectory"]["registration"]
Expand All @@ -433,8 +500,8 @@ def update_aad_settings(cmd, resource_group_name, name, slot=None, # pylint: di
settings = []
settings.append(MICROSOFT_SECRET_SETTING_NAME + '=' + client_secret)
update_app_settings(cmd, resource_group_name, name, settings, slot)
if issuer is not None:
registration["openIdIssuer"] = issuer
if openid_issuer is not None:
registration["openIdIssuer"] = openid_issuer
if allowed_token_audiences is not None:
validation["allowedAudiences"] = allowed_token_audiences.split(",")
existing_auth["identityProviders"]["azureActiveDirectory"]["validation"] = validation
Expand Down Expand Up @@ -754,7 +821,15 @@ def get_openid_connect_provider_settings(cmd, resource_group_name, name, provide

def add_openid_connect_provider_settings(cmd, resource_group_name, name, provider_name, slot=None, # pylint: disable=unused-argument
client_id=None, client_secret_setting_name=None, # pylint: disable=unused-argument
openid_configuration=None, scopes=None): # pylint: disable=unused-argument
openid_configuration=None, scopes=None, # pylint: disable=unused-argument
client_secret=None, yes=False): # pylint: disable=unused-argument
if client_secret is not None and not yes:
msg = 'Configuring --client-secret will add app settings to the web app. ' \
'Are you sure you want to continue?'
if not prompt_y_n(msg, default="n"):
raise CLIError('Usage Error: --client-secret cannot be used without agreeing '
'to add app settings to the web app.')

auth_settings = get_auth_settings_v2(cmd, resource_group_name, name, slot)["properties"]
if "identityProviders" not in auth_settings.keys():
auth_settings["identityProviders"] = {}
Expand All @@ -764,29 +839,51 @@ def add_openid_connect_provider_settings(cmd, resource_group_name, name, provide
raise CLIError('Usage Error: The following custom OpenID Connect provider has already been '
'configured: ' + provider_name + '. Please use az webapp auth oidc update to '
'update the provider.')

final_client_secret_setting_name = client_secret_setting_name
if client_secret is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to worry about the case where client_secret is None? If I'm reading the code correctly (the indenting might be throwing me off), it seems like we'll try to set the clientSecretSettingName regardless of whether a client secret was actually provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So per the code, if the client secret is not None, we set the app setting and set the clientSecretSettingName to whatever name we just gave that app setting we set. If client secret is none, we set clientSecretSettingName to the value of whatever is passed in to --client-secret-setting-name. If it's none, we leave it none, if it's not none, we set it to the specified value. Does that make sense, or do you recommend changing that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I forgot that when client_secret is none, it's likely because client_secret_setting_name was passed instead. Thanks!

provider_name_prefix = provider_name.upper()
if len(provider_name_prefix) > 32:
mkarmark marked this conversation as resolved.
Show resolved Hide resolved
provider_name_prefix = provider_name_prefix[0:31]
final_client_secret_setting_name = provider_name_prefix + "_PROVIDER_AUTHENTICATION_SECRET"
settings = []
settings.append(final_client_secret_setting_name + '=' + client_secret)
update_app_settings(cmd, resource_group_name, name, settings, slot)
mkarmark marked this conversation as resolved.
Show resolved Hide resolved

auth_settings["identityProviders"]["customOpenIdConnectProviders"][provider_name] = {
"registration": {
"clientId": client_id,
"clientCredential": {
"clientSecretSettingName": client_secret_setting_name
"clientSecretSettingName": final_client_secret_setting_name
},
"openIdConnectConfiguration": {
"wellKnownOpenIdConfiguration": openid_configuration
}
}
}
login = {}
if scopes is not None:
login = {}
login["scopes"] = scopes.split(',')
auth_settings["identityProviders"]["customOpenIdConnectProviders"][provider_name]["login"] = login
else:
login["scopes"] = ["openid"]

auth_settings["identityProviders"]["customOpenIdConnectProviders"][provider_name]["login"] = login

updated_auth_settings = update_auth_settings_v2_rest_call(cmd, resource_group_name, name, auth_settings, slot)
return updated_auth_settings["identityProviders"]["customOpenIdConnectProviders"][provider_name]


def update_openid_connect_provider_settings(cmd, resource_group_name, name, provider_name, slot=None, # pylint: disable=unused-argument
client_id=None, client_secret_setting_name=None, # pylint: disable=unused-argument
openid_configuration=None, scopes=None): # pylint: disable=unused-argument
openid_configuration=None, scopes=None, # pylint: disable=unused-argument
client_secret=None, yes=False): # pylint: disable=unused-argument
if client_secret is not None and not yes:
msg = 'Configuring --client-secret will add app settings to the web app. ' \
'Are you sure you want to continue?'
if not prompt_y_n(msg, default="n"):
raise CLIError('Usage Error: --client-secret cannot be used without agreeing '
'to add app settings to the web app.')

auth_settings = get_auth_settings_v2(cmd, resource_group_name, name, slot)["properties"]
if "identityProviders" not in auth_settings.keys():
raise CLIError('Usage Error: The following custom OpenID Connect provider '
Expand All @@ -805,7 +902,7 @@ def update_openid_connect_provider_settings(cmd, resource_group_name, name, prov
custom_open_id_connect_providers[provider_name]["registration"] = {}
registration = custom_open_id_connect_providers[provider_name]["registration"]

if client_secret_setting_name is not None:
if client_secret_setting_name is not None or client_secret is not None:
if "clientCredential" not in custom_open_id_connect_providers[provider_name]["registration"].keys():
custom_open_id_connect_providers[provider_name]["registration"]["clientCredential"] = {}

Expand All @@ -821,6 +918,15 @@ def update_openid_connect_provider_settings(cmd, resource_group_name, name, prov
registration["clientId"] = client_id
if client_secret_setting_name is not None:
registration["clientCredential"]["clientSecretSettingName"] = client_secret_setting_name
if client_secret is not None:
provider_name_prefix = provider_name.upper()
mkarmark marked this conversation as resolved.
Show resolved Hide resolved
if len(provider_name_prefix) > 32:
provider_name_prefix = provider_name_prefix[0:31]
final_client_secret_setting_name = provider_name_prefix + "_PROVIDER_AUTHENTICATION_SECRET"
registration["clientSecretSettingName"] = final_client_secret_setting_name
settings = []
settings.append(final_client_secret_setting_name + '=' + client_secret)
update_app_settings(cmd, resource_group_name, name, settings, slot)
if openid_configuration is not None:
registration["openIdConnectConfiguration"]["wellKnownOpenIdConfiguration"] = openid_configuration
if scopes is not None:
Expand Down
Loading