Use DocApiKey as valid ApiKeys #1115
Conversation
|
I still have some tests to write before it can be a full PR. @paulfitz I'm a little worried about my implementation in the Authorizer. |
app/gen-server/lib/HomeDBManager.ts
Outdated
| return query ? key : query; | ||
| } | ||
|
|
||
| // in parameters linkId is the linkId in db in case of update of this id in the share |
There was a problem hiding this comment.
I took some time to understand this comment, what do you think of this instead:
| // in parameters linkId is the linkId in db in case of update of this id in the share | |
| // the linkId parameter corresponds to the one currently stored in the db in case we want to change it. |
There was a problem hiding this comment.
In fact any parameter can be updated, not just linkId.
If we want to update linkId it have to be set in share object passed as a parameter
share.linkId = newLinkId
So I go with
// 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
Let me know
TODO: tests
TODO fix GET /api/docs/{did}/apikey/{linkId}i\n returns 200 instead of 403 when not owning document
TODO fix GET /api/docs/{did}/apikey/{linkId}i\n returns 200 instead of 403 when not owning document
Missing spaces
app/client/ui/DocApiKey.ts
Outdated
| 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. \ |
There was a problem hiding this comment.
Translation missing if I am right (t() should be called)
There was a problem hiding this comment.
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.
If this piece of code is not used yet, we should keep it in a separate branch and reuse it when it will be.
app/client/ui/DocApiKey.ts
Outdated
| ), | ||
| ), | ||
| 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.
Maybe rather explain the consequences rather than discouraging its use (I tend to think there could be legitimate reasons to share it). Wdyt?
app/client/ui/DocApiKey.ts
Outdated
| 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.
Nitpicking: The \ is not standard for multiline strings (unless it has changed). Probably better to either use template strings or string concatenation.
app/client/ui/DocApiKey.ts
Outdated
| () => this._onDelete(), | ||
| { | ||
| explanation: t( | ||
| "You're about to delete a doc API key. This will cause all future requests \ |
| 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.
Nitpicking:
| if (!doc){ | |
| if (!doc) { |
|
|
||
| const linkId = req.body.linkId; | ||
| const query = await this._dbManager.getDocApiKeyByLinkId(did, linkId); | ||
| if (query){ |
There was a problem hiding this comment.
Nitpicking:
| 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*
There was a problem hiding this comment.
prettier or riot. :p
There was a problem hiding this comment.
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.
Maybe configuring eslint this way:
...
"space-after-keywords": "error",
"space-before-blocks": "error",
| const options = sanitizeDocApiKeyOptions(req.body.options); | ||
| req.body.options = JSON.stringify(options); | ||
|
|
||
| const key = await this._dbManager.createDocApiKey(did, req.body); |
There was a problem hiding this comment.
I prefer avoiding altering the parameters, req.body is meant to store the information passed by the user.
| 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) | |
| }); |
| // 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); |
There was a problem hiding this comment.
| // 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) | |
| }); |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| if (req.body.docId){ | ||
| throw new ApiError("Can't update DocId", 400); |
There was a problem hiding this comment.
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 (!legalAccessValues.includes(options.access)){ | ||
| throw new ApiError(`Invalid access value: ${options.access}`, 400); | ||
| } | ||
| return {...options, "apikey": true}; |
There was a problem hiding this comment.
You may use type-interface-checker, maybe with StringUnion and StringUnion.checkAll(array).
| const key = makeId(); | ||
| const query = await this._connection.createQueryBuilder() | ||
| .insert() | ||
| .setParameter('options', share.options) |
app/server/lib/Authorizer.ts
Outdated
| // anonymous user's profile via the api (e.g. how should the api key be managed). | ||
| return res.status(401).send('Credentials cannot be presented for the anonymous user account via API key'); | ||
| // Bearer needs to be form "DOC-YYYYYYYYY" to apply as Doc Api key | ||
| if (parts[1].match(/^DOC-.*/)) { |
There was a problem hiding this comment.
Nit (improves a bit the performance, especially interesting for requests middlewares):
| if (parts[1].match(/^DOC-.*/)) { | |
| const docApiPrefix = 'DOC-'; | |
| if (parts[1]?.startsWith('DOC-')) { |
And later:
// Doc Api Key
const docApiKey = parts[1].substring(docApiPrefix.length)This way, if by chance the docApi accepts hyphens, there would be no regressions here.
| const didRegex = /(?<=^\/docs\/).+?(?=\/|$)/g; | ||
| const did = url.match(didRegex); | ||
| // Only API scope matching regex MUST be accessed with Doc Api keys | ||
| if (!did || !did[0]){ | ||
| return res.status(401).send(`Access Denied: Scope limited to Documents`); | ||
| } |
There was a problem hiding this comment.
Nit: A bit simpler and probably more optimized, wdyt?
NB: if you agree on that proposal, don't forget to replace all occurrences of did[0] to did below.
| const didRegex = /(?<=^\/docs\/).+?(?=\/|$)/g; | |
| const did = url.match(didRegex); | |
| // Only API scope matching regex MUST be accessed with Doc Api keys | |
| if (!did || !did[0]){ | |
| return res.status(401).send(`Access Denied: Scope limited to Documents`); | |
| } | |
| const didRegex = /\/docs\/(?<did>[^\/]*)/; | |
| const matchRes = url.match(didRegex); | |
| // Only API scope matching regex MUST be accessed with Doc Api keys | |
| const did = matches?.groups.did; | |
| if (!did){ | |
| return res.status(401).send(`Access Denied: Scope limited to Documents`); | |
| } |
| const access = share.options.access ? share.options.access : null; | ||
| mreq.docAuth = {...docAuth, access}; | ||
| mreq.userIsAuthorized = true; | ||
| hasApiKey = true; |
There was a problem hiding this comment.
I share here a proposal about the express routes which would be maybe too ambitious for this PR (let's do that in a next one).
I feel like we can split the DocApi.ts modules in different ones, each one with its specific route (docs/:did, docs/:did/tables, docs/:did/tables/:tid/columns, docs/:did/tables/:tid/records, ...).
The immediate benefit would be to have probably better separation of concerns and lighter modules.
Also, and for our problem here, I feel like this piece of middleware can be delayed and put as a middleware in the /docs/:did route.
Some adaptations has to be found, especially hasApiKey is used for telemetry (I guess it can be moved in a separate function?), and I don't know if there are some caveats with having the user set to anon first before changing again.
|
@paulfitz after a first review with @fflorent It appears that the main doubt about the actual implementation is the following. In I've done this to have a user whom we can apply share.access rights. Getting a user by the org owner I'm expecting that the user still exists and have rights to the doc. But that's maybe false The two other options that have come in our mind after the review are :
Thanks for your feedbacks. |
|
Deployed commit |
Before getting into review of this implementation, or reading the implementation more, I'd like a clear description of what it is supposed to do. I sat down recently to understand it, only to realize some time later that the UI code I was scratching my head over is not in fact used. I have read the linked issue. I can imagine many things this feature might be. In slack, I've been pointed to parts of the code, and given curl commands to try it out. What I haven't had yet is a clear description of what it does (not how). The description of what the feature does will help in the following ways:
Plenty of Grist work has skimpy descriptions, so I'm sorry to single this PR out. The issue it links just doesn't have any clear decisions in it or product conversation, which sometimes compensates for weak descriptions. |
|
Deployed commit |
Hello @paulfitz , We work with @fflorent on a specification to help to have an overview on the feature proposal. |
DocApiKeyFeature AbstractAs a workflow builder, I want to interact through API calls with a single document, with limited rights, in a third party software (like zappier, n8n, ...), without exposing other documents/ressources I have rights on, so I reduce the potential damages in case the API key is leaked or is used for dangerous calls by mistake, and so I can revoke them quickly and easily. MotivationsWe witness that some users are tempted to share their user api Key with others ones in order to implement some automation. User storiesAs a user, I would like to update my document using n8n. To do so, I open the API console of the document, I generate a new DocApiKey with n8n-key as name and choose beetween As a document owner, I list Doc Api keys availaible for a document, so I know how these keys are used. As a document owner I edit Doc Api Keys names/Descriptions. As a document owner I revoke Doc Api Keys, so it cannot be used any longer. As an administrator, I look for REST Api calls using Doc Api Key in the logs, so I know how they are used and can revoke them if necessary. Proposed solution
The functionality delivery is scheduled in two steps:
Proposed implementationDB side (current option in PR draft)
Example of share adapted to docApiKey needs
The only difference is an DB side (alternative option)Add a new table for this DocApiKeys.
EndpointsNew endpoints will be added to CRUD this docApiKeys CREATE a DocApiKey
path Parameters
Request Body schema: application/json
Response200
READ a DocApiKey
path Parameters
Response200
UPDATE a DocApiKey
path Parameters
Request Body schema: application/jsonResponse200
DELETE a DocApiKey
Response200 path Parameters
List all keys of a document
path Parameters
Response200
DELETE all keys of a document
path Parameters
Response200 Nota beneIn order to avoid to guarantee unicity beetween shares.key and user.apikey DocAPIKey and to be sure to discriminate these two keys in API calls, which would be complex to do, Authorization bearer will be formed as :
The logs need to be customized so we can clearly see which key have been used whose (and whose if necessary), and what docApiKey. Helpful when concerns are raised about possible intrusions or api key thief. To be debated
Implementation questionsThe Many options come to my mind to address this point :
And then, of course, limit rights to Can you please tell me which is the best option in your opinion? |
|
@hexaltation thanks for the proposal write-up. Some questions:
Some possible answers off the top of my head (but I have no special authority on design, once we've got a clear proposal we can circulate it):
|
|
Hello @paulfitz, Here are some answers to your previous message.
Ideally the implementation must be modular enough to be compatible in the future with WYSIWYS (What You See Is What You Share) plans. |
Can you tell me more about this new special user? For example, how are the properties of this user determined (see https://support.getgrist.com/access-rules/#access-rule-conditions for a list).
Ok, this simplifies things technically, pending details of the special user associated with the key.
Exposing
Yes, I'd like that, so that there is a proposal I can show to Anais and Dmitry once it is sufficiently clear. It is getting closer, thanks for your clarifications! |
I our mind the default access of this user are editors one.
Granular access rules can be set to a
Yes, It's our goal. |
|
@hexaltation can you tell me more about the following properties of the special user:
I just want to be sure I know what a special user is exactly. |
|
We plan to use the already existing method
The user id is set dynamically by
If you have better suggestions for name and email, I'll be glad to take it :). |
Ah, so these are concrete, real, user accounts (rows of the (I think the exact email address you propose wouldn't be great for gristlabs.com but an alternative with a different domain that was clearly a black hole would be easy) Edit: oh if it is literally the single string Do the users get cleaned up when the associated key gets removed? Edit: I'm now wondering if I misinterpreted everything and this is one single user account for the full installation? |
When we discussed about the feature with @fflorent we were more on a unique special user for all DocApiKeys. In my first try of implementation it was through setting some But I wasn't sure it was the good way to implement it. And what is the proper/best way to do it was as a feature and technically was (may be still is 😓 ) my initial question at the top of this PR. May be stay with the |
|
Doc API keys would be a really valuable feature. It’s worth doing it well. I am glad it’s going through a real proposal + review process. I decided to chime in here with some wishes. Wishes:
My thoughts on some questions listed earlier:
I am also interested in questions that have already been asked:
|
|
@dsagal Thank you for your answers. We agree on everything you propose. It seems that your answers leads us to implement OAuth2 for these service keys. At least :
So we must drop the idea of using the
In our mind a key is attached to a document, Other owners can decide what is the future of the key when a previous owner loose priviledges.
So any owner can revoke the key. |
I don't think these points are required by my wishes, and seem both more difficult to implement and to use. But I may be missing something.
For under the hood, it seems to me that if a key is limited to its creator's privileges, as Paul suggested, then it could be a mechanism very similar to existing API keys; and if a key can exist separately from its owner, then the
To give owners a chance to decide this, we should check for keys and ask what to do with them whenever any user's access is removed or reduced, not only on the document level, but also for a workspace or org (since access can be inherited). Would it be a good idea, then, to require the user making the access changes to take responsibility for these keys? So that each key remains associated with a current owner of the document? |
|
As a new implementation have been designed in #1217 I close this MR |
Resolves #579
DocApiKey are service Api Keys binded to a document rather than to a user.
Under the hood they are using Shares implementation.
To differentiate them for "classical" Shares they have a flag
ApiKeyset to true in sharesOptions.Their
keycan be used to make Api Calls a given document, as well as be used in url like "classical" Shares.They have to be used like this: