App/edfial 388 store and display API client info#18
App/edfial 388 store and display API client info#18edandylytics merged 9 commits intodevelopmentfrom
Conversation
| } | ||
|
|
||
| const TEST_AUDIENCE = process.env.EXTERNAL_API_TOKEN_AUDIENCE ?? 'test-audience'; | ||
| const TEST_AUDIENCE = process.env.OAUTH2_AUDIENCE ?? 'test-audience'; |
There was a problem hiding this comment.
Missed this when updating the env var in an earlier PR, but the default value matched the .env.test value so it didn't impact how the tests ran.
|
Speaking in response to your description, without reading much of the code yet, here's my opinion on the tech debt and scoping: I suspect that the token user attribution columns (and also earthbeam ones?) should be automatically filled more or less the way app/session user ones are, but I'm not sure and it depends on the longer-term vision for what features become accessible to the token users. If it never gets beyond a couple endpoints touching a couple tables, anything much fancier than this would be silly. So I'm very happy to start small — explicitly providing those values to Prisma — and perhaps it never goes beyond that. I do see both the Footnotes |
bhaugeea
left a comment
bhaugeea
left a comment
There was a problem hiding this comment.
Overall 100% on board with the simple approach taken here, and there's just one minor bug that shouldn't be too bad to fix. On the side we discussed longer-term vision for handling human/earthbeam/m2m user context in the request pipeline, business logic, and database. It's something that may deserve refactors at some point (general-purpose prisma that handles all of them and injects appropriate context into the db, db triggers that use that context, addition of attribution columns to more tables, universal audit logging, etc.) but there's no reason to jump the gun on it now. This is the right solution for the present moment.
| /** Used internally to compute isApiInitiated - not included in serialized output */ | ||
| @Expose() | ||
| @Exclude({ toPlainOnly: true }) | ||
| apiClientId: string | null; | ||
|
|
||
| @Expose() | ||
| get isApiInitiated(): boolean { | ||
| return !!this.apiClientId; | ||
| } |
There was a problem hiding this comment.
This doesn't quite seem to work. isApiInitiated is indeed serialized to a JSON property, but then when the class is instantiated in the front end, it becomes a getter again (which always returns false). It's okay when the apiClientName is present because of how the conditions in the display table's ternary are ordered.
|
|
||
| it('should return 403 if ANY required scope is missing', async () => { | ||
| const token = await signExternalApiToken({ scope }); // has create:jobs but not read:jobs | ||
| const token = await signExternalApiToken({ ...tokenPayload, scope: 'read:jobs' }); // has create:jobs but not read:jobs |
There was a problem hiding this comment.
I don't think the scope needs to be overridden here (and the way it's overridden is kind of opposite the existing comment), because what's different about this test is what's required, not what's granted.
There was a problem hiding this comment.
🤦 you're 100% right. I must have been tab-accepting too fast when moving around how the token is handled. Thanks for catching that
2 participants
This PR pulls client info from the access token on external API requests and saves them with the job. We also display the client name in the frontend, in lieu of a user name, for jobs created by the API.
A few design/scope considerations:
createdByuser via the request-scoped Prisma client but the equivalent field for the API client we set directly and use the anonymous Prisma client (which is why we pass the Prisma client to the services). I'm not thrilled with this, but feel it's fine. It gets a by more awkward if we were to set these on the run, too, since we have to pass both the prisma client and the api client info through several fn calls... but probably also fine for this PR. I'm letting this simmer for the moment and don't have strong feelings yet on what, if anything, to do to address this (input welcome!)client_idclaim overazpbut accept either.client_idis what's in RFC 9068. Auth0 sendsazpif using the Auth0 profile. For Auth0, at least, the same value is used to identify the client, whichever claim is used.