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 28 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
145 changes: 145 additions & 0 deletions app/client/ui/DocApiKey.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
import { makeT } from 'app/client/lib/localization';
import { basicButton, textButton } from 'app/client/ui2018/buttons';
import { theme, vars } from 'app/client/ui2018/cssVars';
import { icon } from 'app/client/ui2018/icons';
import { confirmModal } from 'app/client/ui2018/modals';
import { Disposable, dom, IDomArgs, makeTestId, Observable, observable, styled } from 'grainjs';

const t = makeT('DocApiKey');

interface IWidgetOptions {
docApiKey: Observable<string>;
onDelete: () => Promise<void>;
onCreate: () => Promise<void>;
inputArgs?: IDomArgs<HTMLInputElement>;
}

const testId = makeTestId('test-docapikey-');

/**
* DocApiKey component shows an api key with controls to change it. Expects `options.docApiKey` the api
* key and shows it if value is truthy along with a 'Delete' button that triggers the
* `options.onDelete` callback. When `options.docApiKey` is falsy, hides it and show a 'Create' button
* that triggers the `options.onCreate` callback. It is the responsibility of the caller to update
* the `options.docApiKey` to its new value.
*/
export class DocApiKey extends Disposable {
// TODO : user actually logged in, and value if the user is owner of the document.
private _docApiKey: Observable<string>;
private _onDeleteCB: () => Promise<void>;
private _onCreateCB: () => Promise<void>;
private _inputArgs: IDomArgs<HTMLInputElement>;
private _loading = observable(false);
private _isHidden: Observable<boolean> = Observable.create(this, true);

constructor(options: IWidgetOptions) {
super();
this._docApiKey = options.docApiKey;
this._onDeleteCB = options.onDelete;
this._onCreateCB = options.onCreate;
this._inputArgs = options.inputArgs ?? [];
}

public buildDom() {
return dom('div', testId('container'), dom.style('position', 'relative'),
dom.maybe(this._docApiKey, (docApiKey) => dom('div',
cssRow(
cssInput(
{
readonly: true,
value: this._docApiKey.get(),
},
dom.attr('type', (use) => use(this._isHidden) ? 'password' : 'text'),
testId('key'),
{title: t("Click to show")},
dom.on('click', (_ev, el) => {
this._isHidden.set(false);
setTimeout(() => el.select(), 0);
}),
dom.on('blur', (ev) => {
// Hide the key when it is no longer selected.
if (ev.target !== document.activeElement) { this._isHidden.set(true); }
}),
this._inputArgs
),
cssTextBtn(
cssTextBtnIcon('Remove'), t("Remove"),
dom.on('click', () => this._showRemoveKeyModal()),
testId('delete'),
dom.boolAttr('disabled', (use) => use(this._loading)) // or is not owner
),
),
description('This doc API key can be used to access this document via the API. \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Translation missing if I am right (t() should be called)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello,
This are pieces of code written by @CamilleLegeron that I copied from her branch.
I'm not familiar with it.
May be It's a better idea to remove it from this PR as it's not integrated in admin page yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this piece of code is not used yet, we should keep it in a separate branch and reuse it when it will be.

Don’t share this API key.', testId('description')),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rather explain the consequences rather than discouraging its use (I tend to think there could be legitimate reasons to share it). Wdyt?

)),
dom.maybe((use) => !use(this._docApiKey), () => [
basicButton(t("Create"), dom.on('click', () => this._onCreate()), testId('create'),
dom.boolAttr('disabled', this._loading)),
description(t("By generating a doc API key, you will be able to \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking: The \ is not standard for multiline strings (unless it has changed). Probably better to either use template strings or string concatenation.

make API calls for this particular document."), testId('description')),
]),
);
}

// Switch the `_loading` flag to `true` and later, once promise resolves, switch it back to
// `false`.
private async _switchLoadingFlag(promise: Promise<any>) {
this._loading.set(true);
try {
await promise;
} finally {
this._loading.set(false);
}
}

private _onDelete(): Promise<void> {
return this._switchLoadingFlag(this._onDeleteCB());
}

private _onCreate(): Promise<void> {
return this._switchLoadingFlag(this._onCreateCB());
}

private _showRemoveKeyModal(): void {
confirmModal(
t("Remove API Key"), t("Remove"),
() => this._onDelete(),
{
explanation: t(
"You're about to delete a doc API key. This will cause all future requests \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Idem for \

using this doc API key to be rejected. Do you still want to delete?"
),
}
);
}
}

const description = styled('div', `
margin-top: 8px;
color: ${theme.lightText};
font-size: ${vars.mediumFontSize};
`);

const cssInput = styled('input', `
background-color: transparent;
color: ${theme.inputFg};
border: 1px solid ${theme.inputBorder};
padding: 4px;
border-radius: 3px;
outline: none;
flex: 1 0 0;
`);

const cssRow = styled('div', `
display: flex;
`);

const cssTextBtn = styled(textButton, `
text-align: left;
width: 90px;
margin-left: 16px;
`);

const cssTextBtnIcon = styled(icon, `
margin: 0 4px 2px 0;
`);
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).

}
Loading
Loading