diff --git a/.changeset/witty-boats-joke.md b/.changeset/witty-boats-joke.md new file mode 100644 index 00000000000..90fe8643cbd --- /dev/null +++ b/.changeset/witty-boats-joke.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-js': patch +--- + +Replace role based check with permission based checks inside the OrganizationMembers component. diff --git a/packages/clerk-js/src/ui/common/Gate.tsx b/packages/clerk-js/src/ui/common/Gate.tsx index 7da69f6b22f..ca4af25c1bb 100644 --- a/packages/clerk-js/src/ui/common/Gate.tsx +++ b/packages/clerk-js/src/ui/common/Gate.tsx @@ -1,4 +1,4 @@ -import type { IsAuthorized } from '@clerk/types'; +import type { IsAuthorized, OrganizationPermission } from '@clerk/types'; import type { ComponentType, PropsWithChildren, ReactNode } from 'react'; import React, { useEffect } from 'react'; @@ -6,7 +6,7 @@ import { useCoreSession } from '../contexts'; import { useFetch } from '../hooks'; import { useRouter } from '../router'; -type GateParams = Parameters[0]; +type GateParams = Omit[0], 'permission'> & { permission: OrganizationPermission }; type GateProps = PropsWithChildren< GateParams & { fallback?: ReactNode; diff --git a/packages/clerk-js/src/ui/components/OrganizationProfile/ActiveMembersList.tsx b/packages/clerk-js/src/ui/components/OrganizationProfile/ActiveMembersList.tsx index 524b447cc20..66f12915354 100644 --- a/packages/clerk-js/src/ui/components/OrganizationProfile/ActiveMembersList.tsx +++ b/packages/clerk-js/src/ui/components/OrganizationProfile/ActiveMembersList.tsx @@ -1,5 +1,6 @@ import type { MembershipRole, OrganizationMembershipResource } from '@clerk/types'; +import { Gate } from '../../common/Gate'; import { useCoreOrganization, useCoreUser } from '../../contexts'; import { Badge, localizationKeys, Td, Text } from '../../customizables'; import { ThreeDotsMenu, useCardState, UserPreview } from '../../elements'; @@ -8,23 +9,10 @@ import { DataTable, RoleSelect, RowContainer } from './MemberListTable'; export const ActiveMembersList = () => { const card = useCardState(); - const { - organization, - membership: currentUserMembership, - memberships, - ...rest - } = useCoreOrganization({ + const { organization, memberships, ...rest } = useCoreOrganization({ memberships: true, }); - const { memberships: adminMembers } = useCoreOrganization({ - memberships: { - role: ['admin'], - }, - }); - - const isAdmin = currentUserMembership?.role === 'admin'; - const mutateSwrState = () => { const unstable__mutate = (rest as any).unstable__mutate; if (unstable__mutate && typeof unstable__mutate === 'function') { @@ -37,25 +25,17 @@ export const ActiveMembersList = () => { } const handleRoleChange = (membership: OrganizationMembershipResource) => (newRole: MembershipRole) => { - if (!isAdmin) { - return; - } return card .runAsync(async () => { await membership.update({ role: newRole }); - await (adminMembers as any).unstable__mutate?.(); }) .catch(err => handleError(err, [], card.setError)); }; const handleRemove = (membership: OrganizationMembershipResource) => () => { - if (!isAdmin) { - return; - } return card .runAsync(async () => { - const destroyedMembership = membership.destroy(); - await (adminMembers as any).unstable__mutate?.(); + const destroyedMembership = await membership.destroy(); return destroyedMembership; }) .then(mutateSwrState) @@ -82,27 +62,23 @@ export const ActiveMembersList = () => { membership={m} onRoleChange={handleRoleChange(m)} onRemove={handleRemove(m)} - adminCount={adminMembers?.count || 0} /> ))} /> ); }; +// TODO: Find a way to disable Role select based on last admin by using permissions const MemberRow = (props: { membership: OrganizationMembershipResource; onRemove: () => unknown; - adminCount: number; onRoleChange?: (role: MembershipRole) => unknown; }) => { - const { membership, onRemove, onRoleChange, adminCount } = props; + const { membership, onRemove, onRoleChange } = props; const card = useCardState(); - const { membership: currentUserMembership } = useCoreOrganization(); const user = useCoreUser(); - const isAdmin = currentUserMembership?.role === 'admin'; const isCurrentUser = user.id === membership.publicUserData.userId; - const isLastAdmin = adminCount <= 1 && membership.role === 'admin'; return ( @@ -123,21 +99,24 @@ const MemberRow = (props: { {membership.createdAt.toLocaleDateString()} - {isAdmin ? ( + ({ opacity: t.opacity.$inactive })} + localizationKey={roleLocalizationKey(membership.role)} + /> + } + > - ) : ( - ({ opacity: t.opacity.$inactive })} - localizationKey={roleLocalizationKey(membership.role)} - /> - )} + - {isAdmin && ( + - )} + ); diff --git a/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembers.tsx b/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembers.tsx index 47a1315f540..c25477d9db8 100644 --- a/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembers.tsx +++ b/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembers.tsx @@ -1,4 +1,4 @@ -import { NotificationCountBadge } from '../../common'; +import { NotificationCountBadge, useGate } from '../../common'; import { useCoreOrganization, useEnvironment, useOrganizationProfileContext } from '../../contexts'; import { Col, descriptors, Flex, localizationKeys } from '../../customizables'; import { @@ -21,15 +21,19 @@ import { OrganizationMembersTabRequests } from './OrganizationMembersTabRequests export const OrganizationMembers = withCardStateProvider(() => { const { organizationSettings } = useEnvironment(); const card = useCardState(); - const { membership } = useCoreOrganization(); - const isAdmin = membership?.role === 'admin'; - const allowRequests = organizationSettings?.domains?.enabled && isAdmin; + const { isAuthorizedUser: canManageMemberships } = useGate({ permission: 'org:sys_memberships:manage' }); + const isDomainsEnabled = organizationSettings?.domains?.enabled; const { membershipRequests } = useCoreOrganization({ - membershipRequests: allowRequests || undefined, + membershipRequests: isDomainsEnabled || undefined, }); - //@ts-expect-error + + // @ts-expect-error const { __unstable_manageBillingUrl } = useOrganizationProfileContext(); + if (canManageMemberships === null) { + return null; + } + return ( { - {isAdmin && ( + {canManageMemberships && ( )} - {allowRequests && ( + {canManageMemberships && isDomainsEnabled && ( @@ -72,16 +76,16 @@ export const OrganizationMembers = withCardStateProvider(() => { width: '100%', }} > - {isAdmin && __unstable_manageBillingUrl && } + {canManageMemberships && __unstable_manageBillingUrl && } - {isAdmin && ( + {canManageMemberships && ( )} - {allowRequests && ( + {canManageMemberships && isDomainsEnabled && ( diff --git a/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembersTabInvitations.tsx b/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembersTabInvitations.tsx index 335848fbadd..7a57c04c7c3 100644 --- a/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembersTabInvitations.tsx +++ b/packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationMembersTabInvitations.tsx @@ -1,5 +1,5 @@ -import { BlockButton } from '../../common'; -import { useCoreOrganization, useEnvironment, useOrganizationProfileContext } from '../../contexts'; +import { BlockButton, Gate } from '../../common'; +import { useEnvironment, useOrganizationProfileContext } from '../../contexts'; import { Col, descriptors, Flex, Icon, localizationKeys } from '../../customizables'; import { Header, IconButton } from '../../elements'; import { UserAdd } from '../../icons'; @@ -11,17 +11,10 @@ import { MembershipWidget } from './MembershipWidget'; export const OrganizationMembersTabInvitations = () => { const { organizationSettings } = useEnvironment(); const { navigate } = useRouter(); - const { membership } = useCoreOrganization(); //@ts-expect-error const { __unstable_manageBillingUrl } = useOrganizationProfileContext(); - const isAdmin = membership?.role === 'admin'; - - const allowDomains = organizationSettings?.domains?.enabled; - - if (!isAdmin) { - return null; - } + const isDomainsEnabled = organizationSettings?.domains?.enabled; return ( { > {__unstable_manageBillingUrl && } - {allowDomains && ( - - - - - - + + + navigate('organization-settings/domain')} + textVariant='largeMedium' /> - } - redirectSubPath={'organization-settings/domain/'} - verificationStatus={'verified'} - enrollmentMode={'automatic_invitation'} - /> - + + + navigate('organization-settings/domain')} + /> + } + redirectSubPath={'organization-settings/domain/'} + verificationStatus={'verified'} + enrollmentMode={'automatic_invitation'} + /> + + )} { const { navigate } = useRouter(); - const { membership } = useCoreOrganization(); //@ts-expect-error const { __unstable_manageBillingUrl } = useOrganizationProfileContext(); - const isAdmin = membership?.role === 'admin'; - - if (!isAdmin) { - return null; - } return ( { it('renders the Organization Members page', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withOrganizations(); f.withUser({ email_addresses: ['test@clerk.dev'], organization_memberships: ['Org1'] }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + const { getByText, getByRole } = render(, { wrapper }); await waitFor(() => { @@ -32,12 +34,14 @@ describe('OrganizationMembers', () => { }); it('shows requests if domains is turned on', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withOrganizations(); f.withOrganizationDomains(); f.withUser({ email_addresses: ['test@clerk.dev'], organization_memberships: ['Org1'] }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + const { getByRole } = render(, { wrapper }); await waitFor(() => { @@ -46,32 +50,37 @@ describe('OrganizationMembers', () => { }); it('shows an invite button inside invitations tab if the current user is an admin', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withOrganizations(); f.withUser({ email_addresses: ['test@clerk.dev'], organization_memberships: [{ name: 'Org1', role: 'admin' }] }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + const { getByRole, getByText } = render(, { wrapper }); - await userEvent.click(getByRole('tab', { name: 'Invitations' })); - await waitFor(() => { + await waitFor(async () => { + await userEvent.click(getByRole('tab', { name: 'Invitations' })); expect(getByText('Invited')).toBeDefined(); expect(getByRole('button', { name: 'Invite' })).toBeDefined(); }); }); it('does not show invitations and requests if user is not an admin', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withOrganizations(); f.withUser({ email_addresses: ['test@clerk.dev'], - organization_memberships: [{ name: 'Org1', role: 'basic_member' }], + organization_memberships: [{ name: 'Org1' }], }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(false); + const { queryByRole } = render(, { wrapper }); await waitFor(() => { + expect(queryByRole('tab', { name: 'Members' })).toBeInTheDocument(); expect(queryByRole('tab', { name: 'Invitations' })).not.toBeInTheDocument(); expect(queryByRole('tab', { name: 'Requests' })).not.toBeInTheDocument(); }); @@ -83,11 +92,13 @@ describe('OrganizationMembers', () => { f.withUser({ email_addresses: ['test@clerk.dev'], organization_memberships: [{ name: 'Org1', role: 'admin' }] }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + const { getByRole } = render(, { wrapper }); - await userEvent.click(getByRole('tab', { name: 'Invitations' })); - await userEvent.click(getByRole('button', { name: 'Invite' })); - await waitFor(() => { + await waitFor(async () => { + await userEvent.click(getByRole('tab', { name: 'Invitations' })); + await userEvent.click(getByRole('button', { name: 'Invite' })); expect(fixtures.router.navigate).toHaveBeenCalledWith('invite-members'); }); }); @@ -126,7 +137,7 @@ describe('OrganizationMembers', () => { f.withOrganizations(); f.withUser({ email_addresses: ['test@clerk.dev'], - organization_memberships: [{ name: 'Org1', id: '1', role: 'admin' }], + organization_memberships: [{ name: 'Org1', id: '1' }], }); }); @@ -144,6 +155,8 @@ describe('OrganizationMembers', () => { }), ); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + const { queryByText, queryAllByRole } = render(, { wrapper }); await waitFor(() => { @@ -160,7 +173,8 @@ describe('OrganizationMembers', () => { }); }); - it('Last admin cannot change to member', async () => { + // TODO: Bring this test back once we can determine the last admin by permissions. + it.skip('Last admin cannot change to member', async () => { const membersList: OrganizationMembershipResource[] = [ createFakeMember({ id: '1', @@ -185,10 +199,12 @@ describe('OrganizationMembers', () => { f.withOrganizations(); f.withUser({ email_addresses: ['test@clerk.dev'], - organization_memberships: [{ name: 'Org1', id: '1', role: 'admin' }], + organization_memberships: [{ name: 'Org1', id: '1' }], }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + fixtures.clerk.organization?.getMemberships.mockReturnValueOnce( Promise.resolve({ data: membersList, total_count: 0 }), ); @@ -213,7 +229,7 @@ describe('OrganizationMembers', () => { f.withOrganizationDomains(); f.withUser({ email_addresses: ['test@clerk.dev'], - organization_memberships: [{ name: 'Org1', id: '1', role: 'admin' }], + organization_memberships: [{ name: 'Org1', id: '1' }], }); }); @@ -224,6 +240,8 @@ describe('OrganizationMembers', () => { }), ); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + await runFakeTimers(async () => { const { getByText } = render(, { wrapper }); await waitFor(() => { @@ -255,10 +273,12 @@ describe('OrganizationMembers', () => { f.withOrganizations(); f.withUser({ email_addresses: ['test@clerk.dev'], - organization_memberships: [{ name: 'Org1', id: '1', role: 'admin' }], + organization_memberships: [{ name: 'Org1', id: '1' }], }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + fixtures.clerk.organization?.getInvitations.mockReturnValue( Promise.resolve({ data: invitationList, @@ -266,7 +286,9 @@ describe('OrganizationMembers', () => { }), ); const { queryByText, getByRole } = render(, { wrapper }); - await userEvent.click(getByRole('tab', { name: 'Invitations' })); + await waitFor(async () => { + await userEvent.click(getByRole('tab', { name: 'Invitations' })); + }); expect(fixtures.clerk.organization?.getInvitations).toHaveBeenCalled(); expect(queryByText('admin1@clerk.dev')).toBeInTheDocument(); expect(queryByText('Admin')).toBeInTheDocument(); @@ -303,10 +325,12 @@ describe('OrganizationMembers', () => { f.withOrganizationDomains(); f.withUser({ email_addresses: ['test@clerk.dev'], - organization_memberships: [{ name: 'Org1', id: '1', role: 'admin' }], + organization_memberships: [{ name: 'Org1', id: '1' }], }); }); + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); + fixtures.clerk.organization?.getDomains.mockReturnValue( Promise.resolve({ data: [], @@ -315,7 +339,11 @@ describe('OrganizationMembers', () => { ); fixtures.clerk.organization?.getMembershipRequests.mockReturnValue(Promise.resolve(requests)); const { queryByText, getByRole } = render(, { wrapper }); - await userEvent.click(getByRole('tab', { name: 'Requests' })); + + await waitFor(async () => { + await userEvent.click(getByRole('tab', { name: 'Requests' })); + }); + expect(fixtures.clerk.organization?.getMembershipRequests).toHaveBeenCalledWith({ initialPage: 1, pageSize: 10, @@ -336,7 +364,7 @@ describe('OrganizationMembers', () => { f.withUser({ id: '1', email_addresses: ['test@clerk.dev'], - organization_memberships: [{ name: 'Org1', id: '1', role: 'admin' }], + organization_memberships: [{ name: 'Org1', id: '1' }], }); }); @@ -346,6 +374,8 @@ describe('OrganizationMembers', () => { total_count: 2, }), ); + + fixtures.clerk.session?.isAuthorized.mockResolvedValue(true); const { findByText } = render(, { wrapper }); await waitFor(() => expect(fixtures.clerk.organization?.getMemberships).toHaveBeenCalled()); expect(await findByText('You')).toBeInTheDocument();