The authorization logic determines whether the user has already
successfully authenticated by looking to see if `req.session.user`
exists. If an authentication plugin says that it successfully
authenticated the user but it did not create `req.session.user` then
authentication will re-run for every access, and authorization plugins
will be unable to determine whether the user has been authenticated.
Return a 500 internal server error to prevent these problems.
Before it only logged an error like this:
SyntaxError: Unexpected string in JSON at position XYZ
Now it also logs the filename, making it easier to figure out where
the bad data is:
failed to read file /path/to/etherpad-lite/src/locales/en.json: SyntaxError: Unexpected string in JSON at position XYZ
package to reduce http requests: nice-select,
pad_automatic_reconnect, skin_variants, scroll, caretPosition
rename unorm in tar.json so it can be included
Before, anyone who could create a socket.io connection to Etherpad
could read, modify, and create pads at will without authenticating
first.
The `checkAccess` middleware in `webaccess.js` normally handles
authentication and authorization, but it does not run for `/socket.io`
requests. This means that the connection handler in `socketio.js` must
handle authentication and authorization. However, before this change:
* The handler did not require a signed `express_sid` cookie.
* After loading the express-session state, the handler did not check
to see if the user had authenticated.
Now the handler requires a signed `express_sid` cookie, and it ensures
that `socket.request.session.user` is non-null if authentication is
required. (`socket.request.session.user` is non-null if and only if
the user has authenticated.)
* Move session validity check and session author ID fetch to a
separate function. This separate function can be used by hooks,
making it easier for them to properly determine the author ID.
* Rewrite the remainder of checkAccess. Benefits:
- The function is more readable and maintainable now.
- Vulnerability fix: Before, the session IDs in sessionCookie
were not validated when checking settings.requireSession. Now,
sessionCookie must identify a valid session for the
settings.requireSession test to pass.
- Bug fix: Before, checkAccess would sometimes use the author ID
associated with the token even if sessionCookie identified a
valid session. Now it always uses the author ID associated
with the session if available.
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.
Not all authentication plugins require the Authorization header, so it
might not be present in subsequent attempts. (In particular, a reverse
proxy might strip it.)
* Improve the comment describing how the access check works.
* Move the `authenticate` logic to where it is used so that people
don't have to keep jumping back and forth to understand how the
access check works.
* Break up the three steps to reduce the number of indentation
levels and improve readability. This should also make it easier to
implement and review planned future changes.
Includes settings
Includes i18n
Includes a nice notification
Disconnects on rate limit
Includes feeding into metrics/stats
Include console warn to server console.
