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

Use DocApiKey as valid ApiKeys #1115

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
637d3c9
Rebase migration over existing 42
CamilleLegeron Mar 21, 2024
b75e567
create documentSettings api key ui part
CamilleLegeron Mar 28, 2024
554cf1d
WIP
hexaltation Jun 27, 2024
2ff7e67
wip
hexaltation Jun 27, 2024
cfc2c96
Remove uneeded migration
hexaltation Jul 1, 2024
14f97f5
fix: indent
hexaltation Jul 1, 2024
850363c
chore: remove unused key
hexaltation Jul 1, 2024
e4880d4
WIP post apikey
hexaltation Jul 1, 2024
e97f75b
WIP: endpoints
hexaltation Jul 2, 2024
94386cd
wip doc api key
hexaltation Jul 3, 2024
b597a3d
wip doc api key
hexaltation Jul 3, 2024
5a631cb
wip tests
hexaltation Jul 4, 2024
0f425f4
WIP TODO test ADDED
hexaltation Jul 5, 2024
8c0c822
tests POST WIP
hexaltation Jul 8, 2024
c4eaa4a
POST done
hexaltation Jul 8, 2024
8849d0e
Endpoint Tests Donish
hexaltation Jul 9, 2024
50e58db
Endpoint Tests Donish
hexaltation Jul 9, 2024
64abd51
Chore: Endpoints Doc Api Key DONE
hexaltation Jul 10, 2024
798234f
Chore: Endpoints Doc Api Key DONE
hexaltation Jul 10, 2024
d7d4c74
fix: repect ShareOptions.access authorized values in type declaration
hexaltation Jul 11, 2024
9e779f9
feat: docApikey as ApiKey WIP
hexaltation Jul 11, 2024
b5e2fec
fix: typos
hexaltation Jul 11, 2024
a713ec4
feat: test DocApiKey as ApiKey 1/5
hexaltation Jul 15, 2024
1c6c734
feat: wip docApiKey
hexaltation Jul 18, 2024
3cda9e8
feat: DocApiKey as valid ApiKey
hexaltation Jul 22, 2024
41b89ff
fix: Remove from frontend
hexaltation Jul 22, 2024
e5e049e
fix: Remove from frontend
hexaltation Jul 22, 2024
766a328
fix: fflorent review adjustements
hexaltation Jul 22, 2024
4cf4b2c
fix: broken ApiKeyAccess tests
hexaltation Jul 23, 2024
3ab5f95
Typo fix
hexaltation Jul 23, 2024
ea05508
feat: REMOVE unused code
hexaltation Aug 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/common/ShareOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@ export interface ShareOptions {
// Half-baked, just here to exercise an aspect of homedb
// syncing.
access?: 'editors' | 'viewers';

apikey?: true;
}
138 changes: 138 additions & 0 deletions app/gen-server/ApiServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {addPermit, clearSessionCacheIfNeeded, getDocScope, getScope, integerPara
isParameterOn, optStringParam, sendOkReply, sendReply, stringParam} from 'app/server/lib/requestUtils';
import {IWidgetRepository} from 'app/server/lib/WidgetRepository';
import {getCookieDomain} from 'app/server/lib/gristSessions';
import { ShareOptions } from 'app/common/ShareOptions';

// exposed for testing purposes
export const Deps = {
Expand Down Expand Up @@ -324,6 +325,118 @@ export class ApiServer {
return sendOkReply(req, res);
}));

// POST /api/docs/:did/apiKey
this._app.post('/api/docs/:did/apiKey', expressWrap(async (req, res) => {
const did = req.params.did;
const doc = await this._dbManager.getDoc(req);
if (!doc){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking:

Suggested change
if (!doc){
if (!doc) {

throw new ApiError(`No such doc: ${did}`, 404);
}

const linkId = req.body.linkId;
const query = await this._dbManager.getDocApiKeyByLinkId(did, linkId);
if (query){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking:

Suggested change
if (query){
if (query) {

Same for below.


In vim: :%s/if (.*)\zs{/ {/g should fix the problem (not tested).

For \zs, it starts the match after this piece of pattern, it see :help \zs:

\zs	Matches at any position, but not inside [], and sets the start of the
	match there: The next char is the first char of the whole match.
	|/zero-width|
	Example: >
		/^\s*\zsif
<	matches an "if" at the start of a line, ignoring white space.
	Can be used multiple times, the last one encountered in a matching
	branch is used.  Example: >
		/\(.\{-}\zsFab\)\{3}
<	Finds the third occurrence of "Fab".
	This cannot be followed by a multi. *E888*

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

prettier or riot. :p

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eslint has some rules I think for that stuff, but still, I agree with a tool that would prevent us from making style-related comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe configuring eslint this way:

...
"space-after-keywords": "error",
"space-before-blocks": "error",

throw new ApiError("LinkId must be unique", 400);
}
const options = sanitizeDocApiKeyOptions(req.body.options);
req.body.options = JSON.stringify(options);

const key = await this._dbManager.createDocApiKey(did, req.body);
Comment on lines +341 to +344
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer avoiding altering the parameters, req.body is meant to store the information passed by the user.

Suggested change
const options = sanitizeDocApiKeyOptions(req.body.options);
req.body.options = JSON.stringify(options);
const key = await this._dbManager.createDocApiKey(did, req.body);
const options = sanitizeDocApiKeyOptions(req.body.options);
const key = await this._dbManager.createDocApiKey(did, {
...req.body,
options: JSON.stringify(options)
});


return sendOkReply(req, res, key);
}));

// GET /api/docs/:did/apiKey/:linkId
this._app.get('/api/docs/:did/apiKey/:linkId', expressWrap(async (req, res) => {
const did = req.params.did;

const doc = await this._dbManager.getDoc(req);
if (!doc){
throw new ApiError(`No such doc: ${did}`, 404);
}
const linkId = req.params.linkId;
const query = await this._dbManager.getDocApiKeyByLinkId(did, linkId);
return query ? sendOkReply(req, res, query) : res.status(404).send();
}));

// PATCH /api/docs/:did/apiKey/:linkId
this._app.patch('/api/docs/:did/apiKey/:linkId', expressWrap(async (req, res) => {
const did = req.params.did;
const linkId = req.params.linkId;

const doc = await this._dbManager.getDoc(req);
if (!doc){
throw new ApiError(`No such doc: ${did}`, 404);
}

if (req.body.docId){
throw new ApiError("Can't update DocId", 400);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may control and throw what the user passes using the convenient type-interface-checker util:
https://github.com/gristlabs/ts-interface-checker

I can explain you how to do that after lunch.

}

if (req.body.key){
throw new ApiError("Can't update key", 400);
}

const queryLinkId = await this._dbManager.getDocApiKeyByLinkId(did, req.body.linkId);
if (queryLinkId){
throw new ApiError("LinkId must be unique", 400);
}

// In order to catch {options: ""} case
if (Object.keys(req.body).includes("options")){
const options = sanitizeDocApiKeyOptions(req.body.options);
req.body.options = JSON.stringify(options);
}

const query = await this._dbManager.updateDocApiKeyByLinkId(did, linkId, req.body);
Comment on lines +385 to +391
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// In order to catch {options: ""} case
if (Object.keys(req.body).includes("options")){
const options = sanitizeDocApiKeyOptions(req.body.options);
req.body.options = JSON.stringify(options);
}
const query = await this._dbManager.updateDocApiKeyByLinkId(did, linkId, req.body);
// In order to catch {options: ""} case
const options = Object.hasOwnProperty(req.body, 'options') ? // Or you may use req.body.options simply
sanitizeDocApiKeyOptions(req.body.options) : undefined; // Or null depending on what the function expects
const query = await this._dbManager.updateDocApiKeyByLinkId(did, linkId, {
...req.body,
options: JSON.stringify(options)
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also I wonder if we could not adapt or use inheritance for ShareInfo to allow options to be a typed object. This way:

  • we would keep this the parameters strongly typed;
  • less JSON operations, which would improve the readability and a bit the performance.

return sendOkReply(req, res, query);
}));

// DELETE /api/docs/:did/apiKey/:linkId
this._app.delete('/api/docs/:did/apiKey/:linkId', expressWrap(async (req, res) => {
const did = req.params.did;
const linkId = req.params.linkId;

const doc = await this._dbManager.getDoc(req);
if (!doc){
throw new ApiError(`No such doc: ${did}`, 404);
}

const linkId4Did = await this._dbManager.getDocApiKeyByLinkId(did, linkId);
if (!linkId4Did){
throw new ApiError(`Invalid LinkId: ${linkId}`, 404);
}

const query = await this._dbManager.deleteDocApiKeyByLinkId(did, linkId);
return sendOkReply(req, res, query);
}));

// GET /api/docs/:did/apiKeys
this._app.get('/api/docs/:did/apiKeys', expressWrap(async (req, res) => {
const did = req.params.did;

const doc = await this._dbManager.getDoc(req);
if (!doc){
throw new ApiError(`No such doc: ${did}`, 404);
}

const query = await this._dbManager.getDocApiKeys(did);
return sendOkReply(req, res, query);
}));

// DELETE /api/docs/:did/apiKeys
this._app.delete('/api/docs/:did/apiKeys', expressWrap(async (req, res) => {
const did = req.params.did;

const doc = await this._dbManager.getDoc(req);
if (!doc){
throw new ApiError(`No such doc: ${did}`, 404);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are many copy-paste for this part. For that case (here duplication does not help much understanding what is done), what do you think of factorizing?

async function assertDocExistsAndIsAccessible(did: string) {
  const doc = await this._dbManager.getDoc(req);
  if (!doc){
    throw new ApiError(`No such doc: ${did}`, 404);
  }
}

Usage:

await assertDocExistsAndIsAccessible(req);


const query = await this._dbManager.deleteDocApiKeys(did);
return sendOkReply(req, res, query);
}));

// PATCH /api/orgs/:oid/access
// Update the specified org acl rules.
this._app.patch('/api/orgs/:oid/access', expressWrap(async (req, res) => {
Expand Down Expand Up @@ -699,3 +812,28 @@ async function updateApiKeyWithRetry(manager: EntityManager, user: User): Promis
}
throw new Error('Could not generate a valid api key.');
}

function sanitizeDocApiKeyOptions(rawOptions: string): ShareOptions {
const legalOptions: (keyof ShareOptions)[] = ["access"];
const legalAccessValues: ShareOptions["access"][] = ["editors", "viewers"];

if (!rawOptions){
throw new ApiError("Missing body params: options", 400);
}

const options = JSON.parse(rawOptions);
if (!options.access){
throw new ApiError("Missing options: access", 400);
}

Object.keys(options).forEach(element => {
if (!(legalOptions as string[]).includes(element)){
throw new ApiError(`Invalid option: ${element}`, 400);
}
});

if (!legalAccessValues.includes(options.access)){
throw new ApiError(`Invalid access value: ${options.access}`, 400);
}
return {...options, "apikey": true};
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may use type-interface-checker, maybe with StringUnion and StringUnion.checkAll(array).

}
96 changes: 54 additions & 42 deletions app/gen-server/lib/homedb/HomeDBManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
AvailableUsers, GetUserOptions, NonGuestGroup, Resource, UserProfileChange
} from 'app/gen-server/lib/homedb/Interfaces';
import {SUPPORT_EMAIL, UsersManager} from 'app/gen-server/lib/homedb/UsersManager';
import {SharesManager} from 'app/gen-server/lib/homedb/SharesManager';
import {Permissions} from 'app/gen-server/lib/Permissions';
import {scrubUserFromOrg} from "app/gen-server/lib/scrubUserFromOrg";
import {applyPatch} from 'app/gen-server/lib/TypeORMPatches';
Expand Down Expand Up @@ -247,6 +248,7 @@ export type BillingOptions = Partial<Pick<BillingAccount,
*/
export class HomeDBManager extends EventEmitter {
private _usersManager = new UsersManager(this, this._runInTransaction.bind(this));
private _sharesManager = new SharesManager(this);
private _connection: Connection;
private _dbType: DatabaseType;
private _exampleWorkspaceId: number;
Expand Down Expand Up @@ -968,6 +970,17 @@ export class HomeDBManager extends EventEmitter {
return doc;
}

public async getOrgOwnerFromDocApiKey(docApiKey: Share) {
return this._connection.createQueryBuilder()
.select('orgs')
.from(Organization, 'orgs')
.leftJoin(Workspace, 'workspaces', 'orgs.id = workspaces.org_id')
.leftJoin(Document, 'docs', 'workspaces.id = docs.workspace_id')
.leftJoin(Share, 'shares', 'docs.id = shares.doc_id')
.where('shares.key = :key', {key: docApiKey.key})
.getOne();
}

// Calls getDocImpl() and returns the Document from that, caching a fresh DocAuthResult along
// the way. Note that we only cache the access level, not Document itself.
public async getDoc(reqOrScope: Request | Scope, transaction?: EntityManager): Promise<Document> {
Expand Down Expand Up @@ -2497,6 +2510,44 @@ export class HomeDBManager extends EventEmitter {
.getOne() || undefined;
}

public getDocApiKeyByLinkId(docId: string, linkId: string): Promise<Share | null> {
return this._sharesManager.getShareByLinkId(docId, linkId);
}

public getDocApiKeyByKey(key: string): Promise<Share | null> {
return this._sharesManager.getShareByKey(key);
}

public async createDocApiKey(docId: string, share: ShareInfo) {
return this._sharesManager.createDocApiKey(docId, share);
}

// nb. The linkId parameter corresponds to the one currently stored in the db.
// If we want to update it the new value must be passed through the share parameter
public async updateDocApiKeyByLinkId(docId: string, linkId: string, share: ShareInfo) {
return this._sharesManager.updateDocApiKeyByLinkId(docId, linkId, share);
}

public async updateDocApiKeyByKey(docId: string, apiKey: string, share: ShareInfo) {
return this._sharesManager.updateDocApiKeyByKey(docId, apiKey, share);
}

public async deleteDocApiKeyByKey(docId: string, apiKey: string) {
return this._sharesManager.deleteDocApiKeyByKey(docId, apiKey);
}

public async deleteDocApiKeyByLinkId(docId: string, linkId: string) {
return this._sharesManager.deleteDocApiKeyByLinkId(docId, linkId);
}

public async getDocApiKeys(docId: string): Promise<Share[] | undefined> {
return this._sharesManager.getDocApiKeys(docId);
}

public async deleteDocApiKeys(docId: string) {
return this._sharesManager.deleteDocApiKeys(docId);
}

public getAnonymousUser() {
return this._usersManager.getAnonymousUser();
}
Expand Down Expand Up @@ -2717,54 +2768,15 @@ export class HomeDBManager extends EventEmitter {
}

public async syncShares(docId: string, shares: ShareInfo[]) {
return this._connection.transaction(async manager => {
for (const share of shares) {
const key = makeId();
await manager.createQueryBuilder()
.insert()
// if urlId has been used before, update it
.onConflict(`(doc_id, link_id) DO UPDATE SET options = :options`)
.setParameter('options', share.options)
.into(Share)
.values({
linkId: share.linkId,
docId,
options: JSON.parse(share.options),
key,
})
.execute();
}
const dbShares = await manager.createQueryBuilder()
.select('shares')
.from(Share, 'shares')
.where('doc_id = :docId', {docId})
.getMany();
const activeLinkIds = new Set(shares.map(share => share.linkId));
const oldShares = dbShares.filter(share => !activeLinkIds.has(share.linkId));
if (oldShares.length > 0) {
await manager.createQueryBuilder()
.delete()
.from('shares')
.whereInIds(oldShares.map(share => share.id))
.execute();
}
});
return this._sharesManager.syncShares(docId, shares);
}

public async getShareByKey(key: string) {
return this._connection.createQueryBuilder()
.select('shares')
.from(Share, 'shares')
.where('shares.key = :key', {key})
.getOne();
return this._sharesManager.getShareByKey(key);
}

public async getShareByLinkId(docId: string, linkId: string) {
return this._connection.createQueryBuilder()
.select('shares')
.from(Share, 'shares')
.where('shares.doc_id = :docId and shares.link_id = :linkId', {docId, linkId})
.getOne();
return this._sharesManager.getShareByLinkId(docId, linkId);
}

private async _getOrgMembers(org: string|number|Organization) {
Expand Down
Loading
Loading