SSO: Add GitHub auth configuration page #78933

Clarity-89 · 2023-12-01T07:16:12Z

What is this feature?

Enable configuring SSO for GitHub from the UI.

Which issue(s) does this PR fix?:

Part of https://github.com/grafana/identity-access-team/issues/500.

Special notes for your reviewer:

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Clarity-89 · 2023-12-01T07:17:17Z

public/app/core/components/FormPrompt/FormPrompt.tsx

@@ -0,0 +1,117 @@
+import { css } from '@emotion/css';


This is a copy of the component from Enterprise. It'd be removed from there and this version should be used instead.

Clarity-89 · 2023-12-20T15:50:50Z

/deploy-to-hg

ephemeral-instances-bot · 2023-12-20T15:51:33Z

Preparing your instance. A comment containing your instance's url will be added to this PR when the instance is ready.
Your instance will be ready in ~10 minutes.
Check the GitHub actions tab to follow the workflow progress
Slack channel: #proj-ephemeral-hg-instances
Building instance with sso/advanced-auth oss branch and main enterprise branch. How to choose a branch

ephemeral-instances-bot · 2023-12-20T16:01:55Z

Your instance can be accessed at: https://ephemeral1511182178933clarity.grafana-dev.net
The instance is not using the CDN assets.
How to access / How to update instance config / How to build a specific branch

# Conflicts: # docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md

dmihai

The UI for configuring the SSO settings for providers looks good, I only found a couple of small issues while testing locally:

When clicking on a provider from the /admin/authentication page, the UI can load the provider data using the get provider endpoint (for github: /api/v1/sso-settings/github) instead of list all providers endpoint (api/v1/sso-settings)
In the provider form page (admin/authentication/advanced/github), if the user clicks on any field without modifying its value then the form becomes "dirty" and clicking on Discard will show the modal with the 2 options: Continue editing and Discard unsaved changes. I think the form should become dirty only after a value has been modified.
After saving the changes from a provider config page (admin/authentication/advanced/github) the notification Settings saved appears but then the user cannot directly navigate to a different page because the modal Leave page? Changes that you made may not be saved. with 2 options (leave page, discard changes) appears. This may happen because UI expects PUT api/v1/sso-settings/github to respond with 204 No Content instead of 200 OK (I modified this while testing locally).
When the user changes the config for a provider, The UI should send only the fields that the user can change. For example, for github those fields are: clientId, clientSecret, teamIds, allowedOrganizations. Now the UI also sends fields that cannot be modified by the user, for example: tlsClientCa, hostedDomain.

public/app/features/auth-config/fields.ts

public/app/features/auth-config/types.ts

public/app/features/auth-config/ProviderConfigPage.tsx

Clarity-89 · 2023-12-21T10:32:45Z

When the user changes the config for a provider, The UI should send only the fields that the user can change. For example, for github those fields are: clientId, clientSecret, teamIds, allowedOrganizations. Now the UI also sends fields that cannot be modified by the user, for example: tlsClientCa, hostedDomain.

This was specifically requested, so that's why it works the way it is.

dmihai · 2023-12-21T10:36:50Z

When the user changes the config for a provider, The UI should send only the fields that the user can change. For example, for github those fields are: clientId, clientSecret, teamIds, allowedOrganizations. Now the UI also sends fields that cannot be modified by the user, for example: tlsClientCa, hostedDomain.

This was specifically requested, so that's why it works the way it is.

Okay, lets keep the current behaviour for now and see later if we want to change it.

Clarity-89 · 2023-12-21T11:18:07Z

After saving the changes from a provider config page (admin/authentication/advanced/github) the notification Settings saved appears but then the user cannot directly navigate to a different page because the modal Leave page? Changes that you made may not be saved. with 2 options (leave page, discard changes) appears. This may happen because UI expects PUT api/v1/sso-settings/github to respond with 204 No Content instead of 200 OK (I modified this while testing locally).

I thought that part was not ready yet? When I try to submit a form for any provider I get Bad request error and see this in the logs:

ERROR[12-21|13:14:39] Failed to parse request body logger=context userId=1 orgId=1 uname=admin error="json: cannot unmarshal string into Go struct field .source of type models.SettingsSource" remote_addr=127.0.0.1 traceID=

Clarity-89 · 2023-12-21T11:57:07Z

After saving the changes from a provider config page (admin/authentication/advanced/github) the notification Settings saved appears but then the user cannot directly navigate to a different page because the modal Leave page? Changes that you made may not be saved. with 2 options (leave page, discard changes) appears. This may happen because UI expects PUT api/v1/sso-settings/github to respond with 204 No Content instead of 200 OK (I modified this while testing locally).

I thought that part was not ready yet? When I try to submit a form for any provider I get Bad request error and see this in the logs:
ERROR[12-21|13:14:39] Failed to parse request body logger=context userId=1 orgId=1 uname=admin error="json: cannot unmarshal string into Go struct field .source of type models.SettingsSource" remote_addr=127.0.0.1 traceID=

I've figured out how to fix this without a working API.

dmihai

Tested and it works/looks great. Great work!

dmihai · 2023-12-21T12:50:43Z

public/app/features/auth-config/state/actions.ts

  return async (dispatch) => {
    if (!config.featureToggles.ssoSettingsApi) {
      return [];
    }
-    const result = await getBackendSrv().get('/api/v1/sso-settings');
-    dispatch(providersLoaded(result));
+    const result = await getBackendSrv().get(`/api/v1/sso-settings/${provider}`);


If provider is empty and we want the list of all providers then the URL should not contain the ending /. So for requesting the list of providers this should be used /api/v1/sso-settings instead of /api/v1/sso-settings/. If we include the / at the end then the backend will interpret it as a single provider request with an empty provider name (and it will return an error).

Good catch, I'll fix it.

Clarity-89 added 10 commits November 30, 2023 13:13

Setup route

26f1079

Set up the page

3015efb

Add orgs

e26020e

Load settings

0aa29a5

Make API call

81c0289

Remove log

e9e814b

Add FormPrompt

3e37cd8

Update types

5847e8e

Add tests

aa3e8de

Fix tests

f946e38

Clarity-89 added add to changelog no-backport Skip backport of PR labels Dec 1, 2023

Clarity-89 self-assigned this Dec 1, 2023

grafana-delivery-bot bot added this to the 10.3.x milestone Dec 1, 2023

grafana-pr-automation bot added the area/frontend label Dec 1, 2023

Clarity-89 commented Dec 1, 2023

View reviewed changes

Clarity-89 added 5 commits December 1, 2023 09:23

Cleanup

487f8ae

Load settings

da97d8e

Fix naming

0ad283f

Switch to PUT endpoint

528e9cc

Switch to CSS object

2a77c92

Clarity-89 marked this pull request as ready for review December 1, 2023 08:19

Clarity-89 requested review from a team as code owners December 1, 2023 08:19

Clarity-89 requested review from tskarhed, L-M-K-B and mgyongyosi and removed request for a team, tskarhed and L-M-K-B December 1, 2023 08:19

Clarity-89 added 4 commits December 20, 2023 16:34

Merge branch 'main' into sso/advanced-auth

f8726d5

Update fields logic

fc1fc87

Update tests

5db4c8f

Update betterer

16c2f42

Merge branch 'main' into sso/advanced-auth

10db9bd

# Conflicts: # docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md

dmihai reviewed Dec 21, 2023

View reviewed changes

public/app/features/auth-config/fields.ts Outdated Show resolved Hide resolved

public/app/features/auth-config/types.ts Outdated Show resolved Hide resolved

public/app/features/auth-config/ProviderConfigPage.tsx Show resolved Hide resolved

Clarity-89 added 5 commits December 21, 2023 12:40

SSO: Make client secret not required

587522f

Update field name

0e3ca6b

Revert feature toggle to dev mode

0bd2a8e

Use provider endpoint

eb682ec

Fix form state check

fa182f2

Clarity-89 added 4 commits December 21, 2023 13:19

Update tests

f2c496b

Fix URL redirect after form submit

e31e8b0

Mock locationService

8d2b949

Separate Form component

50e081c

Clarity-89 removed request for grafanabot and a team December 21, 2023 12:06

dmihai approved these changes Dec 21, 2023

View reviewed changes

Fix trailing slash

34d6765

Clarity-89 merged commit fde8a00 into main Dec 21, 2023
18 checks passed

Clarity-89 deleted the sso/advanced-auth branch December 21, 2023 13:26

summerwollin modified the milestones: 10.3.x, 10.3.0 Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSO: Add GitHub auth configuration page #78933

SSO: Add GitHub auth configuration page #78933

Clarity-89 commented Dec 1, 2023

Clarity-89 Dec 1, 2023

Clarity-89 commented Dec 20, 2023

ephemeral-instances-bot bot commented Dec 20, 2023

ephemeral-instances-bot bot commented Dec 20, 2023

dmihai left a comment

Clarity-89 commented Dec 21, 2023

dmihai commented Dec 21, 2023

Clarity-89 commented Dec 21, 2023

Clarity-89 commented Dec 21, 2023

dmihai left a comment

dmihai Dec 21, 2023

Clarity-89 Dec 21, 2023

SSO: Add GitHub auth configuration page #78933

SSO: Add GitHub auth configuration page #78933

Conversation

Clarity-89 commented Dec 1, 2023

Clarity-89 Dec 1, 2023

Choose a reason for hiding this comment

Clarity-89 commented Dec 20, 2023

ephemeral-instances-bot bot commented Dec 20, 2023

ephemeral-instances-bot bot commented Dec 20, 2023

dmihai left a comment

Choose a reason for hiding this comment

Clarity-89 commented Dec 21, 2023

dmihai commented Dec 21, 2023

Clarity-89 commented Dec 21, 2023

Clarity-89 commented Dec 21, 2023

dmihai left a comment

Choose a reason for hiding this comment

dmihai Dec 21, 2023

Choose a reason for hiding this comment

Clarity-89 Dec 21, 2023

Choose a reason for hiding this comment