Skip to content

Commit

Permalink
feat: restrict editing certain providers to global staff (openedx#176)
Browse files Browse the repository at this point in the history
This change allows certain LTI-based providers to be disabled for editing by regular course admins/staff. In some cases, there are special configuration needs that require that the provider be configured by a global staff user.
  • Loading branch information
xitij2000 authored Aug 6, 2021
1 parent 0a33523 commit b13bd69
Show file tree
Hide file tree
Showing 14 changed files with 160 additions and 78 deletions.
1 change: 1 addition & 0 deletions .env
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ PUBLISHER_BASE_URL=
REFRESH_ACCESS_TOKEN_ENDPOINT=null
SEGMENT_KEY=null
SITE_NAME=null
SUPPORT_EMAIL=null
SUPPORT_URL=null
USER_INFO_COOKIE_NAME=null
1 change: 1 addition & 0 deletions .env.development
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ REFRESH_ACCESS_TOKEN_ENDPOINT='http://localhost:18000/login_refresh'
SEGMENT_KEY=null
SITE_NAME='edX'
STUDIO_BASE_URL='http://localhost:18010'
SUPPORT_EMAIL='[email protected]'
SUPPORT_URL='https://support.edx.org'
USER_INFO_COOKIE_NAME='edx-user-info'
1 change: 1 addition & 0 deletions .env.test
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ REFRESH_ACCESS_TOKEN_ENDPOINT='http://localhost:18000/login_refresh'
SEGMENT_KEY=null
SITE_NAME='edX'
STUDIO_BASE_URL='http://localhost:18010'
SUPPORT_EMAIL='[email protected]'
SUPPORT_URL='https://support.edx.org'
USER_INFO_COOKIE_NAME='edx-user-info'
1 change: 1 addition & 0 deletions src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ initialize({
config: () => {
mergeConfig({
SUPPORT_URL: process.env.SUPPORT_URL || null,
SUPPORT_EMAIL: process.env.SUPPORT_EMAIL || null,
CALCULATOR_HELP_URL: process.env.CALCULATOR_HELP_URL || null,
}, 'CourseAuthoringConfig');
},
Expand Down
75 changes: 60 additions & 15 deletions src/pages-and-resources/discussions/DiscussionsSettings.test.jsx
Original file line number Diff line number Diff line change
@@ -1,32 +1,31 @@
import React from 'react';

import {
getConfig, history, initializeMockApp, setConfig,
} from '@edx/frontend-platform';
import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
import { AppProvider, PageRoute } from '@edx/frontend-platform/react';
import {
act,
queryByLabelText,
queryByRole,
queryByTestId,
queryByText,
render,
screen,
waitForElementToBeRemoved,
queryByRole,
waitFor,
waitForElementToBeRemoved,
} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { AppProvider, PageRoute } from '@edx/frontend-platform/react';
import { Switch } from 'react-router';

import {
getConfig, history, initializeMockApp, setConfig,
} from '@edx/frontend-platform';
import MockAdapter from 'axios-mock-adapter';
import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
import DiscussionsSettings from './DiscussionsSettings';
import PagesAndResourcesProvider from '../PagesAndResourcesProvider';
import React from 'react';
import { Switch } from 'react-router';
import initializeStore from '../../store';
import { getAppsUrl } from './data/api';
import { piazzaApiResponse, legacyApiResponse } from './factories/mockApiResponses';
import PagesAndResourcesProvider from '../PagesAndResourcesProvider';
import appMessages from './app-config-form/messages';
import appListMessages from './app-list/messages';
import ltiMessages from './app-config-form/apps/lti/messages';
import { getAppsUrl } from './data/api';
import DiscussionsSettings from './DiscussionsSettings';
import { generatePiazzaApiResponse, legacyApiResponse, piazzaApiResponse } from './factories/mockApiResponses';

const courseId = 'course-v1:edX+TestX+Test_Course';
let axiosMock;
Expand Down Expand Up @@ -307,3 +306,49 @@ describe('DiscussionsSettings', () => {
});
});
});

describe.each([
{ isAdmin: false, isAdminOnlyConfig: false },
{ isAdmin: false, isAdminOnlyConfig: true },
{ isAdmin: true, isAdminOnlyConfig: false },
{ isAdmin: true, isAdminOnlyConfig: true },
])('LTI Admin only config test', ({ isAdmin, isAdminOnlyConfig }) => {
beforeEach(() => {
initializeMockApp({
authenticatedUser: {
userId: 3,
username: 'abc123',
administrator: isAdmin,
roles: [],
},
});

store = initializeStore();
axiosMock = new MockAdapter(getAuthenticatedHttpClient());

// Leave the DiscussionsSettings route after the test.
history.push(`/course/${courseId}/pages-and-resources`);
axiosMock.onGet(getAppsUrl(courseId)).reply(200, generatePiazzaApiResponse(isAdminOnlyConfig));
renderComponent();
});

test(`successfully advances to settings step for lti when adminOnlyConfig=${isAdminOnlyConfig} and user ${isAdmin ? 'is' : 'is not'} admin`, async () => {
const showLTIConfig = isAdmin || !isAdminOnlyConfig;
history.push(`/course/${courseId}/pages-and-resources/discussion`);

// This is an important line that ensures the spinner has been removed - and thus our main
// content has been loaded - prior to proceeding with our expectations.
await waitForElementToBeRemoved(screen.getByRole('status'));

userEvent.click(queryByLabelText(container, 'Select Piazza'));
userEvent.click(queryByText(container, appListMessages.nextButton.defaultMessage));

if (showLTIConfig) {
expect(queryByText(container, ltiMessages.formInstructions.defaultMessage)).toBeInTheDocument();
expect(queryByTestId(container, 'ltiConfigFields')).toBeInTheDocument();
} else {
expect(queryByText(container, ltiMessages.formInstructions.defaultMessage)).not.toBeInTheDocument();
expect(queryByTestId(container, 'ltiConfigFields')).not.toBeInTheDocument();
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function AppConfigForm({
app={app}
appConfig={appConfig}
onSubmit={handleSubmit}
title={intl.formatMessage(messages[`appName-${app.id}`])}
providerName={intl.formatMessage(messages[`appName-${app.id}`])}
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,26 @@
import React, { useEffect } from 'react';
import PropTypes from 'prop-types';

import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import {
Card, Form,
} from '@edx/paragon';
import { ensureConfig, getConfig } from '@edx/frontend-platform';
import { getAuthenticatedUser } from '@edx/frontend-platform/auth';
import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import { Card, Form, MailtoLink } from '@edx/paragon';

import { useFormik } from 'formik';
import * as Yup from 'yup';
import { useDispatch } from 'react-redux';
import * as Yup from 'yup';

import {
updateValidationStatus,
} from '../../../data/slice';
import { updateValidationStatus } from '../../../data/slice';
import AppExternalLinks from '../shared/AppExternalLinks';
import messages from './messages';

ensureConfig([
'SITE_NAME',
'SUPPORT_EMAIL',
], 'LTI Config Form');

function LtiConfigForm({
appConfig, app, onSubmit, intl, formRef, title,
appConfig, app, onSubmit, intl, formRef, providerName,
}) {
const ltiAppConfig = {
consumerKey: appConfig.consumerKey || '',
Expand All @@ -26,7 +29,7 @@ function LtiConfigForm({
piiShareUsername: appConfig.piiShareUsername,
piiShareEmail: appConfig.piiShareEmail,
};

const user = getAuthenticatedUser();
const dispatch = useDispatch();
const { externalLinks } = app;
const {
Expand All @@ -47,10 +50,11 @@ function LtiConfigForm({
}),
onSubmit,
});

const isInvalidConsumerKey = Boolean(touched.consumerKey && errors.consumerKey);
const isInvalidConsumerSecret = Boolean(touched.consumerSecret && errors.consumerSecret);
const isInvalidLaunchUrl = Boolean(touched.launchUrl && errors.launchUrl);
const supportEmail = getConfig().SUPPORT_EMAIL;
const showLTIConfig = user.administrator || !app.adminOnlyConfig;

useEffect(() => {
dispatch(updateValidationStatus({ hasError: Object.keys(errors).length > 0 }));
Expand All @@ -59,50 +63,68 @@ function LtiConfigForm({
return (
<Card className="mb-5 p-5" data-testid="ltiConfigForm">
<Form ref={formRef} onSubmit={handleSubmit}>
<h3 className="mb-3">{title}</h3>
<p>{intl.formatMessage(messages.formInstructions)}</p>
<h3 className="mb-3">{providerName}</h3>
<p>{showLTIConfig
? intl.formatMessage(messages.formInstructions)
: (
<FormattedMessage
{...messages.adminOnlyConfig}
values={{
providerName,
platformName: getConfig().SITE_NAME,
supportEmail: supportEmail
? <MailtoLink to={supportEmail}>{supportEmail}</MailtoLink>
: 'support',
}}
/>
)}
</p>
{app.messages && app.messages.map(msg => (
<p key={msg}>{msg}</p>
))}
<Form.Group controlId="consumerKey" isInvalid={isInvalidConsumerKey} className="mb-4">
<Form.Control
floatingLabel={intl.formatMessage(messages.consumerKey)}
onChange={handleChange}
onBlur={handleBlur}
value={values.consumerKey}
/>
{isInvalidConsumerKey && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<span className="x-small">{errors.consumerKey}</span>
</Form.Control.Feedback>
)}
</Form.Group>
<Form.Group controlId="consumerSecret" isInvalid={isInvalidConsumerSecret} className="mb-4">
<Form.Control
floatingLabel={intl.formatMessage(messages.consumerSecret)}
onChange={handleChange}
onBlur={handleBlur}
value={values.consumerSecret}
/>
{isInvalidConsumerSecret && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<span className="x-small">{errors.consumerSecret}</span>
</Form.Control.Feedback>
)}
</Form.Group>
<Form.Group controlId="launchUrl" isInvalid={isInvalidLaunchUrl}>
<Form.Control
floatingLabel={intl.formatMessage(messages.launchUrl)}
onChange={handleChange}
onBlur={handleBlur}
value={values.launchUrl}
/>
{isInvalidLaunchUrl && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<span className="x-small">{errors.launchUrl}</span>
</Form.Control.Feedback>
)}
</Form.Group>
{showLTIConfig && (
<>
<Form.Group controlId="consumerKey" isInvalid={isInvalidConsumerKey} className="mb-4" data-testid="ltiConfigFields">
<Form.Control
floatingLabel={intl.formatMessage(messages.consumerKey)}
onChange={handleChange}
onBlur={handleBlur}
value={values.consumerKey}
/>
{isInvalidConsumerKey && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<span className="x-small">{errors.consumerKey}</span>
</Form.Control.Feedback>
)}
</Form.Group>
<Form.Group controlId="consumerSecret" isInvalid={isInvalidConsumerSecret} className="mb-4">
<Form.Control
floatingLabel={intl.formatMessage(messages.consumerSecret)}
onChange={handleChange}
onBlur={handleBlur}
value={values.consumerSecret}
/>
{isInvalidConsumerSecret && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<span className="x-small">{errors.consumerSecret}</span>
</Form.Control.Feedback>
)}
</Form.Group>
<Form.Group controlId="launchUrl" isInvalid={isInvalidLaunchUrl}>
<Form.Control
floatingLabel={intl.formatMessage(messages.launchUrl)}
onChange={handleChange}
onBlur={handleBlur}
value={values.launchUrl}
/>
{isInvalidLaunchUrl && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<span className="x-small">{errors.launchUrl}</span>
</Form.Control.Feedback>
)}
</Form.Group>
</>
)}
{appConfig.piiSharing && (
<>
<Form.Text className="my-2">
Expand All @@ -129,14 +151,15 @@ function LtiConfigForm({
</>
)}
</Form>
<AppExternalLinks externalLinks={externalLinks} title={title} />
<AppExternalLinks externalLinks={externalLinks} providerName={providerName} />
</Card>
);
}

LtiConfigForm.propTypes = {
app: PropTypes.shape({
id: PropTypes.string.isRequired,
adminOnlyConfig: PropTypes.bool,
externalLinks: PropTypes.shape({
learnMore: PropTypes.string,
configuration: PropTypes.string,
Expand All @@ -158,7 +181,7 @@ LtiConfigForm.propTypes = {
onSubmit: PropTypes.func.isRequired,
// eslint-disable-next-line react/forbid-prop-types
formRef: PropTypes.object.isRequired,
title: PropTypes.string.isRequired,
providerName: PropTypes.string.isRequired,
};

LtiConfigForm.defaultProps = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ const messages = defineMessages({
defaultMessage: 'Launch URL is a required field',
description: 'Tells the user that the Launch URL field is required and must have a value.',
},
adminOnlyConfig: {
id: 'authoring.discussions.adminOnlyConfig',
defaultMessage: '{providerName} can only be configured by {platformName} administrators. Please contact {supportEmail} to enable this feature.',
},
piiSharing: {
id: 'authoring.discussions.piiSharing',
defaultMessage: 'Optionally share a user\'s username and/or email with the LTI provider:',
Expand Down Expand Up @@ -75,7 +79,7 @@ const messages = defineMessages({
},
learnMore: {
id: 'authoring.discussions.appDocInstructions.learnMoreLink',
defaultMessage: 'Learn more about {title}',
defaultMessage: 'Learn more about {providerName}',
description: 'Application Document Instructions message for learn more links',
},
linkTextHeading: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import messages from '../lti/messages';
function AppExternalLinks({
externalLinks,
intl,
title,
providerName,
}) {
const { contactEmail, ...links } = externalLinks;
const linkTypes = Object.keys(links).filter(key => links[key]);
Expand All @@ -31,7 +31,7 @@ function AppExternalLinks({
rel="noopener noreferrer"
showLaunchIcon={false}
>
{ intl.formatMessage(messages[type], { title }) }
{ intl.formatMessage(messages[type], { providerName }) }
</Hyperlink>
</div>
))}
Expand Down Expand Up @@ -67,7 +67,7 @@ AppExternalLinks.propTypes = {
accessibility: PropTypes.string,
contactEmail: PropTypes.string,
}).isRequired,
title: PropTypes.string.isRequired,
providerName: PropTypes.string.isRequired,
intl: intlShape.isRequired,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ describe('TopicItem', () => {
expect(topicCard.querySelector('input')).toBeInTheDocument();
});

test('renders delete topic popup with title, label, helping text, a delete and a cancel button', async () => {
test('renders delete topic popup with providerName, label, helping text, a delete and a cancel button', async () => {
await mockStore(legacyApiResponse);
createComponent(additionalTopic);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('AppCard', () => {
test.each([
[true],
[false],
])('title and text from the app are displayed with full support %s', (hasFullSupport) => {
])('providerName and text from the app are displayed with full support %s', (hasFullSupport) => {
const appWithCustomSupport = { ...app, hasFullSupport };
const title = messages[`appName-${appWithCustomSupport.id}`].defaultMessage;
const text = messages[`appDescription-${appWithCustomSupport.id}`].defaultMessage;
Expand Down
Loading

0 comments on commit b13bd69

Please sign in to comment.