(core) Don't throw error in `onRecord(s)` for insufficient access for `includeColumns`

Summary:
This removes checking for full access in `onRecord/onRecords` when `includeColumns` is a non-default value. The check had two problems:

1. It relied on the access level being present in the URL query parameters, which doesn't work if the page has redirected. See the discussion in https://grist.slack.com/archives/C0234CPPXPA/p1702576602615509. There seems to be no way to reliably and synchronously check the access level.
2. Calling `onRecords` before `ready` and forgetting to handle an error from the access check meant that `ready` wouldn't be called, so Grist couldn't request the correct access level from the user. I made this mistake and it seems like a nasty footgun.

Ultimately this has no effect on security, as an error will still be raised, but in a place where the widget developer can't catch it. They'll still see an error message in the console, and they can still check the access level reliably using `onOptions`, so I think this is OK.

Test Plan: Updated nbrowser test

Reviewers: georgegevoian, paulfitz

Reviewed By: georgegevoian, paulfitz

Differential Revision: https://phab.getgrist.com/D4145
pull/811/head
Alex Hall 6 months ago
parent a2bd753649
commit 225a76c9cb

@ -489,7 +489,9 @@ export class GristViewImpl implements GristView {
// These options are newer and expose more data than the user may have intended, // These options are newer and expose more data than the user may have intended,
// so they require full access. // so they require full access.
if (this._access !== AccessLevel.full) { if (this._access !== AccessLevel.full) {
throwError(this._access); throw new Error(
`Setting includeColumns to ${options.includeColumns} requires full access.` +
` Current access level is ${this._access}`);
} }
if (options.includeColumns === 'normal') { if (options.includeColumns === 'normal') {
// Return all 'normal' columns of the table, regardless of whether the user has shown them. // Return all 'normal' columns of the table, regardless of whether the user has shown them.

@ -365,17 +365,6 @@ export function mapColumnNamesBack(data: any, options?: {
return mapColumnNames(data, {...options, reverse: true}); return mapColumnNames(data, {...options, reverse: true});
} }
/**
* While `fetchSelected(Record|Table)` check the access level on 'the Grist side',
* `onRecord(s)` needs to check this in advance for the caller to be able to handle the error.
*/
function checkAccessLevelForColumns(options: FetchSelectedOptions) {
const accessLevel = new URL(window.location.href).searchParams.get("access");
if (accessLevel !== "full" && options.includeColumns && options.includeColumns !== "shown") {
throw new Error("Access not granted. Current access level " + accessLevel);
}
}
/** /**
* For custom widgets, add a handler that will be called whenever the * For custom widgets, add a handler that will be called whenever the
* row with the cursor changes - either by switching to a different row, or * row with the cursor changes - either by switching to a different row, or
@ -388,7 +377,6 @@ export function onRecord(
callback: (data: RowRecord | null, mappings: WidgetColumnMap | null) => unknown, callback: (data: RowRecord | null, mappings: WidgetColumnMap | null) => unknown,
options: FetchSelectedOptions = {}, options: FetchSelectedOptions = {},
) { ) {
checkAccessLevelForColumns(options);
// TODO: currently this will be called even if the content of a different row changes. // TODO: currently this will be called even if the content of a different row changes.
on('message', async function(msg) { on('message', async function(msg) {
if (!msg.tableId || !msg.rowId || msg.rowId === 'new') { return; } if (!msg.tableId || !msg.rowId || msg.rowId === 'new') { return; }
@ -418,7 +406,6 @@ export function onRecords(
callback: (data: RowRecord[], mappings: WidgetColumnMap | null) => unknown, callback: (data: RowRecord[], mappings: WidgetColumnMap | null) => unknown,
options: FetchSelectedOptions = {}, options: FetchSelectedOptions = {},
) { ) {
checkAccessLevelForColumns(options);
options = {...options, format: options.format || 'rows'}; options = {...options, format: options.format || 'rows'};
on('message', async function(msg) { on('message', async function(msg) {
if (!msg.tableId || !msg.dataChange) { return; } if (!msg.tableId || !msg.dataChange) { return; }

@ -2,16 +2,13 @@
function setup() { function setup() {
const data = { const data = {
shown: 0,
default: {}, default: {},
options: {}, options: {},
}; };
let showCount = 0;
function showData() { function showData() {
showCount += 1; data.shown += 1;
if (showCount < 12) {
return;
}
document.getElementById('data').innerHTML = JSON.stringify(data, null, 2); document.getElementById('data').innerHTML = JSON.stringify(data, null, 2);
} }
@ -40,24 +37,17 @@ function setup() {
showData(); showData();
}); });
try { // NOTE: These cases will hit an access error when trying to trigger the callback
// when access level isn't full, and we can't catch that error.
grist.onRecord(function (rec) { grist.onRecord(function (rec) {
data.options.onRecord = rec; data.options.onRecord = rec;
showData(); showData();
}, {keepEncoded: true, includeColumns: 'normal', format: 'columns'}); }, {keepEncoded: true, includeColumns: 'normal', format: 'columns'});
} catch (e) {
data.options.onRecord = String(e);
showData();
}
try {
grist.onRecords(function (recs) { grist.onRecords(function (recs) {
data.options.onRecords = recs; data.options.onRecords = recs;
showData(); showData();
}, {keepEncoded: true, includeColumns: 'all', format: 'columns'}); }, {keepEncoded: true, includeColumns: 'all', format: 'columns'});
} catch (e) {
data.options.onRecords = String(e);
showData();
}
grist.fetchSelectedTable( grist.fetchSelectedTable(
{keepEncoded: true, includeColumns: 'all', format: 'rows'} {keepEncoded: true, includeColumns: 'all', format: 'rows'}
).then(function (table) { ).then(function (table) {

@ -609,14 +609,17 @@ describe('CustomView', function() {
} }
}; };
async function getData() { async function getData(shown: number) {
await driver.findContentWait('#data', /\{/, 1000); await driver.findContentWait('#data', `"shown": ${shown}`, 1000);
const data = await driver.find('#data').getText(); const data = await driver.find('#data').getText();
return JSON.parse(data); const result = JSON.parse(data);
assert.equal(result.shown, shown);
delete result.shown;
return result;
} }
await inFrame(async () => { await inFrame(async () => {
const parsed = await getData(); const parsed = await getData(12);
assert.deepEqual(parsed, expected); assert.deepEqual(parsed, expected);
}); });
@ -625,25 +628,25 @@ describe('CustomView', function() {
await gu.waitForServer(); await gu.waitForServer();
await inFrame(async () => { await inFrame(async () => {
const parsed = await getData(); // onRecord(s) with custom includeColumns without full access will fail
// with an error that we can't catch and display,
// so only wait for 10 results instead of 12.
const parsed = await getData(10);
// The default options don't require full access, so the result is the same. // The default options don't require full access, so the result is the same.
assert.deepEqual(parsed.default, expected.default); assert.deepEqual(parsed.default, expected.default);
// The alternative options all set includeColumns to 'normal' or 'all', // The alternative options all set includeColumns to 'normal' or 'all',
// which requires full access. // which requires full access.
assert.deepEqual(parsed.options, { assert.deepEqual(parsed.options, {
"onRecord":
"Error: Access not granted. Current access level read table",
"onRecords":
"Error: Access not granted. Current access level read table",
"fetchSelectedTable": "fetchSelectedTable":
"Error: Access not granted. Current access level read table", "Error: Setting includeColumns to all requires full access. Current access level is read table",
"fetchSelectedRecord": "fetchSelectedRecord":
"Error: Access not granted. Current access level read table", "Error: Setting includeColumns to normal requires full access. Current access level is read table",
"viewApiFetchSelectedTable": "viewApiFetchSelectedTable":
"Error: Access not granted. Current access level read table", "Error: Setting includeColumns to all requires full access. Current access level is read table",
"viewApiFetchSelectedRecord": "viewApiFetchSelectedRecord":
"Error: Access not granted. Current access level read table" "Error: Setting includeColumns to normal requires full access. Current access level is read table"
}); });
}); });
}); });

Loading…
Cancel
Save