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.

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

User identity authentication in configuration UI #569

Merged
merged 8 commits into from
May 26, 2023

Conversation

kostrse
Copy link
Contributor

@kostrse kostrse commented Apr 22, 2023

This PR adds "Current User" as an authentication option for the datasource.

Current User authentication

The option is available to the user only if user identity authentication was enabled in Grafana config (see grafana/grafana#50277). It can be enabled as default via feature flag adxUserIdentityPreferred .

Fixes #381

@kostrse kostrse requested a review from a team as a code owner April 22, 2023 07:48
@kostrse kostrse changed the title User auth configuration UI User identity authentication in configuration UI Apr 22, 2023
Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

Initial testing on this with grafana/grafana-azure-sdk-go#32 and grafana/grafana#50277 has been unsuccessful. With the necessary updates to appropriately configure user-based auth there are some issues:

  • The default auth is set to Current User, however with the clusterUrl value set the schema cannot be reloaded until the auth type option has been reselected.
  • Attempting to reload the schema leads to the error: invalid Azure configuration: user identity authentication is not supported by this datasource

The only configuration I've added is to set user_auth_enabled to true. All other parameters needed should be pulled from the existing auth.azuread configuration for OBO.

Additionally, setting the authType to Current User should present the user with an alert describing the feature as experimental and highlighting that certain features will be broken, particularly alerting. An InlineBanner component should suffice for the details here.

@kostrse
Copy link
Contributor Author

kostrse commented May 3, 2023

  • The default auth is set to Current User, however with the clusterUrl value set the schema cannot be reloaded until the auth type option has been reselected.

I will double check, if there's some UI-specific bug in the behavior of the form, I will see what can be done.

  • Attempting to reload the schema leads to the error: invalid Azure configuration: user identity authentication is not supported by this datasource

I think because you need this in httpclient.go (after this line):

	// Allow user identity authentication for this datasource if it's configured in Grafana config
	authOpts.AllowUserIdentity()

Datasource needs to explicitly enable support for user identity auth. This is security precaution, e.g. if a datasource isn't designed to properly isolate cache etc., then user identity auth should not be possible.

Plus add 3rd argument 'false' to token_provider_azure.go to make Grafana core compile (here).

These changes are not included into existing PRs because they need to be introduced in correct order and will be added later.

The only configuration I've added is to set user_auth_enabled to true. All other parameters needed should be pulled from the existing auth.azuread configuration for OBO.

If you add the authOpts.AllowUserIdentity() above, only user_auth_enabled to true should be enough.

Additionally, setting the authType to Current User should present the user with an alert describing the feature as experimental and highlighting that certain features will be broken, particularly alerting. An InlineBanner component should suffice for the details here.

I will add alert message.

I hope you should be able to make it work end to end.

Each PR is self-containing and responsible for specific parts, so we can get back to this UI PR later, after we finish the backend part.

The first PR which I want merge is this one:
grafana/grafana-azure-sdk-go#32

This will allow me to release a new version of SDK

@kostrse
Copy link
Contributor Author

kostrse commented May 16, 2023

Added the experimental banner and documentation page.

JsonData:

{
    "azureCredentials":
    {
        "authType": "currentuser"
    },
    "clusterUrl": "https://example.kusto.windows.net",
    "dataConsistency": "strongconsistency",
    "defaultEditorMode": "visual",
    "oauthPassThru": true
}

Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

I've added some additional comments on the wording. Ideally, we'd like to be as specific as possible with what may or may not work right now so that we don't confuse users and cause ourselves to receive unexpected issue reports.

Also, the form still doesn't behave as expected because the hasCredentials function needs to be updated to take into account the fact that current user auth does not require any additional properties 😊

doc/current-user-auth.md Outdated Show resolved Hide resolved
src/components/ConfigEditor/AzureCredentialsForm.tsx Outdated Show resolved Hide resolved
kostrse and others added 2 commits May 23, 2023 03:37
Co-authored-by: Andreas Christou <andreas.christou@grafana.com>
Co-authored-by: Andreas Christou <andreas.christou@grafana.com>
@kostrse
Copy link
Contributor Author

kostrse commented May 23, 2023

Also, the form still doesn't behave as expected because the hasCredentials function needs to be updated to take into account the fact that current user auth does not require any additional properties

Logic of hasCredentials from here:

    typeof options.jsonData.azureCredentials === 'object' ||
    (typeof options.jsonData.tenantId === 'string' && typeof options.jsonData.clientId === 'string')

If azureCredentials object exists (of any type, including user auth), then credentials are set, otherwise check the legacy scenario when client fields were set directly in the root of JsonData, not in azureCredentials object.

This considered as having credentials:

{
  "azureCredentials": {
    "..."
  }
}

@aangelisc
Copy link
Contributor

Yep I checked the logic beforehand. The problem is when a user creates a new datasource the current user auth method is the default method but it is not actually set in the jsonData value which causes the reload button to be disabled.

@kostrse
Copy link
Contributor Author

kostrse commented May 23, 2023

Yep I checked the logic beforehand. The problem is when a user creates a new datasource the current user auth method is the default method but it is not actually set in the jsonData value which causes the reload button to be disabled.

This bug is not specific to user auth.

# Conflicts:
#	src/components/ConfigEditor/index.tsx
@aangelisc aangelisc merged commit 0dfbf0a into grafana:main May 26, 2023
4 checks passed
@kostrse kostrse deleted the user-auth-credentials-ui branch August 16, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add user identity authentication option to configuration UI
2 participants