✨ Webdav connector to the Twake Drive #643

rumata-feathers · 2024-08-27T14:15:15Z

WebDAV implementation

Description

This is implementation the WebDAV protocol for our application using the nephele library.
Key components of this implementation include:

Creating new 'webdav' service
Custom adapter class to interface
Resource class wrapper for drive items
Device authorisation
Implementing locks for the drive items

Related Issue

#573

Motivation and Context

WebDAV protocol is needed for desktop synchronisation

How Has This Been Tested?

Manual testing using curl requests to simulate various WebDAV operations
Practical testing by mounting the WebDAV server as a network drive:
- Tested on macOS Sonoma
- Tested file creation, deletion, and modification through the mounted drive

Testing environment:

Native explorer
ACCOUNTS_TYPE=internal

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added the Signed-off-by statement at the end of my commit message.

…on and routing

…the stream immediately before it's being written

chibenwa · 2024-08-27T15:04:43Z

tdrive/backend/node/src/services/documents/entities/drive-file-lock.ts

+  @Column("timeout", "number")
+  timeout: number;


A duration without unit? at least in a comment...

tdrive/backend/node/src/services/documents/entities/drive-file-lock.ts

tdrive/backend/node/src/services/documents/entities/drive-file.ts

chibenwa · 2024-08-27T15:09:58Z

tdrive/backend/node/src/services/user/entities/device.ts

-  @Column("id", "string")
+  @Column("id", "uuid", { generator: "uuid" })


How do we migrate existing data?
Is changing type of a column allowed on a production system?

this, the special ORM doesn't do I think... but that table should be empty for the moment, this is the first actual use, we'll have to remember to test this before deploy

chibenwa · 2024-08-27T15:11:54Z

tdrive/backend/node/src/services/user/entities/device.ts

+  @Column("password", "string", { generator: "uuid" })
+  password: string;


Is this random enough? For such a sensible use case I would make sure I use cryptography grade random with enough entropy.

Also: does it make sense to store a password in clear? What's the impact if it gets read?

If it is stored only for validation later shouldn't we use bcrypt and friends?

I think we might need a clear text to support digest authentication (if realm is configurable).

The UUID should have enough entropy for crypto https://github.com/uuidjs/uuid

@rumata-feathers, could you please take a look at how password is encoded in the "user" entity, and do the same with the same salt and the same passwordEncode component

chibenwa · 2024-08-27T15:14:21Z

tdrive/backend/node/src/services/webdav/web/adapter.ts

+export const adapterServiceReady = initializeAdapterService();
+
+export const getDriveExecutionContext = (url: URL): DriveExecutionContext => ({
+  //TODO[ASH] username will be actually a device, and we will need to get real user by device


Still relevant? Remove TODO?

chibenwa

Partial review.
Disclaimer: I do not know tdrive, I do not know dav.
Feel free to discard comments if irrelevant.
Also: no tests? I see complex logic all across the place and many way things can go bust. It would be invaluable to invest in HTTP layer tests exercising this brand new DAV extension.
(from my experience standards needs extensive tests)

ericlinagora · 2024-08-27T17:26:11Z

tdrive/backend/node/src/core/platform/services/webserver/index.ts

@@ -116,7 +116,7 @@ export default class WebServerService extends TdriveService<WebServerAPI> implem
      },
    });
    this.server.register(formbody);
-    this.server.register(corsPlugin, this.configuration.get<FastifyCorsOptions>("cors", {}));
+    // this.server.register(corsPlugin, this.configuration.get<FastifyCorsOptions>("cors", {}));


Am I reading this right ? Does this disable CORS site-wide ????

yes, it should be reconfigured only for the webdav uri

i'm surprised webdav clients care at all about CORS

I had similar issues with JMAP

CORS do not makes sense for JMAP so native JMAP clients ignores them

But web based JMAP clients are bound to browser safety limitations hence CORS...

That makes sense, but still I'd prefer if native clients indeed ignore to keep them everywhere, and if/when we do have a browser client to support, and if it is a problem, then we look into removing them for that

ericlinagora · 2024-08-27T17:28:36Z

tdrive/backend/node/src/services/documents/entities/drive-file.ts

@@ -118,6 +119,9 @@ export class DriveFile {
  @Type(() => String)
  @Column("scope", "string")
  scope: DriveScope;
+
+  @Column("locks", "encoded_json")
+  locks: DriveLock[];


If DriveLocks are stored in that json field, why create the entity and db table and all that ?

ericlinagora · 2024-08-27T17:30:07Z

tdrive/backend/node/src/services/documents/services/index.ts

@@ -1317,4 +1328,157 @@ export class DocumentsService {
      throw new CrudException(`Not enough space: ${size}, ${leftQuota}.`, 403);
    }
  };
+
+  /**
+   * Returns the file


I dont think this kind of documentation really adds anything ("returns the file" it's not hard to see the return type). However it would be nice, also eg on exported things like DriveLock

ericlinagora · 2024-08-27T17:31:33Z

tdrive/backend/node/src/services/documents/services/index.ts

+  };
+
+  /**
+   * Copies item to destination


ericlinagora · 2024-08-27T17:32:18Z

tdrive/backend/node/src/services/documents/services/index.ts

+      this.logger.error({ error: `${error}` }, "Failed to grant access to the drive item");
+      throw new CrudException("User does not have access to this item or its children", 401);
+    }
+    return Promise.resolve(driveFile);


No need for Promise.resolve in an async function

ericlinagora · 2024-08-27T17:34:11Z

tdrive/backend/node/src/services/documents/services/index.ts

+  ): Promise<void> => {
+    assert(id != null, "Cannot copy null");
+    const item = await this.get(id, context);
+    assert(item.item.parent_id != null, "Cannot copy root");


what if id is not found ? How do you return say a 404 ? What about the asserts, shouldn't a lot of them be 400/401 type errors ?

ericlinagora · 2024-08-27T19:15:15Z

tdrive/backend/node/src/services/webdav/web/routes.ts

+      reply.send({ message: "Test route" });
+    });
+    fastify.all("webdav", (request, reply) => {
+      reply.send({ message: "Test route" });


ericlinagora · 2024-08-27T19:15:22Z

tdrive/backend/node/src/services/webdav/web/routes.ts

+    fastify.all("webdav", (request, reply) => {
+      reply.send({ message: "Test route" });
+    });
+    console.debug(fastify.printRoutes());


ericlinagora · 2024-08-27T19:15:58Z

tdrive/backend/node/src/services/webdav/web/routes.ts

+
+  return routes;
+}
+export const routes = eval("import('nephele').then(builder)");


I'd really prefer if we did this horrible eval hack in a single place and exported what we need from there, and documented the raison-d'etre

tdrive/backend/node/src/utils/files.ts

ericlinagora · 2024-08-27T19:18:03Z

tdrive/backend/node/tsconfig.json

@@ -16,7 +16,7 @@
              "src/types/*"
          ],
          "src/*": ["src/*"]
-      }
+      },


technically that's not valid json, seems benign, but also, why in this pr ?

ericlinagora · 2024-10-01T11:55:15Z

tdrive/backend/node/src/services/webdav/web/routes.ts

@@ -76,14 +75,6 @@ async function builder(nephele: any) {
      });
      fastify.use(`${webdavUrl}`, webdavMiddleware);
    });
-    fastify.all("webdav/*", (request, reply) => {


apparently this chunk is vital, without it, we get 404s only in the webdav stuff....

shepilov

!

ericlinagora · 2024-10-02T13:43:55Z

.github/workflows/build.yml

@@ -62,7 +62,7 @@ jobs:
      - name: e2e-mongo-s3-test
        run: |
          cd tdrive
-          docker compose -f docker-compose.tests.yml run --rm -e NODE_OPTIONS=--unhandled-rejections=warn -e SEARCH_DRIVER=mongodb -e DB_DRIVER=mongodb -e PUBSUB_TYPE=local node npm run test:all
+          docker compose -f docker-compose.tests.yml run --rm -e "NODE_OPTIONS=--unhandled-rejections=warn --experimental-vm-modules" -e SEARCH_DRIVER=mongodb -e DB_DRIVER=mongodb -e PUBSUB_TYPE=local node npm run test:all


Still needed after the eval evil hack ?

rumata-feathers and others added 21 commits July 29, 2024 10:42

🌟Creating adapter implementation for the nephele server ,authenticati…

61c1fdd

…on and routing

🌟Defining copy, move, find functions for documents service

b043c3c

🌟Creating properties service

b0f7a55

🌟Creating file - resource service

ae0f0b5

🐛 Add error handling

af92790

🐛 Connect webdav service

ead2adf

🌟Implementation of MCKOL method and handling errors as instances

ab4fc40

🌟Implementation of COPY and MOVE methods

863f867

🌟Implementation of PUT and GET methods

04875fb

🐛Add config

441e25d

♻️ backend: change how stream size is calculated to prevent starting …

d787fc1

…the stream immediately before it's being written

🐛 Fixed PUT method (setStream and getStream functions)

c01f47d

🌟 Generate device id

4c6d0d2

♻️Improved Stream uploading

d720b47

🌟Implementing locks and adding new properties handling

e8fd313

🌟Implementing device handling

2620d44

♻️Added company information to device

0dc90e0

♻️Updates packages and dependencies needed for webdav

325928e

♻️Clean-ups

e73e907

♻️Adding mime-type for files

694ec1b

♻️Adding documentation

bf014de

rumata-feathers requested review from MontaGhanmy, shepilov and ericlinagora and removed request for MontaGhanmy and shepilov August 27, 2024 14:15

chibenwa reviewed Aug 27, 2024

View reviewed changes

tdrive/backend/node/src/services/documents/entities/drive-file-lock.ts Show resolved Hide resolved

chibenwa reviewed Aug 27, 2024

View reviewed changes

tdrive/backend/node/src/services/documents/entities/drive-file.ts Show resolved Hide resolved

chibenwa reviewed Aug 27, 2024

View reviewed changes

ericlinagora reviewed Aug 27, 2024

View reviewed changes

ericlinagora requested changes Aug 27, 2024

View reviewed changes

shepilov changed the title ~~Webdav express~~ ✨ Webdav connector to the Twake Drive Aug 28, 2024

✅ github actions: adding flag to load module dynamically

36857f6

ericlinagora mentioned this pull request Aug 28, 2024

🐛 Webdav - PROPFIND Method implementation #576

Closed

8 tasks

Merge branch 'main' into webdav-express

044979b

ericlinagora force-pushed the webdav-express branch from cbe6bf0 to a09fea8 Compare August 28, 2024 12:36

✅ github actions: testing different quoting

06749f6

ericlinagora force-pushed the webdav-express branch from a09fea8 to 06749f6 Compare August 28, 2024 12:39

rumata-feathers added 6 commits August 28, 2024 15:28

♻️Prettifying

fdd5404

♻️Beatifying with lint

7354874

♻️fixing tests

7f43084

♻️fixed documentation

d233eec

♻️Adding testing

662eae8

♻ Fixing testing

1b22ff3

ericlinagora marked this pull request as draft September 2, 2024 22:21

🎨🔇♻️⚰️ backend: minor cleanup (#573)

fc0219c

ericlinagora reviewed Oct 1, 2024

View reviewed changes

🐛 backend: Adding vital routing code back, lest we get 404s (#573)

4462866

shepilov approved these changes Oct 1, 2024

View reviewed changes

shepilov requested changes Oct 1, 2024

View reviewed changes

⬆️ Update of package.json files (#573)

41b8472

ericlinagora force-pushed the webdav-express branch from c7b35d9 to 41b8472 Compare October 1, 2024 17:32

ericlinagora reviewed Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Webdav connector to the Twake Drive #643

✨ Webdav connector to the Twake Drive #643

rumata-feathers commented Aug 27, 2024 •

edited

Loading

chibenwa Aug 27, 2024 •

edited

Loading

chibenwa Aug 27, 2024

ericlinagora Aug 27, 2024

chibenwa Aug 27, 2024

ericlinagora Aug 27, 2024

shepilov Aug 28, 2024

chibenwa Aug 27, 2024

chibenwa left a comment

ericlinagora Aug 27, 2024

shepilov Aug 28, 2024

ericlinagora Aug 28, 2024

chibenwa Aug 28, 2024

ericlinagora Aug 28, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Aug 27, 2024

ericlinagora Oct 1, 2024

shepilov left a comment

ericlinagora Oct 2, 2024

		@Column("id", "string")
		@Column("id", "uuid", { generator: "uuid" })

		@Column("password", "string", { generator: "uuid" })
		password: string;

✨ Webdav connector to the Twake Drive #643

Are you sure you want to change the base?

✨ Webdav connector to the Twake Drive #643

Conversation

rumata-feathers commented Aug 27, 2024 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

chibenwa Aug 27, 2024 • edited Loading

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

chibenwa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepilov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rumata-feathers commented Aug 27, 2024 •

edited

Loading

chibenwa Aug 27, 2024 •

edited

Loading