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

Auth Drawer: Use redux store to load settings #85110

Merged
merged 6 commits into from Mar 26, 2024
+64 −38
Merged

Auth Drawer: Use redux store to load settings #85110

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ae3089
wip
linoman Mar 25, 2024
b0d9ce2
wip
linoman Mar 25, 2024
b7c371b
Merge remote-tracking branch 'origin/main' into linoman/auth_drawer-u…
linoman Mar 26, 2024
ad33e09
use actions instead of importing the backend service
linoman Mar 26, 2024
75ff1cc
Replace the test render for redux-rtl
linoman Mar 26, 2024
d8c4758
turn promises into async calls
linoman Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions public/app/features/auth-config/AuthDrawer.test.tsx
View file
Open in desktop
@@ -1,17 +1,21 @@
import { render, screen } from '@testing-library/react';
import { screen } from '@testing-library/react';
import React from 'react';
import { render } from 'test/redux-rtl';

import { AuthDrawer, Props } from './AuthDrawer';
import { AuthDrawerUnconnected, Props } from './AuthDrawer';

const defaultProps: Props = {
onClose: jest.fn(),
allowInsecureEmail: false,
loadSettings: jest.fn(),
saveSettings: jest.fn(),
};

async function getTestContext(overrides: Partial<Props> = {}) {
jest.clearAllMocks();

const props = { ...defaultProps, ...overrides };
const { rerender } = render(<AuthDrawer {...props} />);
const { rerender } = render(<AuthDrawerUnconnected {...props} />);

return { rerender, props };
}
Expand Down
90 changes: 51 additions & 39 deletions public/app/features/auth-config/AuthDrawer.tsx
View file
Open in desktop
@@ -1,53 +1,65 @@
import { css } from '@emotion/css';
import React, { useState } from 'react';
import React, { JSX } from 'react';
import { connect, ConnectedProps } from 'react-redux';

import { GrafanaTheme2 } from '@grafana/data';
import { getBackendSrv } from '@grafana/runtime';
import { Button, Drawer, Text, TextLink, Switch, useStyles2 } from '@grafana/ui';
import { useAppNotification } from 'app/core/copy/appNotification';
import { StoreState } from 'app/types';

export interface Props {
import { loadSettings, saveSettings } from './state/actions';

interface OwnProps {
onClose: () => void;
}

const SETTINGS_URL = '/api/admin/settings';

export const AuthDrawer = ({ onClose }: Props) => {
const [isOauthAllowInsecureEmailLookup, setOauthAllowInsecureEmailLookup] = useState(false);
export type Props = OwnProps & ConnectedProps<typeof connector>;

const getSettings = async () => {
try {
const response = await getBackendSrv().get(SETTINGS_URL);
setOauthAllowInsecureEmailLookup(response.auth.oauth_allow_insecure_email_lookup?.toLowerCase?.() === 'true');
} catch (error) {}
};
const updateSettings = async (property: boolean) => {
try {
const body = {
updates: {
auth: {
oauth_allow_insecure_email_lookup: '' + property,
},
},
};
await getBackendSrv().put(SETTINGS_URL, body);
} catch (error) {}
const mapStateToProps = (state: StoreState) => {
const allowInsecureEmail =
state.authConfig.settings?.auth?.oauth_allow_insecure_email_lookup.toLowerCase() === 'true';
return {
allowInsecureEmail,
};
};

const resetButtonOnClick = async () => {
try {
const body = {
removals: {
auth: ['oauth_allow_insecure_email_lookup'],
const mapActionsToProps = {
loadSettings,
saveSettings,
};

const connector = connect(mapStateToProps, mapActionsToProps);

export const AuthDrawerUnconnected = ({
allowInsecureEmail,
loadSettings,
onClose,
saveSettings,
}: Props): JSX.Element => {
const notifyApp = useAppNotification();

const oauthAllowInsecureEmailLookupOnChange = async () => {
saveSettings({
updates: {
auth: {
oauth_allow_insecure_email_lookup: '' + !allowInsecureEmail,
},
};
await getBackendSrv().put(SETTINGS_URL, body);
getSettings();
} catch (error) {}
},
})
.then(() => loadSettings(false))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend using async/await approach, it produces much clearer code.

try {
  await saveSettings()
  await loadSettings(false)
  notifyApp.success('Settings saved')
} catch (error) {
  notifyApp.error('Failed to save settings')
}

You also could put loadSettings() call into saveSettings() action if you always call it after (I think you in most of cases need to re-fetch settings after saving it to display updated values).

Copy link
Contributor Author

@linoman linoman Mar 26, 2024 •
edited

Choose a reason for hiding this comment

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

I prefer promises over async calls, but this is just me. I have changed the code as per your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided not to include loadSettings inside saveSettings because I'm aware they're being used by the old LDAP and SAML UI. I rather keep this clean for now.

Do you think is necessary?

.then(() => notifyApp.success('Settings saved'))
.catch(() => notifyApp.error('Failed to save settings'));
};

const oauthAllowInsecureEmailLookupOnChange = async () => {
updateSettings(!isOauthAllowInsecureEmailLookup);
setOauthAllowInsecureEmailLookup(!isOauthAllowInsecureEmailLookup);
const resetButtonOnClick = async () => {
saveSettings({
removals: {
auth: ['oauth_allow_insecure_email_lookup'],
},
})
.then(() => loadSettings(false))
.then(() => notifyApp.success('Settings saved'))
.catch(() => notifyApp.error('Failed to save settings'));
};

const subtitle = (
Expand All @@ -65,8 +77,6 @@ export const AuthDrawer = ({ onClose }: Props) => {

const styles = useStyles2(getStyles);

getSettings();

return (
<Drawer title="Auth Settings" subtitle={subtitle} size="md" onClose={onClose}>
<div className={styles.advancedAuth}>
Expand All @@ -75,7 +85,7 @@ export const AuthDrawer = ({ onClose }: Props) => {
<Text variant="body" color="secondary">
Allow users to use the same email address to log into Grafana with different identity providers.
</Text>
<Switch value={isOauthAllowInsecureEmailLookup} onChange={oauthAllowInsecureEmailLookupOnChange} />
<Switch value={allowInsecureEmail} onChange={oauthAllowInsecureEmailLookupOnChange} />
</div>
<Button
size="md"
Expand All @@ -90,6 +100,8 @@ export const AuthDrawer = ({ onClose }: Props) => {
);
};

export default connector(AuthDrawerUnconnected);

const getStyles = (theme: GrafanaTheme2) => {
return {
advancedAuth: css({
Expand Down
2 changes: 1 addition & 1 deletion public/app/features/auth-config/AuthProvidersListPage.tsx
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ import { Page } from 'app/core/components/Page/Page';
import { config } from 'app/core/config';
import { StoreState } from 'app/types';

import { AuthDrawer } from './AuthDrawer';
import AuthDrawer from './AuthDrawer';
import ConfigureAuthCTA from './components/ConfigureAuthCTA';
import { ProviderCard } from './components/ProviderCard';
import { loadSettings } from './state/actions';
Expand Down
10 changes: 7 additions & 3 deletions public/app/features/auth-config/state/actions.ts
View file
Open in desktop
Expand Up @@ -17,15 +17,19 @@ import {
settingsUpdated,
} from './reducers';

export function loadSettings(): ThunkResult<Promise<Settings>> {
export function loadSettings(showSpinner = true): ThunkResult<Promise<Settings>> {
return async (dispatch) => {
if (contextSrv.hasPermission(AccessControlAction.SettingsRead)) {
dispatch(loadingBegin());
if (showSpinner) {
dispatch(loadingBegin());
}
dispatch(loadProviders());
const result = await getBackendSrv().get('/api/admin/settings');
dispatch(settingsUpdated(result));
await dispatch(loadProviderStatuses());
dispatch(loadingEnd());
if (showSpinner) {
dispatch(loadingEnd());
}
return result;
}
};
Expand Down