-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Changes from 28 commits
637d3c9
b75e567
554cf1d
2ff7e67
cfc2c96
14f97f5
850363c
e4880d4
e97f75b
94386cd
b597a3d
5a631cb
0f425f4
8c0c822
c4eaa4a
8849d0e
50e58db
64abd51
798234f
d7d4c74
9e779f9
b5e2fec
a713ec4
1c6c734
3cda9e8
41b89ff
e5e049e
766a328
4cf4b2c
3ab5f95
ea05508
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. \ | ||
Don’t share this API key.', testId('description')), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicking: The |
||
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 \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
`); |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 = { | ||||||||||||||||||||||||||||||||
|
@@ -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){ | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicking:
Suggested change
|
||||||||||||||||||||||||||||||||
throw new ApiError(`No such doc: ${did}`, 404); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const linkId = req.body.linkId; | ||||||||||||||||||||||||||||||||
const query = await this._dbManager.getDocApiKeyByLinkId(did, linkId); | ||||||||||||||||||||||||||||||||
if (query){ | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicking:
Suggested change
Same for below. In vim: For
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prettier or riot. :p There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe configuring eslint this way:
|
||||||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer avoiding altering the parameters,
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may control and throw what the user passes using the convenient 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I wonder if we could not adapt or use inheritance for
|
||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||||||||||||||||||||||||||||||||
|
@@ -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}; | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may use |
||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.