If `settings.json` contains a user without a `password` property then
nobody should be able to log in as that user using the built-in HTTP
basic authentication. This is true both with and without this change,
but before this change it wasn't immediately obvious that a malicious
user couldn't use an empty or null password to log in as such a user.
This commit adds an explicit nullish check and some unit tests to
ensure that an empty or null password will not work if the `password`
property is null or undefined.
Rewrite the `callAll` and `aCallAll` functions to support all
reasonable hook behaviors and to report errors for unreasonable
behaviors (e.g., calling the callback twice).
Now a hook function like the following works as expected when invoked
by `aCallAll`:
```
exports.myHookFn = (hookName, context, cb) => {
cb('some value');
return;
};
```
* don't include sendkeys in index.html as it's included in helper.init
mocha opts: add default timeout and replace ignoreLeaks with checkLeaks,
as the former is deprecated
* introduce helper.edit to write to a pad
* add test to check if helper.edit() supports line numbers
* helper tests: waitFor/waitForPromise seem to be a little bit faster sometimes
* tests: refactor chat.js
* tests: refactor timeslider_numeric_padID
* tests: refactor timeslider_labels
* tests: refactor timeslider_follow
* ensure followContents is enabled, although it should be by default
* timeslider_follow: increase number of revision for Edge
* make textLines() depend on linesDiv()
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* make linesDiv return standard Array
* use `contain` instead of `indexOf`
* more fixes from the review
* review fixes
* align waitFor and waitForPromise behaviour
* timeslider_follow: check if it's following to the correct lines
* lower expected waitFor/waitForPromise interval check
* disable responsivness and regression test in timeslider_follow
* timeslider_follow: fix Range detection
* more explicit test for linesDiv
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* Avoid a false positive if a Promise that is expected to reject
doesn't reject.
* Use modern JavaScript language features: arrow functions,
`const`/`let` instead of `var`.
* Remove the tests that test Promise behavior.
* Add new test that checks that it returns a Promise.
There are a few problems with sleeping before checking the condition
for the first time:
* It slows down tests.
* The predicate is never checked if the interval duration is greater
than the timeout.
* 0 can't be used to test if the condition is currently true.
There is a minor disadvantage to sleeping before checking: It will
cause more tests to run without an asynchronous interruption, which
could theoretically mask some async bugs.
The `helper.waitFor()` function returns a jQuery Deferred object.
Deferred objects are supposed to have a `.fail()` method that is
chainable (it should return `this`). Before this change,
`helper.waitFor()` monkey-patched the `.fail()` method with a function
that returned `undefined`. Now the monkey-patched `.fail()` returns
the Deferred object.
Also modernize the code a bit.
This will be a breaking change for some people.
We removed all internal password control logic. If this affects you, you have two options:
1. Use a plugin for authentication and use session based pad access (recommended).
1. Use a plugin for password setting.
The reasoning for removing this feature is to reduce the overall security footprint of Etherpad. It is unnecessary and cumbersome to keep this feature and with the thousands of available authentication methods available in the world our focus should be on supporting those and allowing more granual access based on their implementations (instead of half assed baking our own).
This makes it easier to see the test results, and it hides some
scary-looking but intentional error messages.
This code will likely have to be updated if/when we change the logging
library (see issue #1922).
This makes it possible to test various settings combinations and
examine internal state to confirm correct behavior. Also, the user
doesn't need to start an Etherpad server before running these tests.
There's no need to perform an authentication check in the socket.io
middleware because `PadMessageHandler.handleMessage` calls
`SecurityMananger.checkAccess` and that now performs authentication
and authorization checks.
This change also improves the user experience: Before, access denials
caused socket.io error events in the client, which `pad.js` mostly
ignores (the user doesn't see anything). Now a deny message is sent
back to the client, which causes `pad.js` to display an obvious
permission denied message.
This also fixes a minor bug: `settings.loadTest` is supposed to bypass
authentication and authorization checks, but they weren't bypassed
because `SecurityManager.checkAccess` did not check
`settings.loadTest`.
Previously Etherpad would not pass the correct client IP address through and this caused the rate limiter to limit users behind reverse proxies. This change allows Etherpad to use a client IP passed from a reverse proxy.
Note to devs: This header can be spoofed and spoofing the header could be used in an attack. To mitigate additional *steps should be taken by Etherpad site admins IE doing rate limiting at proxy.* This only really applies to large scale deployments but it's worth noting.
Before this change, the authorize hook was invoked twice: once before
authentication and again after (if settings.requireAuthorization is
true). Now pre-authentication authorization is instead handled by a
new preAuthorize hook, and the authorize hook is only invoked after
the user has authenticated.
Rationale: Without this change it is too easy to write an
authorization plugin that is too permissive. Specifically:
* If the plugin does not check the path for /admin then a non-admin
user might be able to access /admin pages.
* If the plugin assumes that the user has already been authenticated
by the time the authorize function is called then unauthenticated
users might be able to gain access to restricted resources.
This change also avoids calling the plugin's authorize function twice
per access, which makes it easier for plugin authors to write an
authorization plugin that is easy to understand.
This change may break existing authorization plugins: After this
change, the authorize hook will no longer be able to authorize
non-admin access to /admin pages. This is intentional. Access to admin
pages should instead be controlled via the `is_admin` user setting,
which can be set in the config file or by an authentication plugin.
Also:
* Add tests for the authenticate and authorize hooks.
* Disable the authentication failure delay when testing.
