Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] 433 anonymously uploaded files should appear in trash of folder creator #536

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,31 @@ import search from "./drive-file.search";

export const TYPE = "drive_files";
export type DriveScope = "personal" | "shared";

/**
* This represents an item in the file hierarchy.
*
* DriveFile s do not have a notion of owner, only a creator. The `creator`
* is used to locate the root files. The `parent_id` fields then form the
* hierarchy.
*
* The `is_in_trash` field must be checked when enumerating as DrifeFiles with
* this field set are roots inside the trash folder. Only the root DriveFiles
* that were moved to trash have this set, items below them, while also in the
* trash, do not have this field set.
*
* The `parent_id` can point to the `id` of another `DriveFile`, or one
* of the named entries:
* - `"user_$userid"`: Root of the personal "My Drive" (and Trash) folder of the user in the string.
* Usually this is assumed to be the creator, there is no official way
* of extracting the user id from the `parent_id`.
* - `"root"`: Root of the creator's company "Shared Drive" feature
* - The following virtual values never appear in the stored `parent_id` field but are used
* in queries and URLs etc:
* - `"trash"`: used to query items at the root of the creator with `is_in_trash == true`,
* if `scope == "personal"`, otherwise the trash of the shared drive
* - `"trash_$userid"`: Trash folder for a given user (same note as `"user_$userid"`)
* - `"shared_with_me"`: for the feature of the same name
*/
@Entity(TYPE, {
globalIndexes: [
["company_id", "parent_id"],
Expand Down
54 changes: 52 additions & 2 deletions tdrive/backend/node/src/services/documents/services/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { hasCompanyAdminLevel } from "../../../utils/company";
import gr from "../../global-resolver";
import { DriveFile, TYPE } from "../entities/drive-file";
import { FileVersion, TYPE as FileVersionType } from "../entities/file-version";
import User, { TYPE as UserType } from "../../user/entities/user";

import {
DriveTdriveTab as DriveTdriveTabEntity,
TYPE as DriveTdriveTabRepoType,
Expand Down Expand Up @@ -56,12 +58,14 @@ import {
import archiver from "archiver";
import internal from "stream";
import config from "config";

export class DocumentsService {
version: "1";
repository: Repository<DriveFile>;
searchRepository: SearchRepository<DriveFile>;
fileVersionRepository: Repository<FileVersion>;
driveTdriveTabRepository: Repository<DriveTdriveTabEntity>;
userRepository: Repository<User>;
ROOT: RootType = "root";
TRASH: TrashType = "trash";
quotaEnabled: boolean = config.has("drive.featureUserQuota")
Expand All @@ -88,8 +92,10 @@ export class DocumentsService {
DriveTdriveTabRepoType,
DriveTdriveTabEntity,
);
this.userRepository = await globalResolver.database.getRepository<User>(UserType, User);
} catch (error) {
logger.error({ error: `${error}` }, "Error while initializing Documents Service");
throw error;
}

return this;
Expand Down Expand Up @@ -626,12 +632,12 @@ export class DocumentsService {
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);
}

const path = await getPath(item.parent_id, this.repository, true, context);
const previousParentId = item.parent_id;
if (
(await isInTrash(item, this.repository, context)) ||
item.parent_id === this.TRASH ||
(await getPath(item.parent_id, this.repository, true, context))[0].id === this.TRASH
path[0].id === this.TRASH
) {
//This item is already in trash, we can delete it definitively

Expand Down Expand Up @@ -670,6 +676,50 @@ export class DocumentsService {
} else {
//This item is not in trash, we move it to trash
item.is_in_trash = true;
// Check item belongs to someone
if (item.creator !== context?.user?.id) {
const creator = await this.userRepository.findOne({ id: item.creator });
if (creator.type === "anonymous") {
const loadedCreators = new Map<string, User>();
let firstOwnedItem: DriveFile | undefined;
for (let i = path.length - 1; i >= 0; i--) {
const item = path[i];
if (!item.creator) continue;
const user =
loadedCreators.get(item.creator) ??
(await this.userRepository.findOne({ id: item.creator }));
loadedCreators.set(item.creator, user);
if (user.type !== "anonymous") {
firstOwnedItem = item;
break;
}
}
if (firstOwnedItem) {
const firstKnownCreator = loadedCreators.get(firstOwnedItem.creator);
const accessEntitiesWithoutUser = item.access_info.entities.filter(
({ id, type }) => type != "user" || id != firstKnownCreator.id,
);
item.access_info.entities = [
...accessEntitiesWithoutUser,
// This is not functionally required, but creates an audit trace of what
// happened to this anonymously uploaded file
{
type: "user",
id: firstKnownCreator.id,
level: "manage",
grantor: context.user.id,
ericlinagora marked this conversation as resolved.
Show resolved Hide resolved
},
];
item.creator = firstKnownCreator.id;
} else {
// Move to company trash
item.parent_id = "trash";
item.scope = "shared";
}
await this.repository.save(item);
}
}

await this.update(item.id, item, context);
}
await updateItemSize(previousParentId, this.repository, context);
Expand Down
2 changes: 1 addition & 1 deletion tdrive/backend/node/src/services/documents/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export type DriveItemDetails = {
export type BrowseDetails = DriveItemDetails & { nextPage: Paginable };

export type DriveFileAccessLevel = "read" | "write" | "manage";
export type publicAccessLevel = "write" | "read" | "none";
export type publicAccessLevel = "write" | "read" | "none" | "manage";

export type RootType = "root";
export type TrashType = "trash";
Expand Down
55 changes: 39 additions & 16 deletions tdrive/backend/node/src/services/documents/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,30 +256,44 @@ export const updateItemSize = async (
};

/**
* gets the path for the driveitem
* Get a list of parents for the provided DriveFile id, in top-down order,
* but internally iterated towards the top.
*
* @param {string} id
* @param {Repository<DriveFile>} repository
* @param {boolean} ignoreAccess
* @param {CompanyExecutionContext} context
* @returns
* @param {boolean} ignoreAccess If user from context doesn't have
* read access to an item, the item is not included and traversing
* towards parents is stopped there.
* @param {(item: DriveFile) => Promise<boolean>} predicate If set,
* returned items in the array include only those for which the
* `predicate`'s result resolved to true.
* @param {boolean?} stopAtFirstMatch If true, the lowest item
* in the hierarchy that matches the `predicate` will be the
* only item in the returned array.
* @returns A promise to an array of DriveFile entries in order
* starting from the root (eg. "My Drive"), and ending in the
* DriveFile matching the provided `id` ; both included.
*
* If `stopAtFirstMatch` is true and `predicate` is provided, the
* result is an array with a single item or an empty array.
*/
export const getPath = async (
id: string,
repository: Repository<DriveFile>,
ignoreAccess?: boolean,
context?: DriveExecutionContext,
predicate?: (item: DriveFile) => Promise<boolean>,
stopAtFirstMatch: boolean = false,
): Promise<DriveFile[]> => {
id = id || "root";
if (isVirtualFolder(id))
return !context?.user?.public_token_document_id || ignoreAccess
? [
{
id,
name: await getVirtualFoldersNames(id, context),
} as DriveFile,
]
if (isVirtualFolder(id)) {
const virtualItem = {
id,
name: await getVirtualFoldersNames(id, context),
} as DriveFile;
return (!context?.user?.public_token_document_id || ignoreAccess) &&
(!predicate || (await predicate(virtualItem)))
? [virtualItem]
: [];
}
const item = await repository.findOne({
id,
company_id: context.company.id,
Expand All @@ -288,8 +302,17 @@ export const getPath = async (
if (!item || (!(await checkAccess(id, item, "read", repository, context)) && !ignoreAccess)) {
return [];
}

return [...(await getPath(item.parent_id, repository, ignoreAccess, context)), item];
const isMatch = !predicate || (await predicate(item));
if (stopAtFirstMatch && isMatch) return [item];
const parents = await getPath(
item.parent_id,
repository,
ignoreAccess,
context,
predicate,
stopAtFirstMatch,
);
return isMatch ? [...parents, item] : parents;
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const TYPE = "group_user";
type: TYPE,
})
export default class CompanyUser {
/** company_id */
@Column("group_id", "timeuuid")
group_id: string;

Expand Down
61 changes: 52 additions & 9 deletions tdrive/backend/node/test/e2e/common/user-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@ import {
} from "./entities/mock_entities";
import { logger } from "../../../src/core/platform/framework";
import { expect } from "@jest/globals";
import { publicAccessLevel } from "../../../src/services/documents/types";
import { DriveFileAccessLevel, publicAccessLevel } from "../../../src/services/documents/types";
import { UserQuota } from "../../../src/services/user/web/types";
import { Api } from "../utils.api";
import { OidcJwtVerifier } from "../../../src/services/console/clients/remote-jwks-verifier";

/** The UserApi is an abstraction for E2E tests that
* represents the high level actions a user can take
* in the application.
*/
export default class UserApi {

private static readonly DOC_URL = "/internal/services/documents/v1";
Expand Down Expand Up @@ -54,8 +58,8 @@ export default class UserApi {

private async init(newUser: boolean, options?: {}) {
this.dbService = await TestDbService.getInstance(this.platform, true);
this.workspace = this.platform.workspace;
if (newUser) {
this.workspace = this.platform.workspace;
const workspacePK = {
id: this.workspace.workspace_id,
company_id: this.workspace.company_id,
Expand All @@ -69,7 +73,6 @@ export default class UserApi {
uuidv1());
} else {
this.user = this.platform.currentUser;
this.workspace = this.platform.workspace;
}
this.api = new Api(this.platform, this.user);
this.jwt = await this.doLogin();
Expand Down Expand Up @@ -213,12 +216,13 @@ export default class UserApi {
return files;
};

async createDirectory(parent = "root") {
async createDirectory(parent = "root", overrides?: Partial<DriveFile>) {
const directory = await this.createDocument({
company_id: this.platform.workspace.company_id,
name: "Test Folder Name",
parent_id: parent,
is_directory: true
is_directory: true,
...overrides
}, {});
expect(directory).toBeDefined();
expect(directory).not.toBeNull();
Expand All @@ -227,6 +231,26 @@ export default class UserApi {
return directory;
}

/** Run the provided callback using the specified bearer JWT token */
async impersonateWithJWT<T>(jwt: string, cb: () => Promise<T>): Promise<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed to rollback to the old token.

For anonymous access you can crate a new instance of the UserApi in the test, and store the token permanently.
These callbacks just bring complexity

const anonymousUser = UserApi()
anonymousUser.loginWithToken()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more complicated than that, it's what I wanted to discuss. The UserAPI class has a user object that wouldn't be valid with the other JWT, it also loads workspace and anonymous user stuff which we don't need, and it inits the Api class with the user, which then loads it's jwt on its own. I can't figure out what they're supposed to be but they don't match a user nor a session one to one. This is a hacky way around all that complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This is a hacky way around all that complexity.", exactly, so please let's do not add additional.
Actually it represents a user session in the browser, and sometimes it can be with "real" user or anonymous session.

So we can refactor the constructor of the class, and make some fields optional, but until this, let's KISS
anonymousUser.jwt = token.value;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤮 that's worse ! at least my hack is clean and has no side effects (also I disagree it's complex, it's one callback that's advised), and the call is readable and descriptive. Modifying the field from outside on an object used in multiple tests....

Yes I want to refactor UserApi, and also, remove User-Api, but, we should talk first because I'm not sure about the actual responsibility scope we should aim for

const previous = this.jwt;
this.jwt = jwt;
let result: T | undefined = undefined;
try {
result = await cb();
} finally {
this.jwt = previous;
}
return result;
}

/** Gets the public link access token then `impersonateWithJWT` as an anonymous user with that link */
async impersonatePublicLinkAccessOf<T>(item: Partial<DriveFile> & { id: string }, cb: () => Promise<T>): Promise<T> {
const publicToken = await this.getPublicLinkAccessToken(item);
expect(publicToken?.value?.length ?? "").toBeGreaterThan(0);
return this.impersonateWithJWT(publicToken?.value, cb);
}

async createDocument(
item: Partial<DriveFile>,
version: Partial<FileVersion>
Expand All @@ -241,31 +265,49 @@ export default class UserApi {
return deserialize<DriveFile>(DriveFile, response.body);
};

async createDefaultDocument(): Promise<DriveFile> {
async createDefaultDocument(overrides?: Partial<DriveFile>): Promise<DriveFile> {
const scope: "personal" | "shared" = "shared";
const item = {
name: "new test file",
parent_id: "root",
company_id: this.platform.workspace.company_id,
scope,
...overrides,
};

return await this.createDocument(item, {});
};

async shareWithPublicLink(doc: Partial<DriveFile>, accessLevel: publicAccessLevel) {
async shareWithPublicLink(doc: Partial<DriveFile> & { id: string }, accessLevel: publicAccessLevel) {
return await this.updateDocument(doc.id, {
...doc,
access_info: {
...doc.access_info,
...doc.access_info!,
public: {
...doc.access_info.public!,
...doc.access_info?.public!,
level: accessLevel
}
}
});
}

async shareWithPublicLinkWithOkCheck(doc: Partial<DriveFile> & { id: string }, accessLevel: publicAccessLevel) {
const shareResponse = await this.shareWithPublicLink(doc, accessLevel);
expect(shareResponse.statusCode).toBe(200);
return deserialize<DriveFile>(DriveFile, shareResponse.body);
}

async shareWithPermissions(doc: Partial<DriveFile> & { id: string }, toUserId: string, permissions: DriveFileAccessLevel) {
doc.access_info.entities.push({
type: "user",
id: toUserId,
level: permissions,
grantor: null,
});
console.log(`INFO:: ${doc.access_info}`);
return await this.updateDocument(doc.id, doc);
}

async getPublicLinkAccessToken(doc: Partial<DriveFile>) {
const accessRes = await this.platform.app.inject({
method: "POST",
Expand Down Expand Up @@ -415,6 +457,7 @@ export default class UserApi {
expect(response.statusCode).toBe(200);
const doc = deserialize<DriveItemDetailsMockClass>(DriveItemDetailsMockClass, response.body);
expect(doc.item?.id).toBe(id);
return doc;
};

async sharedWithMeDocuments(
Expand Down
Loading
Loading