Stop Automatically Enabling Columns Added to a Table in its Views

[Ghrehh]

Noticed an issue/oversight where adding a column to a table with views would also automatically enable that column on the view. This not only seems a little insecure, but also wasn't reflected in the UI, which recognised the newly added column as disabled.

Though the field is marked disabled in the UI, a consumer of the view in the client has access to the new field, which can be seen by looking at the API response or doing some binding trickery.

This seems to be due to how columns being enabled or disabled in views is handled. For the backend, the presence of the column in the view schema denotes that it's enabled, and visible: true/false isn't respected. The code that was removed to fix the issue below actually enabled the field, which doesn't seem to be the intention.

I don't think this fix breaks anything else, but it would be good for someone on the backend team to look over this for me please 🙏 The code for views was very complex 🙃

[deanhannigan]

You could also leave in the snippet you removed and just filter out the fields in the search request itself. Columns with config set to { visible : false } could be filtered out here:
https://github.com/Budibase/budibase/blob/master/packages/server/src/api/controllers/row/views.ts#L27

This wouldn't require any update to the view and should immediately hide the value in subsequent view requests.

[samwho]

This appears to have broken some tests, and I'm not sure myself what the correct behaviour would be. @mike12345567?

[mike12345567]

This appears to have broken some tests, and I'm not sure myself what the correct behaviour would be. @mike12345567?

This is due to the way the tests are written - they depended on the syncSchema function which has been adjusted in this PR adding the columns to the view.

This functionality changes that logic so that columns are not added to views without a specific call to do so - therefore the tests will need to be updated to mimic this logic.

Everywhere in the views.spec.ts test that the syncSchema has been called, this will need to be adjusted to instead explicitly sync the schema and then update the schema with the columns. This is a more unit-test-y example of a test as it doesn't go through the API at all, which makes this a little more difficult.

This is a tough one because the test does appear to dependent on the functionality that this is changing, so the test does need a bit of reconfiguration to work in the flow that is now expected.

[aptkingston]

Just to check - was the issue that you can see the field in the view schema, or that you actually get access to data in that field if you read a row through the view?

If it's just the former then I'm pretty sure that's by design - the schema isn't intended to be secret, and we want to be able to tell what fields exist but are invisible so we can do things like show them for the builder to toggle visible again, or warn users that there are hidden required fields (which is something the grid currently does).

If it's the latter then yea this definitely needs addressed.

[Ghrehh]

@aptkingston It was the latter, in the scenario I outlined in the main body of the PR a view could expose a value present in the base table that wasn't specified as a column in the view.

[adrinr]

@Ghrehh I could not replicate the issue in master. I created a view, added a new column, but it's hidden in the views for both client and builder. Could you send a video or a step by step replication?