Fix readonly mode triggering permission denied error in FormMeta

[sharadsw]

Fixes #4997

Checklist

• Self-review the PR after opening it to make sure the changes look good
  and self-explanatory (or properly documented)
• Add automated tests
• Add relevant issue to release milestone

Testing instructions

• Go to any pre-existing form (e.g. a saved Collection Object resource)
• Click on the form meta button in the top right
• Click on 'Enable Read-Only Mode'
• Click on form meta again
• Verify there is no permission denied error and read only mode can be disabled again
• Re-run the same steps for a form with merge records enabled (Agent, Locality, PaleoContext, CollectingEvent)

Additional test:

• In Readonly mode, make a query on LoanPreparation
• Verify the page loads correctly and there is no permission denied error dialog

[maxpatiiuk]

hmm, could you please explain what was causing the bug and why this fixes it?
it's hard without any context to know if this is a good fix or not.
i.e is there an issue with this particular check in this one place, or is it a more systematic failure that could affect all such checks anywhere in the codebase?
and what change lead to this error to start appearing where as it did not before?

[sharadsw]

@maxpatiiuk My bad, forgot to add my notes here.

NOTES:
This was caused in #4606 when adding a merge record button to FormMeta.
MergeRecord was wrapped in ProtectedTable which checked if user had update permissions on the form. If not, it would render the permission denied dialog box seen in the issue.

specify7/specifyweb/frontend/js_src/lib/components/FormMeta/index.tsx

Lines 221 to 228 in b62a79b

	<ProtectedTable
	action="update"
	tableName={resource.specifyTable.name}
	>
	{recordMergingTables.has(resource.specifyTable.name) && (
	<MergeRecord resource={resource} />
	)}
	</ProtectedTable>

specify7/specifyweb/frontend/js_src/lib/components/Permissions/PermissionDenied.tsx

Lines 159 to 173 in bb328a3

	export function ProtectedTable({
	tableName,
	action,
	children,
	}: {
	readonly tableName: keyof Tables;
	readonly action: typeof tableActions[number];
	readonly children: React.ReactNode;
	}): JSX.Element {
	return hasTablePermission(tableName, action) ? (
	<>{children}</>
	) : (
	<TablePermissionDenied action={action} tableName={tableName} />
	);
	}

When readonly mode is enabled, the user no longer has update permissions on the table and so we see the permission denied error. With my change, we still check for table permission but simply don't render the merge record button when the form is readonly (or if the user has no permission)

[melton-jason]

While this does fix the issue in this case, the ProtectedTable would still cause the error dialog in another case: when viewing any LoanPreparation Query.

Is this intended? I would instead think disabling or hiding the button in this case would be more user-friendly (rather than make the query completely non-functional in ReadOnly Mode)

Screen.Recording.2024-06-13.at.11.00.50.AM.mov

specify7/specifyweb/frontend/js_src/lib/components/QueryBuilder/Header.tsx

Lines 112 to 128 in bb328a3

	{state.baseTableName === 'LoanPreparation' && (
	<ProtectedAction action="execute" resource="/querybuilder/query">
	<ProtectedTable action="update" tableName="Loan">
	<ProtectedTable action="create" tableName="LoanReturnPreparation">
	<ProtectedTable action="read" tableName="LoanPreparation">
	<ErrorBoundary dismissible>
	<QueryLoanReturn
	fields={state.fields}
	getQueryFieldRecords={getQueryFieldRecords}
	queryResource={queryResource}
	/>
	</ErrorBoundary>
	</ProtectedTable>
	</ProtectedTable>
	</ProtectedTable>
	</ProtectedAction>
	)}

This raises questions about the general behavior of permissions in Specify, especially relating to tools such as these. I don't think that raising an error in these cases is ever helpful (at least in the current state. It would be better if the Error Dialog was dismissible).

In these cases, I would imagine the intended behavior is to either disable the tool/button, or not render it in the first place.

In my opinion, raising the error dialog should only occur when the user explicitly tries to do something against their permissions, such as navigating to a form they don't have read access to. Small tools/buttons such as the Enable Read Only Mode or Return Loan Records buttons are already in the Interface while the user is trying to accomplish another task entirely.

In the case of Return Loan Records in the QueryBuilder, the user is likely there to Query on records rather than Return the Loans Preparations, so violating the table create permission should be approached with less severity/notice to the user.
We already hide the button without raising an error in the InteractionsDialog.

specify7/specifyweb/frontend/js_src/lib/components/Interactions/InteractionsDialog.tsx

Lines 66 to 88 in bb328a3

	{entries
	.filter((table) => hasTablePermission(table.name, 'create'))
	.map((table, index) => (
	<li key={index}>
	<Link.Default
	href={
	table.name === 'LoanReturnPreparation'
	? `/specify/overlay/interactions/return-loan/`
	: interactionsWithPrepTables.includes(
	(table as SpecifyTable<InteractionWithPreps>).name
	)
	? `/specify/overlay/interactions/create/${table.name}/`
	: getResourceViewUrl(table.name)
	}
	>
	<TableIcon label={false} name={table.name} />
	{table.name === 'LoanReturnPreparation'
	? interactionsText.returnLoan({
	tableLoan: tables.Loan.label,
	})
	: table.label}
	</Link.Default>
	</li>

Also, I would think the Dismiss button on the Permission Denied Dialog redirecting the user back to the home page is unintentional.

[sharadsw]

@melton-jason I agree that permission denied should only be shown when the user actively attempts to do something against their permissions. I think the ProtectedX components should only be used in such cases and for less severe cases using hasXPermission() should be good enough.
We could modify ProtectedX to render undefined based on a prop but that would be the same as using hasXPermission() with extra (unnecessary) steps.

[maxpatiiuk]

Thank you for the details

In these cases, I would imagine the intended behavior is to either disable the tool/button, or not render it in the first place.

Yeah, ideally the permission denied dialog would only be shown in two cases:

• The user directly navigated to a URL of the action they don't have a permission for (in which case you need to explicitly tell them that, rather than display an empty page)
• Knowing that the user does not have a permission is for some reason important (rather than simply hiding the feature). One such use case I remember is when we added a new feature (might have been agent merging), but the permission for which wasn't included by default (so no one had is except for the admins), thus people might not even know that this feature was added because they won't see it in the UI. Where as seeing a disabled "Merge records" button in the UI will at least be a cause for some investigation (or a support email)

Given the above, I think we should try to follow this rule:

• When displaying a button/link to an action the user has no permission for, use one of the has permission functions (i.e hasTablePermission) to conditionally render the UI only if permission is given, and else don't display anything
• On the other hand, when the user is at the URL for the page they have no permission to, the content of the page needs to be wrapped in one of the component checks (i.e ProtectedTable), which will render the UI, or an error dialog which permission is missing. Think of it as we are wrapping the area in one of the ProtectedAction/ProtectedTable/... component to designate it as a "protected area".

If we are careful about where we use function check and where we use the component check, most issues with readonly should be resolved. If that is not enough however, we could add a check inside the component check to not display an error dialog if user is in readonly mode

Given the above, it becomes more clear that the use of Protected component checks for a button is incorrect - permission function check should be used instead.

---

As far as the other places were permission checks are used, it would be great if someone could check them (maybe one of the student programmers), though that would have to be compared to the other priorities

[melton-jason]

I agree with #5001 (comment) in that we should also go through the other uses of the Protected components (ProtectedAction, ProtectedTool, ProtectedTable) and the hasPermission functions to ensure no other error like this is present.

It would be a great exercise for another student developer!

---

Also from #5001 (review)

Also, I would think the Dismiss button on the Permission Denied Dialog redirecting the user back to the home page is unintentional.

I would imagine this is a bug.

---

Although neither of these Issues likely have to be resolved in this PR: the PR might become too big, and testing instructions would be a nightmare! (As well as keep potential merge conflicts to a minimum).

This smaller scope works for now. Nice changes!

[emenslin]

Testing instructions

• Go to any pre-existing form (e.g. Collection Object)
• Click on the form meta button in the top right
• Click on 'Enable Read-Only Mode'
• Click on form meta again
• Verify there is no permission denied error and read only mode can be disabled again
• Re-run the same steps for a form with merge records enabled (Agent, Locality, PaleoContext, CollectingEvent)

Additional test:

• In Readonly mode, make a query on LoanPreparation
• Verify the page loads correctly and there is no permission denied error dialog

Looks good, I did not run into any other errors with read only

[Areyes42]

Testing instructions

• Go to any pre-existing form (e.g. Collection Object)
• Click on the form meta button in the top right
• Click on 'Enable Read-Only Mode'
• Click on form meta again
• Verify there is no permission denied error and read only mode can be disabled again
• Re-run the same steps for a form with merge records enabled (Agent, Locality, PaleoContext, CollectingEvent)

Additional test:

• In Readonly mode, make a query on LoanPreparation
• Verify the page loads correctly and there is no permission denied error dialog

---

Looks good now, no longer throwing error dialogs

[lexiclevenger]

Testing instructions

• Go to any pre-existing form (e.g. Collection Object)
• Click on the form meta button in the top right
• Click on 'Enable Read-Only Mode'
• Click on form meta again
• Verify there is no permission denied error and read only mode can be disabled again
• Re-run the same steps for a form with merge records enabled (Agent, Locality, PaleoContext, CollectingEvent)

Additional test:

• In Readonly mode, make a query on LoanPreparation
• Verify the page loads correctly and there is no permission denied error dialog

---

Looks good!

Screen.Recording.2024-06-14.at.2.07.43.PM.mov

I tested querying for Loan Preparations and some other tables with no issue.