Before this commit, webaccess.checkAccess saved the authorization in
user.padAuthorizations[padId] with padId being the read-only pad ID,
however later stages, e.g. in PadMessageHandler, use the real pad ID for
access checks. This led to authorization being denied.
This commit fixes it by only storing and comparing the real pad IDs and
not read-only pad IDs.
This fixes test case "authn user readonly pad -> 200, ok" in
src/tests/backend/specs/socketio.js.
readonly paste links should be readable even if authentication is turned
on, as long as the user provides valid login data.
This test currently fails.
Also test that readonly paste IDs can be exported under the same
condition, which currently succeeds.
This has a few benefits:
* It's more readable: It's easier for a user of the function to know
that they should use `await` when calling the function.
* Stack traces are more useful.
* Some code (e.g., the async npm package) uses introspection to
determine if a function is `async` vs. takes a callback.
Benefits of this change:
* It avoids race conditions with tests that clear cookies.
* Any attempt to get or set a value before `init()` is called will
throw an error, ensuring the API is used properly.
* Improved readability: It's easier to understand what the
`pad.noCookie` check is doing.
* Rename "current"/"other" to "user0"/"user1".
* Delete unnecessary `_createTokenFor*` functions.
* Rename helper functions to remove unnecessary leading underscore
and for brevity.
* Use jQuery's `.attr()` to build the second iframe.
* Use js-cookie to manipulate the token cookie.
* Don't attempt to set the token cookie if the pad isn't loaded.
* Use the token generated by the pad.
* Only clear the token cookie at path=/.
Move server message queue processing out of `handleUserChanges()` for
the following reasons:
* Fix a race condition: Before this change the client would stop
processing incoming messages and stop sending changes to the
server if a `NEW_CHANGES` message arrived while the user was
composing a character and waiting for an `ACCEPT_COMMIT` message.
* Improve readability: The `handleUserChanges()` function is for
handling changes from the local user, not for handling changes
from other users.
* Simplify the code.
Reusing the same op object for each iteration can result in very weird
behaviors because previously yielded op objects will get a surprise
mutation.
It is unclear why the code was written to reuse the same object. There
was no comment, nor is there a commit message providing rationale (it
has behaved this way since the very first commit). Perhaps the objects
were reused to improve performance (fewer object allocations that need
to be garbage collected). I do expect this change to reduce
performance somewhat, but not enough to warrant reverting this commit.
Safari takes a while to initialize `document.styleSheets`, which
results in a race condition when loading the pad. Avoid the race
condition by accessing the CSSStyleSheet objects directly from the
HTMLStyleElement DOM objects.
* add more endpoints that do not need a session
* Update src/node/hooks/express/webaccess.js
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* Update src/node/hooks/express/webaccess.js
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
Co-authored-by: John McLear <john@mclear.co.uk>
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
This change makes no visual difference right now, but will matter (for
reasons I don't understand) once we change `ace.js` to build the
iframes by constructing elements in JavaScript (vs. writing HTML).
* Add the class "pad" to the `<html>` tag in `pad.html` (the outer
iframe's parent).
* Change the CSS selector that refers to the `<html>` tag in
`pad.html` from `html:not(.inner-editor)` to `html.pad`.
* Change the class name of the outer iframe's `<html>` tag from
"inner-editor" to "outer-editor".
* Update CSS rules to use the new class name.
This reverts commit a17f9bf3cf, which
caused a mysterious bug with the line numbers. Revert to avoid
blocking a new release while I figure out the bug.
There are two main benefits:
* HTML is no longer printed in the startup debug logs.
* `require()` is no longer called on client-side files. This
eliminates "Failed to load <file> for <plugin>: ReferenceError:
window is not defined" errors when users visit
`/admin/plugins/info`.
Constructing a relative pathname on Windows is problematic because the
two absolute pathnames might be on different drives (or UNC paths).
Use `path.resolve()` instead of `path.join()` where appropriate to
avoid the need to construct a relative path.
The intention of the deleted code was to reduce the number of fetches,
but it only saved a single fetch due to implementation flaws. The
right way to reduce the number of fetches is to use a bundling
technology such as webpack, and this change makes it easier to do so.
This isn't strictly necessary right now, but will become
necessary (due to a Safari quirk) when we change to building the
iframes programmatically (vs. the current `document.write()`
approach).
* tests: Frontend test Windows ZIP
This PR introduces Frontend testing within Github actions!
We're depending a lot on saucelabs recently and that's fine but sometimes we just want to quickly do a frontend simple test on a weird environment (IE windows build) so this PR solves that problem.
Things to note.
It still builds the windows .zip if the cypress tests fail.
It does not add any heavy deps to Etherpad as cypress must be installed in CI.
Cypress is responsible for running the Etherpad instance.
It's up to us how much we use this or not, I know it introduces a bunch of technical debt but I tried to keep that a minimum by compartmentalizing things and documenting where required.
* Update .github/workflows/windows-zip.yml
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* remove timeouts
* Move folder structure up a level
* Update windows-zip.yml
* Update test.js
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
Outputs a list of pluginnames and email address for maintainers to contact. Useful for me to bump folks to maintain there stuff and stop it getting stale :)
* Lint functions
* Fix assignment of `settings.minify`
* Use a for loop to avoid copied code for the `minify = true` and
`minify = false` cases
* Put each resource fetch into its own test case
* Check for 200 status code
* Use `.expect()` to check header value
* Use `.expect(fn)` instead of `.then(fn)`
* docs: fix links from TOC to Headings
* docs: Styling
Just a little modernisation of the appearance of the documentation
* Update src/bin/doc/package.json
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
Firefox 52 has issues with rendering SVG animations which caused random tests to fail. Less than 2% of total Firefox users now use Firefox 52 so we're safe to drop testing for it.
The testing approach was redone to fix numerous issues:
* Even if the tests had been working, none of them would have caught
https://github.com/ether/etherpad-lite/issues/4808 because they
didn't exercise the client-side import logic. Now they do.
* Follow-up logic was not in the `helper.waitFor()` callback like it
should have been. Now the code uses `async` and `await` to ensure
proper execution order.
* All `$.ajax()` calls used `async: false`. Now they're properly
asynchronous.
* The `helper.waitFor()` condition callbacks threw instead of
returning false.
* The string comparisons didn't allow for different attribute
order (e.g., `<ol start="1" class="list-number1">` vs. `<ol
class="list-number1" start="1">`). Now `Node.isEqualNode()` is
used to reduce fragility. (`Node.isEqualNode()` is not perfect, so
the tests are still a bit fragile: If class names or style strings
are in a different order then `Node.isEqualNode()` will return
false even if the nodes are semantically equivalent.)
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* CI: Leave log level at INFO for frontend tests
* CI: Disable frontend admin tests for non-admin workflow
* CI: Disable import/export rate limiting for frontend tests
* tests: fix importexport tests
The testing approach was redone to fix numerous issues:
* Even if the tests had been working, none of them would have caught
https://github.com/ether/etherpad-lite/issues/4808 because they
didn't exercise the client-side import logic. Now they do.
* Follow-up logic was not in the `helper.waitFor()` callback like it
should have been. Now the code uses `async` and `await` to ensure
proper execution order.
* All `$.ajax()` calls used `async: false`. Now they're properly
asynchronous.
* The `helper.waitFor()` condition callbacks threw instead of
returning false.
* The string comparisons didn't allow for different attribute
order (e.g., `<ol start="1" class="list-number1">` vs. `<ol
class="list-number1" start="1">`). Now `Node.isEqualNode()` is
used to reduce fragility. (`Node.isEqualNode()` is not perfect, so
the tests are still a bit fragile: If class names or style strings
are in a different order then `Node.isEqualNode()` will return
false even if the nodes are semantically equivalent.)
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* code tidy up: always evaluates
* tidy up: is always true
* tidy up: remove unused code
* always true/false variables
* unused variable
* tidy up: remove unused code in caretPosition.js
* for squash: Revert "tidy up: remove unused code in caretPosition.js"
The `if` condition was previously always true, so the body should be
preserved. If the body is preserved, other logic can be deleted. I
opened PR #4845 to clean it all up.
This reverts commit 75b03e5a7d.
* for squash: simplify
* for squash: Explain that the getter is used for its side effects
It's very weird to call a getter without using its return value. Add a
comment explaining why this is done so that the reader doesn't get
confused.
* for squash: Revert "tidy up: remove unused code"
The exception test was the purpose of the code.
This reverts commit 85153b1676.
* for squash: Log the tsort results
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
