Where feasible I put the await at the end of the function to
minimize the impact on latency.
My motivation for this change: Eliminate a race condition in tests I
am writing.
There are two different ways an author ID becomes associated with a
user: either bound to a token or bound to a session ID. (The token and
session ID come from the `token` and `sessionID` cookies, or, in the
case of socket.io messages, from the `token` and `sessionID` message
properties.) When `settings.requireSession` is true or the user is
accessing a group pad, the session ID should be used. Otherwise the
token should be used.
Before this change, the `/p/:pad/import` handler was always using the
token, even when `settings.requireSession` was true. This caused the
following error because a different author ID was bound to the token
versus the session ID:
> Unable to import file into ${pad}. Author ${authorID} exists but he
> never contributed to this pad
This bug was reported in issue #4006. PR #4012 worked around the
problem by binding the same author ID to the token as well as the
session ID.
This change does the following:
* Modifies the import handler to use the session ID to obtain the
author ID (when appropriate).
* Expands the documentation for the SecurityManager checkAccess
function.
* Removes the workaround from PR #4012.
* Cleans up the `bin/createUserSession.js` test script.
Includes settings
Includes i18n
Includes a nice notification
Disconnects on rate limit
Includes feeding into metrics/stats
Include console warn to server console.
Before this change, a client would require two versions of the same assets (with
and without randomVersionString), wasting resources and triggering all sorts of
hard to debug inconsistencies.
This change should have been part of 95fd5ce2a4 and completes it.
Clearing the authorship colors of a document with at least two authors, and then
undoing that action caused a disconnect from the pad.
This change disallows undoing clearing authorship colors in order to prevent
the problem from affecting users, and adds the relative test coverage.
This is a change of behaviour, and is documented in the changelog.
Fixes#2802 (sidestepping it).
For some reason authorInfo is sometimes null, and therefore it is not possible
to get colorId from it.
This resulted in the following stack trace:
[2020-03-16 09:27:17.291] [ERROR] console - (node:1746) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'colorId' of null
at <BASEDIR>/src/node/handler/PadMessageHandler.js:1199:37
at runMicrotasks (<anonymous>)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at async Promise.all (index 0)
at async handleClientReady (<BASEDIR>/src/node/handler/PadMessageHandler.js:1171:5)
[2020-03-16 09:27:17.291] [ERROR] console - (node:1746) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 76)
[2020-03-16 09:27:19.034] [WARN] message - Dropped message, USERINFO_UPDATE Session not ready.[object Object]
Which is due to a bug in Etherpad that we are not going to solve now.
As a workaround, when this happens, let's set the username to "Anonymous" (if
it is not already set), and colorId to the fixed value "#daf0b2". Warning
messages are written in the logs to signal this condition.
This is no definitive solution, but fixes#3612 (via a workaround).
some code chunks previously used `async.parallel` but if you
use `await` that forces them to be run serially. Instead,
you can initiate the operation (getting a Promise) and then
_later_ `await` the result of that Promise.
If you use `await` inside a loop it makes the loop inherently serial.
If you omit the `await` however, the tasks will all start but the loop
will finish while the tasks are still being scheduled.
So, to make a set of tasks run in parallel but then have the
code block after the loop once all the tasks have been completed
you have to get an array of Promises (one for each iteration) and
then use `Promise.all()` to wait for those promises to be resolved.
Using `Array#map` is a convenient way to go from an array of inputs
to the require array of Promises.
Also converted the handler functions that depend on checkAccess() into async
functions too.
NB: this commit needs specific attention to it because it touches a lot of
security related code!
This change extracts the grammar correction performed on the async branch,
anticipating them in a single commit. It cannot be folded with the previous
one, as it is not purely cosmetic.
This change is only cosmetic. Its aim is do make it easier to understand the
async changes that are going to be merged later on. It was extracted from the
original work from Ray Bellis.
To verify that nothing has changed, you can run the following command on each
file touched by this commit:
npm install uglify-es
diff --unified <(uglify-js --beautify bracketize <BEFORE.js>) <(uglify-js --beautify bracketize <AFTER.js>)
This is a complete script that does the same automatically (works from a
mercurial clone):
```bash
#!/usr/bin/env bash
set -eu
REVISION=<THIS_REVISION>
PARENT_REV=$(hg identify --rev "${REVISION}" --template '{p1rev}')
FILE_LIST=$(hg status --no-status --change ${REVISION})
UGLIFYJS="node_modules/uglify-es/bin/uglifyjs"
for FILE_NAME in ${FILE_LIST[@]}; do
echo "Checking ${FILE_NAME}"
diff --unified \
<("${UGLIFYJS}" --beautify bracketize <(hg cat --rev "${PARENT_REV}" "${FILE_NAME}")) \
<("${UGLIFYJS}" --beautify bracketize <(hg cat --rev "${REVISION}" "${FILE_NAME}"))
done
```
This is documented to be more performant.
The substitution was made on frontend code, too (i.e., the one in /static),
because Date.now() is supported since IE 9, and we are life supporting only
IE 11.
Commands:
find . -name *.js | xargs sed --in-place "s/new Date().getTime()/Date.now()/g"
find . -name *.js | xargs sed --in-place "s/(new Date()).getTime()/Date.now()/g"
Not done on jQuery.
The guard condition on count being non negative and < 100 used the wrong
boolean operator. In its form it was impossible.
This error was introduced in 2013, in 5592c4b0fe.
Fixes#3499
Get rid of an else branch to simplify code layout. No functional changes at all.
==============
This series is an attempt to reduce the control structure depth of the code
base, maintaining at the same time its exact same behaviour, bugs included. It
is, in a sense, an initial attempt at a refactoring in the spirit of its
original definition [0].
The idea beyond this refactoring is that reducing the code depth and, sometimes,
inverting some conditions, bugs and logic errors may become easier to spot, and
the code easier to read.
When looked at ignoring whitespace changes, all of these diffs should appear
trivial.
[0] https://refactoring.com/
skinName must be a single string (no directory separators in it) pointing to an
existing directory under /src/static/skins.
In case these conditions are not met, its value is rewritten to "no-skin".
Also, the value of skinName if sent to the client via clientVars for allowing
its use it in the browser.
