Summary:
The `_repairWorkspaceGuests` method is slow for workspaces with large numbers of documents. It makes a query that produces a lot of rows. The query itself is tolerable, but TypeORM processing uses enough CPU to be a likely culprit in some production instability. This diff splits the query into two pieces that are logically independent, but which when combined were resulting in the number of rows being the product of the two pieces. Once split, there is also a where clause that can be applied to one of the pieces.
The purpose of the method is to add every user that a document within a workspace is shared with to a "guest" group of the workspace itself. The design of "guest" groups is not ideal, but this diff leaves the design unchanged and is intended only to speed up operation.
Made some small tweaks to the timing of a flakey test, and temporarily recreated the `samples` directory removed in a previous diff (this is currently breaking tests badly on a fresh worker without a `samples` directory lying around)
Test Plan: added test; existing tests pass
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D2844
Summary:
This cleans up a few things about SELF_HYPERLINK urls:
* Use `urlId` rather than `docId`.
* Correctly merge personal org subdomain.
* In dev environment, use clearer port number.
Test Plan: updated test
Reviewers: alexmojaki, dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2841
Summary:
Names of private tables and columns were leaking via Code View.
This plugs that leak.
Test Plan: adds test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2840
Summary:
We used tslint earlier, and on switching to eslint, some rules were not
transfered. This moves more rules over, for consistent conventions or helpful
warnings.
- Name private members with a leading underscore.
- Prefer interface over a type alias.
- Use consistent spacing around ':' in type annotations.
- Use consistent spacing around braces of code blocks.
- Use semicolons consistently at the ends of statements.
- Use braces around even one-liner blocks, like conditionals and loops.
- Warn about shadowed variables.
Test Plan: Fixed all new warnings. Should be no behavior changes in code.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2831
Summary:
Currently, to compute intermediate steps in a bundle, the bundle
is sent to the data engine to process. Then, if the intermediate
steps break a rule, it is reverted. One problem introduced by
checking permissions this late is that the data engine can be
exposed for formulas with python code by users who don't have the
right to change formulas. This diff pre-checks cases that change
formulas.
Test Plan: added a test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2816
Summary: For row creations and deletions, treat `rec` and `newRec` variables as identical. This simplifies writing a single rule that controls multiple permissions.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2812
Summary:
This diff discounts indirect changes for access control purposes. A UserAction that updates a cell A, which in turn causes changes in other dependent cells, will be considered a change to cell A for access control purposes.
The `engine.apply_user_actions` method now returns a `direct` array, with a boolean for each `stored` action, set to `true` if the action is attributed to the user or `false` if it is attributed to the engine. `GranularAccess` ignores actions attributed to the engine when checking for edit rights.
Subtleties:
* Removal of references to a removed row are considered direct changes.
* Doesn't play well with undos as yet. An action that indirectly modifies a cell the user doesn't have rights to may succeed, but it will not be reversible.
Test Plan: added tests, updated tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2806
Summary:
- Adds a new ChoiceList type, and widgets to view and edit it.
- Store in SQLite as a JSON string
- Support conversions between ChoiceList and other types
Test Plan: Added browser tests, and a test for how these values are stored
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2803
Summary:
When a document has an exception to allow copies,
unset that option on any copies of the document.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2794
Summary:
Exceptional document operations (particularly `system` and `nascent`
operations) should never be denied by a granular access rule.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2792
Summary: Access to structural tables currently depends on SchemaEdit permission. We now make an exception for owner access to _grist_ACLResources and _grist_ACLRules, giving them unconditional access. It was too easy for owners to lock themselves out of editing access rules.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2790
Summary:
- Update rules to be more like we've had with tslint
- Switch tsserver plugin to eslint (tsserver makes for a much faster way to lint in editors)
- Apply suggested auto-fixes
- Fix all lint errors and warnings in core/, app/, test/
Test Plan: Some behavior may change subtly (e.g. added missing awaits), relying on existing tests to catch problems.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2785
Summary:
attribute ActiveDoc log messages to users regardless of whether
they were triggered via a client or directly via api
Test Plan: log messages checked manually
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2786
Summary: add user and docId to heartbeat logging
Test Plan: checked manually
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2784
Summary:
This removes some unintentional repetition of work when there are
no row-level rules (there was a missing `return`).
Test Plan: existing tests pass
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2782
Summary: This addresses a weakness in the following case: rules controlling view access for a column, with a dependency on the values of other columns. We had disabled support for such rules, since the existing implementation worked only on table loads and not on broadcast changes. This diff adds in logic to enrich broadcasts as needed, and allows such rules.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2774
Summary:
When adding robustness to schema changes to granular access control,
a calculation of intermediate row states that was previously done
semi-intelligently on need started happening less intelligently.
This diff separates out the row state calculations from metadata
state calculations so that one can happen without the other.
Test Plan: extended a test. Also did some manual checks.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2773
Summary:
For a long array with removals proportional to that length,
lodash/pullAt becomes slow due to doing one splice per removal.
This diff swaps in an alternate implementation that doesn't become
quadratic. On a 250k-row doc with a row-level access rule, this improves
initial page load for a viewer with access to half the rows from minutes
to seconds.
Test Plan: added test; did manual benchmarking
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2777
Summary:
A client hit a situation where a granular access control "bundle"
was not closed, leaving the document locked until reset. I don't
yet have a replication. This diff is a possible mitigation,
trusting various methods less.
Test Plan: existing tests pass
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2775
Summary:
This fixes DocStorage.fetchQuery when the number of parameters
exceeds the maximum that can be passed directly to sqlite.
In this case, parameters are now stored and used from a temporary
table.
Problem first noticed via a use of DocStorage.fetchQuery by
granular access controls. Access control should be optimized
to make fewer such queries, but that is a separate issue.
Test Plan: added tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2772
Summary:
* adds a smoke test to grist-core
* fixes a problem with highlight.js failing to load correctly
* skips survey for default user
* freshens docker build
Utility files in test/nbrowser are moved to core/test/nbrowser, so that gristUtils are available there. This increased the apparent size of the diff as "./" import paths needed replacing with "test/nbrowser/" paths. The utility files are untouched, except for the code to start a server - it now has a small grist-core specific conditional in it.
Test Plan: adds test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2768
Summary:
This allows `*SPECIAL:AccessRules` to give read access to the access rules to more users, and `*SPECIAL:FullCopies` to grant download/copy rights to more users.
This diff also changes forks to be owned by the user who forked them (previously they were an editor), since that feels more natural.
Test Plan: Added and updated tests.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2760
Summary:
- Use special ACLResources of the form "*SPECIAL:<RuleType>" to represent
special document-wide rules.
- Include default rules that give Read access to these resources to Owners only.
- Add UI with a checkbox to give access to everyone instead.
- Allow expanding the UI for advanced configuration.
- These rules don't actually have any behavior yet.
Test Plan: WIP
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2764
Summary:
Previously in {{D1053}} we switched to using BLOB as the "type" for all columns, to prevent SQLite from casting data unexpectedly. This diff now returns to more meaningful types. We apply marshalling to values when being placed in a column where a cast might occur, to inhibit such casting.
The benefit is that Grist documents become easier to interact with via regular database clients/libraries, which often rely on the column type more than a purely SQLite tool would.
On column type conversion, we run all blobs in the column through a decode/encode cycle so if they no longer need to be marshalled they revert to native type. This could be optimized further, it is somewhat brute force.
Test Plan: Updated tests and reference document
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2755
Summary: this adds constants for user access roles, to facilitate autocomplete.
Test Plan: updated tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2761
Summary:
* Adds a `SELF_HYPERLINK()` python function, with optional keyword arguments to set a label, the page, and link parameters.
* Adds a `UUID()` python function, since using python's uuid.uuidv4 hits a problem accessing /dev/urandom in the sandbox. UUID makes no particular quality claims since it doesn't use an audited implementation. A difficult to guess code is convenient for some use cases that `SELF_HYPERLINK()` enables.
The canonical URL for a document is mutable, but older versions generally forward. So for implementation simplicity the document url is passed it on sandbox creation and remains fixed throughout the lifetime of the sandbox. This could and should be improved in future.
The URL is passed into the sandbox as a `DOC_URL` environment variable.
The code for creating the URL is factored out of `Notifier.ts`. Since the url is a function of the organization as well as the document, some rejiggering is needed to make that information available to DocManager.
On document imports, the new document is registered in the database slightly earlier now, in order to keep the procedure for constructing the URL in different starting conditions more homogeneous.
Test Plan: updated test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2759
Summary:
This particular combination of features is not built out - data will be
censored but changes to data will not. So the user will now get an error
if they try to do it. Existing rules of this kind will continue to
operate as before, and can be set via the api.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2751
Summary: This fixes a bug where one client's access control limits could remove data from others via a cache.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2748
Summary:
Now as the user an owner might choose to view their document as
is likely to not have access to rules, it is better to start
viewing on the default document page rather than /p/acl.
The "Access Rules" link is grayed out when in "view as" mode for
now (improvements are planned).
Test Plan: updated test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2743
Summary:
Previously, if some columns are allowed and the rest are denied,
the client could see unnecessary blank columns. This diff cleans
up column metadata censorship. It also adds a small tweak to
retain the `manualSort` column when filtering columns for a mixed
access table.
Test Plan: added tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2742
Summary:
- Support schema changes in the presence of non-trivial ACL rules.
- Fix update of `aclFormulaParsed` when updating formulas automatically after schema change.
- Filter private metadata in broadcasts, not just fetches. Censorship method is unchanged, just refactored.
- Allow only owners to change ACL rules.
- Force reloads if rules are changed.
- Track rule changes within bundle, for clarity during schema changes - tableId and colId changes create a muddle otherwise.
- Show or forbid pages dynamically depending on user's access to its sections. Logic unchanged, just no longer requires reload.
- Fix calculation of pre-existing rows touched by a bundle, in the presence of schema changes.
- Gray out acl page for non-owners.
Test Plan: added tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2734
Summary:
With this change, if a comment is added to an ACL formula, then that comment will be offered to the user if access is denied and that rule could potentially have granted access.
The code is factored so that when access is permitted, or when partially visible tables are being filtered, there is little overhead. Comments are gathered only when an explicit denial of access.
Test Plan: added tests, updated tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2730
Summary:
Column conversions involve changes to metadata tables such as
_grist_Tables_column. When fetched (from GranularAccess), ExpandedQuery used to
fail with 'table not found' because there is no metadata for metadata tables.
This diff limits the need for metadata in ExpandedQuery to when it's actually
needed (to implmement some formulas for on-demand tables), which no longer
interferes with GranularAccess.
Test Plan: Added a test case that reproduces the issue before the fix.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2729
Test Plan: Added a test case to tickle the bug this was causing.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2718
Summary:
The way linkId was set on actions to tie them together for undo bundling was
incorrect. This diff fixes it by moves the setting of linkIds to Sharing.ts,
which already serializes the processing of actions.
Test Plan: Added a test case for submitting actions together while bundling (which fails without this change).
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2716
Summary:
- This replaces the message "Unexpected Error / Access Denied / Report a problem" with a
one-line "Blocked by access rules".
Test Plan: Only tested manually
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2712
Summary:
This redefines `newRec` to be the state at the end of a bundle,
for the purposes of modifying a document. Updates and adds tests
for creation/updates of rows that are now more intuitive hopefully.
Test Plan: added tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2707
Summary:
This adds back-end support for query parameters `aclAsUser_` and
`aclAsUserId_` which, when either is present, direct Grist to
process granular access control rules from the point of view
of that user (specified by email or id respectively).
Some front end support is added, in the form of a tag that
shows up when in this mode, and a way to cancel the mode.
No friendly way to initiate the mode is offered yet.
Test Plan: added test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2704
Summary:
The goal is that those who can edit ACL rules can create or change rules for
any resource, even if the rules block their own ability to see the resource.
Test Plan: Added a browser test, and a server test for who can call the new method.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2703
Summary:
This allows a fork to be made by a user if:
* That user is an owner of the document being forked, or
* That user has full read access to the document being forked.
The bulk of the diff is reorganization of how forking is done. ActiveDoc.fork is now responsible for creating a fork, not just a docId/urlId for the fork. Since fork creation should not be limited to the doc worker hosting the trunk, a helper endpoint is added for placing the fork.
The change required sanitizing worker allocation a bit, and allowed session knowledge to be removed from HostedStorageManager.
Test Plan: Added test; existing tests pass.
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2700
Summary:
This adds endpoints that allow the support user to remove unlisted
snapshots for a document, and to remove all action history for
a document.
This does increase what the support user can do, but not in a way
that would be particularly valuable to attack. It would have some
destructive value, for removing history (removing unlisted
snapshots doesn't impact the user, by contrast).
This would simplify some maintenance operations.
Test Plan: added test for snapshots; tested states manually
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2699
Summary:
- Placed rule-updating functions in acl.py.
- Reset UI when rules update externally, or alert the user to reset if there
are pending local changes.
- Removed some unused and distracting bits from client-side DocModel.
A few improvements related to poor error handling:
- In case of missing DocActions (tickled by broken ACL rule handling), don't
add to confusion by attempting to process bad actions
- In case of missing attributes in ACL formulas, return undefined rather than
fail; the latter creates more problems.
- In case in invalid rules, fail rather than skip; this feels more correct now
that we have error checking and recovery option, and helps avoid invalid rules.
- Prevent saving invalid rules with an empty ACL formula.
- Fix bug with rule positions.
Test Plan: Added a python and browser test for table/column renames.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2698
Summary:
- Add headers to tables.
- Change styles to reduce boxes-within-boxes.
- Add validation of table and column IDs, both in UI and on server when saving rules.
- Add autocomplete for tables/columns used for UserAttribute rules.
- Add a fancy widget to set permission bits.
Test Plan: Updated browser test for new UI, added a test case for user attributes.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2695
Summary:
Currently, if a document is created by importing a file, inventory
creation is a little haphazard - it works, but triggers a
"surprise" message. This diff makes initialization of inventory
explicit, so that surprise messages shouldn't happen during
document creation.
Test Plan: manual
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2696
Summary:
This adds a snapshots/remove and states/remove endpoint, primarily
for maintenance work rather than for the end user. If some secret
gets into document history, it is useful to be able to purge it
in an orderly way.
Test Plan: added tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2694
Summary:
After switch to using an inventory file, old document versions were
not in fact being pruned. This corrects that and adds a test
that fails with the previous implementation.
The pruner was operating correctly, but was being applied to an
inventory list rather than s3 directly - and the inventory list
did not pass through version removals to s3.
This fix will leave a stock of undeleted versions that can
be eliminated by an external script (there are alternatives
but that seems simplest overall).
Test Plan: updated test
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2693
Summary:
The client relies on metadata tables for laying out pages and sections.
These tables are filtered according to what tables the user has access
to, in a crude way. This diff updates the logic to at least support
the table wildcard.
Test Plan: added tests
Reviewers: dsagal
Reviewed By: dsagal
Differential Revision: https://phab.getgrist.com/D2690
Summary:
- Fix error-handling in bundleActions(), and wait for the full bundle to complete.
(The omissions here were making it impossibly to react to errors from inside bundleActions())
- Catch problematic rules early enough to undo them, by trying out ruleCollection.update()
on updated rules before the updates are applied.
- Added checkAclFormula() call to DocComm that checks parsing and compiling
formula, and reports errors.
- In UI, prevent saving if any aclFormulas are invalid, or while waiting for the to get checked.
- Also fixed some lint errors
Test Plan: Added a test case of error reporting in ACL formulas.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D2689
