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

Jump to bottom

Request management token via Azure CLI only for Service Principals and not human users #408

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Request management token via Azure CLI only for Service Principals and not human users #408

merged 1 commit into from
Nov 8, 2023

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Oct 17, 2023 •
edited
Loading

Changes

This PR removes the call to az account get-access-token to retrieve the management token if the machine is authenticated with a non-human user.

Tests

  • make test run locally
  • make fmt applied
  • relevant integration tests applied

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2023 •
edited
Loading

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Files Coverage Δ
databricks/sdk/core.py 81.42% <93.33%> (+0.07%) ⬆️
databricks/sdk/oauth.py 55.51% <42.85%> (-0.67%) ⬇️

📢 Thoughts on this report? Let us know!

@nfx
Copy link
Contributor Author

nfx commented Oct 17, 2023

Verified this by decoding claims from databricks/databricks-sdk-go#659

mgyucht
mgyucht reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Very nicely documented, thanks for this. Main comment is about the choice of field used to determine UPN vs SPN auth. For my understanding, is there a functional issue with the current implementation that this addresses, or is it just to reduce the unnecessary call to the Azure CLI?

databricks/sdk/core.py
guaranteed to be unique within a tenant and should be used only for display purposes.
- 'upn' - The username of the user.
"""
return 'upn' in self.token().jwt_claims()
Copy link
Contributor

Choose a reason for hiding this comment

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

upn is an optional field, so we can't count on it always being present in JWT claims, specifically v2.0 tokens: https://learn.microsoft.com/en-us/azure/active-directory/develop/access-token-claims-reference#v10-basic-claims.

Let's use the presence of scp instead, which according to the docs is only present for user tokens. https://learn.microsoft.com/en-us/azure/active-directory/develop/access-token-claims-reference#payload-claims:~:text=The%20set%20of%20scopes%20exposed%20by%20the%20application%20for%20which%20the%20client%20application%20has%20requested%20(and%20received)%20consent.%20Only%20included%20for%20user%20tokens.

@nfx
Request management token via Azure CLI only for Service Principals an…
34e509b 
…d not human users

This PR removes the call to `az account get-access-token` to retrieve management token in case the machine is authenticated with a non-human user.
@nfx nfx force-pushed the fix/azure-cli-mgmt-token-for-upns-only branch from f013b68 to 34e509b Compare November 7, 2023 22:54
@nfx nfx added this pull request to the merge queue Nov 8, 2023
Merged via the queue into main with commit a862adc Nov 8, 2023
9 checks passed
@nfx nfx deleted the fix/azure-cli-mgmt-token-for-upns-only branch November 8, 2023 16:57
mgyucht added a commit that referenced this pull request Nov 14, 2023
@mgyucht
Release v0.13.0
9747f59 
* Introduce more specific exceptions, like `NotFound`, `AlreadyExists`, `BadRequest`, `PermissionDenied`, `InternalError`, and others ([#376](#376)). This makes it easier to handle errors thrown by the Databricks API. Instead of catching `DatabricksError` and checking the error_code field, you can catch one of these subtypes of `DatabricksError`, which is more ergonomic and removes the need to rethrow exceptions that you don't want to catch. For example:
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except DatabricksError as e:
  if e.error_code in [
    "RESOURCE_DOES_NOT_EXIST",
    "RESOURCE_NOT_FOUND",
    "PERMISSION_DENIED",
    "FEATURE_DISABLED",
    "BAD_REQUEST"]:
    logger.warning(...)
    return None
  raise RetryableError(...) from e
```
can be replaced with
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except PermissionDenied, FeatureDisabled:
  logger.warning(...)
  return None
except NotFound:
  raise RetryableError(...)
```
* Paginate all SCIM list requests in the SDK ([#440](#440)). This change ensures that SCIM list() APIs use a default limit of 100 resources, leveraging SCIM's offset + limit pagination to batch requests to the Databricks API.
* Added taskValues support in remoteDbUtils ([#406](#406)).
* Added more detailed error message on default credentials not found error ([#419](#419)).
* Request management token via Azure CLI only for Service Principals and not human users ([#408](#408)).

API Changes:

 * Fixed `create()` method for [w.functions](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/functions.html) workspace-level service and corresponding `databricks.sdk.service.catalog.CreateFunction` and `databricks.sdk.service.catalog.FunctionInfo` dataclasses.
 * Changed `create()` method for [w.metastores](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/metastores.html) workspace-level service with new required argument order.
 * Changed `storage_root` field for `databricks.sdk.service.catalog.CreateMetastore` to be optional.
 * Added `skip_validation` field for `databricks.sdk.service.catalog.UpdateExternalLocation`.
 * Added `libraries` field for `databricks.sdk.service.compute.CreatePolicy`, `databricks.sdk.service.compute.EditPolicy` and `databricks.sdk.service.compute.Policy`.
 * Added `init_scripts` field for `databricks.sdk.service.compute.EventDetails`.
 * Added `file` field for `databricks.sdk.service.compute.InitScriptInfo`.
 * Added `zone_id` field for `databricks.sdk.service.compute.InstancePoolGcpAttributes`.
 * Added several dataclasses related to init scripts.
 * Added `databricks.sdk.service.compute.LocalFileInfo` dataclass.
 * Replaced `ui_state` field with `edit_mode` for `databricks.sdk.service.jobs.CreateJob` and `databricks.sdk.service.jobs.JobSettings`.
 * Replaced `databricks.sdk.service.jobs.CreateJobUiState` dataclass with `databricks.sdk.service.jobs.CreateJobEditMode`.
 * Added `include_resolved_values` field for `databricks.sdk.service.jobs.GetRunRequest`.
 * Replaced `databricks.sdk.service.jobs.JobSettingsUiState` dataclass with `databricks.sdk.service.jobs.JobSettingsEditMode`.
 * Removed [a.o_auth_enrollment](https://databricks-sdk-py.readthedocs.io/en/latest/account/o_auth_enrollment.html) account-level service. This was only used to aid in OAuth enablement during the public preview of OAuth. OAuth is now enabled for all AWS E2 accounts, so usage of this API is no longer needed.
 * Added `network_connectivity_config_id` field for `databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
 * Added [a.network_connectivity](https://databricks-sdk-py.readthedocs.io/en/latest/account/network_connectivity.html) account-level service.
 * Added `string_shared_as` field for `databricks.sdk.service.sharing.SharedDataObject`.

Internal changes:

* Added regression question to issue template ([#414](#414)).
* Made test_auth no longer fail if you have a default profile setup ([#426](#426)).

OpenAPI SHA: d136ad0541f036372601bad9a4382db06c3c912d, Date: 2023-11-14
@mgyucht mgyucht mentioned this pull request Nov 14, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 14, 2023
@mgyucht
Release v0.13.0 (#442)
86ca043 
* Introduce more specific exceptions, like `NotFound`, `AlreadyExists`,
`BadRequest`, `PermissionDenied`, `InternalError`, and others
([#376](#376)). This
makes it easier to handle errors thrown by the Databricks API. Instead
of catching `DatabricksError` and checking the error_code field, you can
catch one of these subtypes of `DatabricksError`, which is more
ergonomic and removes the need to rethrow exceptions that you don't want
to catch. For example:
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except DatabricksError as e:
  if e.error_code in [
    "RESOURCE_DOES_NOT_EXIST",
    "RESOURCE_NOT_FOUND",
    "PERMISSION_DENIED",
    "FEATURE_DISABLED",
    "BAD_REQUEST"]:
    logger.warning(...)
    return None
  raise RetryableError(...) from e
```
can be replaced with
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except PermissionDenied, FeatureDisabled:
  logger.warning(...)
  return None
except NotFound:
  raise RetryableError(...)
```
* Paginate all SCIM list requests in the SDK
([#440](#440)). This
change ensures that SCIM list() APIs use a default limit of 100
resources, leveraging SCIM's offset + limit pagination to batch requests
to the Databricks API.
* Added taskValues support in remoteDbUtils
([#406](#406)).
* Added more detailed error message on default credentials not found
error
([#419](#419)).
* Request management token via Azure CLI only for Service Principals and
not human users
([#408](#408)).

API Changes:

* Fixed `create()` method for
[w.functions](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/functions.html)
workspace-level service and corresponding
`databricks.sdk.service.catalog.CreateFunction` and
`databricks.sdk.service.catalog.FunctionInfo` dataclasses.
* Changed `create()` method for
[w.metastores](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/metastores.html)
workspace-level service with new required argument order.
* Changed `storage_root` field for
`databricks.sdk.service.catalog.CreateMetastore` to be optional.
* Added `skip_validation` field for
`databricks.sdk.service.catalog.UpdateExternalLocation`.
* Added `libraries` field for
`databricks.sdk.service.compute.CreatePolicy`,
`databricks.sdk.service.compute.EditPolicy` and
`databricks.sdk.service.compute.Policy`.
* Added `init_scripts` field for
`databricks.sdk.service.compute.EventDetails`.
* Added `file` field for
`databricks.sdk.service.compute.InitScriptInfo`.
* Added `zone_id` field for
`databricks.sdk.service.compute.InstancePoolGcpAttributes`.
 * Added several dataclasses related to init scripts.
 * Added `databricks.sdk.service.compute.LocalFileInfo` dataclass.
* Replaced `ui_state` field with `edit_mode` for
`databricks.sdk.service.jobs.CreateJob` and
`databricks.sdk.service.jobs.JobSettings`.
* Replaced `databricks.sdk.service.jobs.CreateJobUiState` dataclass with
`databricks.sdk.service.jobs.CreateJobEditMode`.
* Added `include_resolved_values` field for
`databricks.sdk.service.jobs.GetRunRequest`.
* Replaced `databricks.sdk.service.jobs.JobSettingsUiState` dataclass
with `databricks.sdk.service.jobs.JobSettingsEditMode`.
* Removed
[a.o_auth_enrollment](https://databricks-sdk-py.readthedocs.io/en/latest/account/o_auth_enrollment.html)
account-level service. This was only used to aid in OAuth enablement
during the public preview of OAuth. OAuth is now enabled for all AWS E2
accounts, so usage of this API is no longer needed.
* Added `network_connectivity_config_id` field for
`databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
* Added
[a.network_connectivity](https://databricks-sdk-py.readthedocs.io/en/latest/account/network_connectivity.html)
account-level service.
* Added `string_shared_as` field for
`databricks.sdk.service.sharing.SharedDataObject`.

Internal changes:

* Added regression question to issue template
([#414](#414)).
* Made test_auth no longer fail if you have a default profile setup
([#426](#426)).

OpenAPI SHA: d136ad0541f036372601bad9a4382db06c3c912d, Date: 2023-11-14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgyucht mgyucht mgyucht left review comments

@hectorcast-db hectorcast-db hectorcast-db approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nfx @codecov-commenter @mgyucht @hectorcast-db