Skip to content

fix: require verified email to invite #66948

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

fix: require verified email to invite #66948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
833d8ac
rebase master
mdtro Mar 14, 2024
91e0426
fix: add emails to stub
mdtro Mar 14, 2024
f8d0dba
fix tests
mdtro Apr 29, 2024
f6dd77f
return superuser once
mdtro Apr 29, 2024
bf4005d
:hammer_and_wrench: apply eslint style fixes
getsantry[bot] Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion fixtures/js-stubs/user.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,13 @@ export function UserFixture(params: Partial<User> = {}): User {
authenticators: [],
canReset2fa: false,
dateJoined: '2020-01-01T00:00:00.000Z',
emails: [],
emails: [
{
id: '1',
email: 'foo@example.com',
is_verified: true,
},
],
experiments: [],
has2fa: false,
identities: [],
Expand Down
41 changes: 34 additions & 7 deletions static/app/components/modals/inviteMembersModal/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,46 @@ import InviteMembersModal from 'sentry/components/modals/inviteMembersModal';
import {ORG_ROLES} from 'sentry/constants';
import TeamStore from 'sentry/stores/teamStore';
import type {DetailedTeam, Scope} from 'sentry/types';
import {isActiveSuperuser} from 'sentry/utils/isActiveSuperuser';
import useOrganization from 'sentry/utils/useOrganization';

jest.mock('sentry/utils/useOrganization');
jest.mock('sentry/utils/isActiveSuperuser', () => ({
isActiveSuperuser: jest.fn(),
}));

describe('InviteMembersModal', function () {
const styledWrapper = styled(c => c.children);

type MockApiResponseFn = (
client: typeof MockApiClient,
orgSlug: string,
is_superuser: boolean,
verified_email: boolean,
roles?: object[]
) => jest.Mock;
const defaultMockOrganizationRoles: MockApiResponseFn = (client, orgSlug, roles) => {
const defaultMockOrganizationRoles: MockApiResponseFn = (
client,
orgSlug,
is_superuser,
verified_email,
roles
) => {
return client.addMockResponse({
url: `/organizations/${orgSlug}/members/me/`,
method: 'GET',
body: {orgRoleList: roles},
body: {
user: {
isSuperuser: is_superuser,
emails: [
{
email: 'test@dev.getsentry.net',
is_verified: verified_email,
},
],
},
orgRoleList: roles,
},
});
};

Expand All @@ -50,6 +73,8 @@ describe('InviteMembersModal', function () {
const setupView = ({
orgTeams = [TeamFixture()],
orgAccess = ['member:write'],
is_superuser = false,
verified_email = false,
roles = [
{
id: 'admin',
Expand All @@ -69,11 +94,13 @@ describe('InviteMembersModal', function () {
modalProps = defaultMockModalProps,
mockApiResponses = [defaultMockOrganizationRoles],
}: {
is_superuser?: boolean;
mockApiResponses?: MockApiResponseFn[];
modalProps?: ComponentProps<typeof InviteMembersModal>;
orgAccess?: Scope[];
orgTeams?: DetailedTeam[];
roles?: object[];
verified_email?: boolean;
} = {}) => {
const org = OrganizationFixture({access: orgAccess, teams: orgTeams});
TeamStore.reset();
Expand All @@ -82,7 +109,9 @@ describe('InviteMembersModal', function () {
MockApiClient.clearMockResponses();
const mocks: jest.Mock[] = [];
mockApiResponses.forEach(mockApiResponse => {
mocks.push(mockApiResponse(MockApiClient, org.slug, roles));
mocks.push(
mockApiResponse(MockApiClient, org.slug, is_superuser, verified_email, roles)
);
});
jest.mocked(useOrganization).mockReturnValue(org);

Expand All @@ -106,7 +135,7 @@ describe('InviteMembersModal', function () {
};

it('renders', async function () {
setupView();
setupView({verified_email: true});
await waitFor(() => {
// Starts with one invite row
expect(screen.getByRole('listitem')).toBeInTheDocument();
Expand All @@ -120,9 +149,7 @@ describe('InviteMembersModal', function () {
});

it('renders for superuser', async function () {
jest.mock('sentry/utils/isActiveSuperuser', () => ({
isActiveSuperuser: jest.fn(),
}));
jest.mocked(isActiveSuperuser).mockReturnValueOnce(true);

const errorResponse: MockApiResponseFn = (client, orgSlug, _) => {
return client.addMockResponse({
Expand Down
20 changes: 15 additions & 5 deletions static/app/components/modals/inviteMembersModal/inviteMembersModalview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {IconAdd} from 'sentry/icons';
import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import type {Member} from 'sentry/types/organization';
import {isActiveSuperuser} from 'sentry/utils/isActiveSuperuser';

interface Props {
Footer: ModalRenderProps['Footer'];
Expand Down Expand Up @@ -64,8 +65,6 @@ export default function InviteMembersModalView({
willInvite,
error,
}: Props) {
const disableInputs = sendingInvites || complete;

const inviteEmails = invites.map(inv => inv.email);
const hasDuplicateEmails = inviteEmails.length !== new Set(inviteEmails).size;
const isValidInvites = invites.length > 0 && !hasDuplicateEmails;
Expand All @@ -76,18 +75,29 @@ export default function InviteMembersModalView({
</Alert>
) : null;

const userEmails = member?.user?.emails;
const isVerified = userEmails ? userEmails.some(element => element.is_verified) : false;

const isSuperUser = isActiveSuperuser();

const disableInputs = sendingInvites || complete || (!isVerified && !isSuperUser);

return (
<Fragment>
{errorAlert}
<Heading>{t('Invite New Members')}</Heading>
{willInvite ? (
<Subtext>{t('Invite new members by email to join your organization.')}</Subtext>
) : (
{!isVerified && !isSuperUser ? (
<Alert type="warning" showIcon>
{t('Please verify your email before inviting other users.')}
Copy link
Member

Choose a reason for hiding this comment

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

we have some very old users with unverified email so they won't have verify email in their inbox. We should add a button that would trigger that email again here in the flow.

Should we make this required? It seems to me that we should have emails verified and this would be a good way to enforce it.

</Alert>
) : !willInvite && isVerified && !isSuperUser ? (
<Alert type="warning" showIcon>
{t(
'You can’t invite users directly, but we’ll forward your request to an org owner or manager for approval.'
)}
</Alert>
) : (
<Subtext>{t('Invite new members by email to join your organization.')}</Subtext>
)}

{headerInfo}
Expand Down
Loading