mws authentication #8596

webplusai · 2024-09-12T18:42:17Z

No description provided.

stackblitz · 2024-09-12T18:42:19Z

Run & review this pull request in StackBlitz Codeflow.

github-actions · 2024-09-12T18:42:29Z

Confirmed: webplusai has already signed the Contributor License Agreement (see contributing.md)

Jermolene · 2024-09-18T11:33:45Z

Thank you @webplusai this is looking good. I will provide some detailed feedback shortly.

…s permission to access on index page

joshuafontany · 2024-09-23T19:52:14Z

Following along (very busy at work atm, company is reorging... again). This is exciting stuff!

Jermolene

Thank you @webplusai this is looking good. There is what looks like some work-in-progress areas such as handleLogin. It makes a lot of sense to keep things simple there temporarily, and so I would like to merge this. However I think it may just be worth you adding comments to tag the TODO areas to make explicit the areas that we need to revisit.

plugins/tiddlywiki/authentication/tiddlers/login.tid

plugins/tiddlywiki/authentication/plugin.info

Jermolene

Thank you @webplusai that's great. I think the best way forwards now is to merge this and discuss further refinements. However at the moment the tests are failing, if you could kindly fix them up then I'll go ahead and merge.

webplusai · 2024-10-01T07:13:17Z

@Jermolene I've fixed the test issue. Also some issues that I had related to the user list and roles were also fixed.

Jermolene · 2024-10-01T10:41:10Z

Thank you @webplusai. As discussed I am keen to get this merged. There are some points I would like to return to but we can worry about those later.

I think the blocker now is that we've got users who are testing the multi-wiki-support branch, and it is going to be a bad experience for them if they upgrade and then are presented with a login dialogue, with no obvious way to create or access a user account.

Ideally, we should get to the point where by default the user gets the previous experience of anonymous read/write access, with an obvious route to get to an admin screen so that they can enable password access and try things out.

One possibility would be to add commands like --mws-add-user, --mws-list-users etc. and use them to set up a demo configuration in a new npm command.

Let me know if that makes sense, ideally we should discuss the details before spending too much time on implementation.

…sions

webplusai · 2024-10-02T09:32:32Z

@Jermolene I've added the commands as you indicated.

pmario

I did only have a closer look at the code, where I did add some comments. I will have a closer look soon.

pmario · 2024-10-02T09:06:27Z

plugins/tiddlywiki/multiwikiserver/modules/commands/mws-add-user.js

+      return "Usage: --mws-add-user <username> <password> [email]";
+    }
+
+    if(!$tw.mws || !$tw.mws.store || !$tw.mws.store.sqlTiddlerDatabase) {


I think the usernames and passwords should never be stored with the content. So for me !$tw.mws.store.sqlTiddlerDatabase looks like it is in the same database file?

Hi @pmario I think it's perfectly OK for this data to be in the same database. There's no more obvious way for queries of the tiddler database to accidentally start returning results from the user database than there is for any data corruption within a program.

What I do want to pay attention to is the best practice of making the admin UI run off of a different HTTP host/port, so that administrators can use external tools to lock down admin access.

But this will prevent us from using different authentication and authorisation services / mechanisms in the future.

Using Usernames and passwords is notoriously unsecure and imo outdated already today. The minimal requirement I would consider somewhat save is username/password in a combination with a time based 2-factor-code generated with an authenticator app.

The plan is to have pluggable authentication schemes. Form-based authentication is still pretty standard for website logins and so I don't think we can avoid offering it, but I definitely want to support things like logging in with Google or GitHub credentials.

pmario · 2024-10-02T09:08:38Z

plugins/tiddlywiki/multiwikiserver/modules/commands/mws-add-user.js

+
+    var username = this.params[0];
+    var password = this.params[1];
+    var email = this.params[2] || username + "@tiddlywiki.com";


If username + @tiddlywiki.com is a placeholder, imo it should be example.com -- Otherwise it means, that tiddlywiki.com also provided an email-service. Which is not the case

Perhaps use example.com for the moment?

pmario · 2024-10-02T09:20:51Z

plugins/tiddlywiki/multiwikiserver/modules/commands/mws-add-user.js

+    var username = this.params[0];
+    var password = this.params[1];
+    var email = this.params[2] || username + "@tiddlywiki.com";
+    var hashedPassword = crypto.createHash("sha256").update(password).digest("hex");


Simple sha256 hashing the password is not enough. The password needs to be salted and then encrypted at least 10000 times. Key-stretching makes it harder to use rainbow-tables and brute force password attacks.

I'm comfortable with having a placeholder implementation to begin with. At this point, I think clarity is the most important consideration, and with adequate encapsulation it will be straightforward for us to revisit the implementation.

The problem is, that it should be done right from the beginning, because the salt has to be stored with the user data. It also plays into the login form. So there are at least 3 different positions, where a change here causes changes up to the UI password handling.

@pmario the challenge is that we need to merge this PR to get feedback. If we wait until we've got a production ready authentication system then this PR would have to stay open for a long time, making all other work on MWS much more complicated. At this point we have the freedom that if required MWS can nuke the user database on an upgrade. We will lose that freedom as MWS moves beyond the experimental stage, so we should take advantage of it now to improve our overall velocity.

pmario · 2024-10-02T09:50:57Z

@webplusai ... Is there some description which mechanisms are used with the whole authentication system?

@Jermolene ... As I wrote sensible data should not be part of the same database as the content. They should be completely separated. IMO this will also help to use other systems that provide secure vaults in the future.

pmario · 2024-10-02T11:25:04Z

I did run: npm run mws-add-user
Then I did run npm start

Is this order right, or did I do it wrong?

Which does start the server and I do get the user UI. But if I try to create a new bag, I do get a server-runtime error

syncer-server-filesystem: Dispatching 'save' task: $:/StoryList
Serving on http://127.0.0.1:8080
(press ctrl-C to exit)
$:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31
    var partiallyDecoded = entityName.replace(/%3A/g, ":");
                                      ^

TypeError: Cannot read properties of undefined (reading 'replace')
    at exports.middleware ($:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31:39)
    at exports.handler ($:/plugins/tiddlywiki/multiwikiserver/routes/handlers/post-bag.js:31:2)
    at IncomingMessage.<anonymous> ($:/plugins/tiddlywiki/multiwikiserver/mws-server.js:525:10)
    at IncomingMessage.emit (node:events:519:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v20.14.0
PS E:\git\tiddly\tiddlywiki\TiddlyWiki5>

Jermolene · 2024-10-02T11:47:45Z

Thanks @webplusai

Great to have your feedback @pmario, this is a great stage to be hammering out some important details.

pmario · 2024-10-02T11:28:59Z

plugins/tiddlywiki/multiwikiserver/auth/form/login/form.tid

@@ -0,0 +1,10 @@
+title: $:/plugins/tiddlywiki/multiwikiserver/auth/form/login/form


Only 1 empty line is needed after the tiddler-header

please check all you files to use tabs for indentation.

pmario · 2024-10-02T11:30:13Z

plugins/tiddlywiki/multiwikiserver/modules/commands/mws-add-permission.js

+Command to create a permission
+
+\*/
+(function(){


Tiddlywiki code uses tabs for indentation. Tab width = 4

The first level inside the (function(){ wrapper is not indented. So eg: export.info = ... does not need a leading tab.

pmario · 2024-10-02T11:41:38Z

plugins/tiddlywiki/multiwikiserver/modules/mws-server.js

@@ -301,6 +302,22 @@ Server.prototype.addRoute = function(route) {
 	this.routes.push(route);
 };

+Server.prototype.verifyPassword = function(inputPassword, storedHash) {


Should authentication be part of this file. IMO it should be it's own file and it should be required. So we get a clear separation of concerns. It should also help to keep the api cleaner.

I think that's a good idea @pmario

pmario · 2024-10-02T11:43:24Z

plugins/tiddlywiki/multiwikiserver/modules/routes/handlers/get-bag.js

-					"bag-tiddlers": JSON.stringify(bagTiddlers)
+(function () {
+
+	/*jslint node: true, browser: true */


The first level inside the wrapper function should not be indented. Otherwise the diff does not show, what really changed

pmario · 2024-10-02T11:45:17Z

plugins/tiddlywiki/multiwikiserver/modules/routes/handlers/get-users.js

+
+\*/
+(function() {
+


indentation

pmario · 2024-10-02T11:47:07Z

plugins/tiddlywiki/multiwikiserver/modules/routes/handlers/put-recipe-tiddler.js

-			});
-		} else {
-			response.writeHead(400);
+(function () {


indentation -- please check all files

pmario · 2024-10-02T11:51:01Z

plugins/tiddlywiki/multiwikiserver/templates/manage-roles.tid

@@ -0,0 +1,123 @@
+title: $:/plugins/tiddlywiki/multiwikiserver/templates/manage-roles
+
+\define add-role-actions()


use tabs instead of spaces for indent - please check all files

webplusai · 2024-10-04T08:27:39Z

I did run: npm run mws-add-user
Then I did run npm start

Is this order right, or did I do it wrong?

Which does start the server and I do get the user UI. But if I try to create a new bag, I do get a server-runtime error

syncer-server-filesystem: Dispatching 'save' task: $:/StoryList
Serving on http://127.0.0.1:8080
(press ctrl-C to exit)
$:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31
    var partiallyDecoded = entityName.replace(/%3A/g, ":");
                                      ^

TypeError: Cannot read properties of undefined (reading 'replace')
    at exports.middleware ($:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31:39)
    at exports.handler ($:/plugins/tiddlywiki/multiwikiserver/routes/handlers/post-bag.js:31:2)
    at IncomingMessage.<anonymous> ($:/plugins/tiddlywiki/multiwikiserver/mws-server.js:525:10)
    at IncomingMessage.emit (node:events:519:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v20.14.0
PS E:\git\tiddly\tiddlywiki\TiddlyWiki5>

@pmario you did it correct. There was an error and I fixed it.

webplusai · 2024-10-04T08:28:17Z

@Jermolene @pmario
All of the major comments are fixed except the ones that @Jermolene mentioned to do later.

webplusai · 2024-10-08T07:56:39Z

Hi @pmario
I've used tabs for indent.

Jermolene · 2024-10-08T14:22:54Z

Hi @webplusai I checked out the latest commits, ran npm install and then npm test, but received the following error:

Running Server Test: Check index page
Failed "Check index page" with "Test failed"
Running Server Test: Upload a 1px PNG
$:/plugins/tiddlywiki/multiwikiserver/commands/mws-test-server.js:137
                        return jsonData["imported-tiddlers"] && $tw.utils.isArray(jsonData["imported-tiddlers"]) && jsonData["imported-tiddlers"][0] === "One White Pixel";
                                       ^

TypeError: Cannot read properties of undefined (reading 'imported-tiddlers')
    at Object.expectedResult ($:/plugins/tiddlywiki/multiwikiserver/commands/mws-test-server.js:137:19)
    at IncomingMessage.<anonymous> ($:/plugins/tiddlywiki/multiwikiserver/commands/mws-test-server.js:98:33)
    at IncomingMessage.emit (node:events:529:35)
    at endReadableNT (node:internal/streams/readable:1400:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v18.20.3

Jermolene

Thank you @webplusai I've included some comments below, but am continuing my review, and will have more to add.

One thing that I do think we need to address is some documentation:

Update the PR description at mws authentication #8596 to include a brief description of the changes in this PR, and instructions for trying it out
An overview of how roles and authentication are handled (can go into the plugin readme for the moment)
Documentation for the end points and commands. We should make two new tabs within the plugin for the commands and the API. Each command and endpoint should be a separate tiddler, transcluded together into an alphabetical list

Let me know if that makes sense.

Jermolene · 2024-10-08T14:32:37Z

boot/boot.js

@@ -336,7 +336,7 @@ Get the browser location.hash. We don't use location.hash because of the way tha
 */
 $tw.utils.getLocationHash = function() {
 	var href = window.location.href;
-	var idx = href.indexOf('#');
+	var idx = href.indexOf("#");


There seem to be a few of these corrections to existing core code. These corrections definitely need to be made, but don't belong in this PR and should be submitted to "master" as a separate PR that I can merge first.

Jermolene · 2024-10-08T14:33:29Z

editions/multiwikiserver/tiddlywiki.info

@@ -11,6 +11,18 @@
 		"tiddlywiki/snowwhite"
 	],
 	"build": {
+		"--mws-list-users": [


I think it would be confusing to have a named build that matches a built-in command. Perhaps this one isn't necessary in any case?

Jermolene · 2024-10-08T14:34:22Z

editions/multiwikiserver/tiddlywiki.info

+		"--mws-list-users": [
+			"--mws-list-users"
+		],
+		"mws-add-user": [


At the moment there is no parameterisation of build commands, so I can't see how this one could be used in practice. Is it intended to be part of the documentation?

Jermolene · 2024-10-08T14:43:14Z

plugins/tiddlywiki/multiwikiserver/modules/mws-server.js

+			} else {
+					console.log(`Invalid return URL detected: ${returnUrl}. Redirecting to home page.`);
+			}
+			response.setHeader('Set-Cookie', `returnUrl=${encodeURIComponent(sanitizedReturnUrl)}; HttpOnly; Path=/`);


Should we include the Secure and SameSite flags?

Jermolene · 2024-10-08T14:45:15Z

plugins/tiddlywiki/multiwikiserver/auth/form/login/form.tid

@@ -0,0 +1,9 @@
+title: $:/plugins/tiddlywiki/multiwikiserver/auth/form/login/form
+
+<$macrocall $name="loginForm"/>


What is the purpose of this macro call?

Jermolene · 2024-10-08T15:00:35Z

plugins/tiddlywiki/multiwikiserver/modules/mws-server.js

@@ -34,6 +34,7 @@ function Server(options) {
 	this.authenticators = options.authenticators || [];
 	this.wiki = options.wiki;
 	this.boot = options.boot || $tw.boot;
+	this.sqlTiddlerDatabase = $tw.mws.store.sqlTiddlerDatabase;


It would be better to make the database be one of the options properties, and to amend the call sites to explicitly pass the database:

``
this.sqlTiddlerDatabase = options.sqlTiddlerDatabase || $tw.mws.store.sqlTiddlerDatabase;

Jermolene · 2024-10-08T15:02:24Z

plugins/tiddlywiki/multiwikiserver/modules/mws-server.js

+
+Server.prototype.redirectToLogin = function(response, returnUrl) {
+	if (!response.headersSent) {
+			const validReturnUrlRegex = /^\/(?!.*\.(ico|png|jpg|jpeg|gif|svg|css|js|woff|woff2|ttf|eot|json)$).*$/;


What is the purpose of this sanitisation?

The url sanitization was done to prevent the user from being redirected to a file url instead of a page.

Jermolene · 2024-10-08T15:05:38Z

plugins/tiddlywiki/multiwikiserver/modules/routes/handlers/post-bag.js

@@ -26,6 +28,7 @@ exports.bodyFormat = "www-form-urlencoded";
 exports.csrfDisable = true;

 exports.handler = function(request,response,state) {
+	aclMiddleware(request, response, state, "bag", "WRITE");


It looks like the middleware call can fail authentication, and send a response to the client. Presumably we would need some processing here to respond to the outcome of the middleware.

Is it necessary for each route to invoke the middleware explicitly like this? I was hoping the main handler in mws-server.js could handle the middleware more or less invisibly from the point of view of the routes.

Jermolene · 2024-10-08T15:10:41Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-database.js

@@ -117,6 +214,16 @@ SqlTiddlerDatabase.prototype.createBag = function(bag_name,description,accesscon
 		$accesscontrol: accesscontrol,
 		$description: description
 	});
+
+
+	// update the permissions on ACL records


There are still some minor style issues. Comments should be capitalised, and blank lines should not be used so liberally. Generally, we prefer to use comments to delineate sections of code.

Jermolene · 2024-10-08T15:11:16Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-database.js

+
+	const entityInfo = entityTypeToTableMap[entityType];
+	if (!entityInfo) {
+		throw new Error('Invalid entity type: ' + entityType);


I appreciate its a nuisance, but there are still a lot of string constants using single quotes instead of the preferred double quotes.

Jermolene · 2024-10-08T15:12:07Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-database.js

+		throw new Error('Invalid entity type: ' + entityType);
+	}
+
+	// if the entityName starts with "$:/", we'll assume its a system tiddler, then grant the user permission


I think this is intended to be special treatment for system bags and recipes, not tiddlers.

webplusai · 2024-10-10T09:06:41Z

@Jermolene I've fixed all of your comments.

mws authentication

9583fda

webplusai added 3 commits September 13, 2024 18:40

add more tests and permission checkers

9b69959

add logic to ensure that only authenticated users' requests are handled

d331115

add custom login page

4cb0379

webplusai added 4 commits September 18, 2024 19:07

Implement user authentication as well as session handling

c5bc0df

work on user operations authorization

0f0d8be

add middleware to route handlers for bags & tiddlers routes

0885eda

add feature that only returns the tiddlers and bags which the user ha…

4e75360

…s permission to access on index page

Jermolene reviewed Sep 24, 2024

View reviewed changes

plugins/tiddlywiki/authentication/tiddlers/login.tid Outdated Show resolved Hide resolved

plugins/tiddlywiki/authentication/plugin.info Outdated Show resolved Hide resolved

refactor auth routes & added user management page

0932f2c

Jermolene reviewed Sep 30, 2024

View reviewed changes

webplusai added 2 commits September 30, 2024 19:11

fix Ci Test failure issue

887025b

fix users list page, add manage roles page

316aa01

webplusai added 2 commits October 2, 2024 08:14

add commands and scripts to create new user & assign roles and permis…

c5053f5

…sions

resolved ci-test failure

6a9dcac

pmario reviewed Oct 2, 2024

View reviewed changes

webplusai added 2 commits October 3, 2024 18:23

add ACL permissions to bags & tiddlers on creation

13d7cd9

fix comments and access control list bug

18f97ab

webplusai added 3 commits October 7, 2024 08:09

fix indentation issues

1a1675a

fix conflicts

b61789b

working on user profile edit

f02c856

Jermolene reviewed Oct 8, 2024

View reviewed changes

remove list users command & added support for database in server options

81f73de

implement user profile update and password change feature

851a7ab

		@@ -0,0 +1,10 @@
		title: $:/plugins/tiddlywiki/multiwikiserver/auth/form/login/form

		@@ -0,0 +1,123 @@
		title: $:/plugins/tiddlywiki/multiwikiserver/templates/manage-roles

		\define add-role-actions()

		@@ -0,0 +1,9 @@
		title: $:/plugins/tiddlywiki/multiwikiserver/auth/form/login/form

		<$macrocall $name="loginForm"/>


		\*/
		(function() {

mws authentication #8596

Are you sure you want to change the base?

mws authentication #8596

Conversation

webplusai commented Sep 12, 2024

stackblitz bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

Jermolene commented Sep 18, 2024

joshuafontany commented Sep 23, 2024

Jermolene left a comment

Choose a reason for hiding this comment

Jermolene left a comment

Choose a reason for hiding this comment

webplusai commented Oct 1, 2024

Jermolene commented Oct 1, 2024

webplusai commented Oct 2, 2024

pmario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmario commented Oct 2, 2024 • edited Loading

pmario commented Oct 2, 2024 • edited Loading

Jermolene commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webplusai commented Oct 4, 2024

webplusai commented Oct 4, 2024

webplusai commented Oct 8, 2024

Jermolene commented Oct 8, 2024

Jermolene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webplusai commented Oct 10, 2024

pmario commented Oct 2, 2024 •

edited

Loading

pmario commented Oct 2, 2024 •

edited

Loading