Update SDK Connections model to consume context and filter by readAccess #2074
Conversation
mknowlton89
changed the base branch from
main
to
mk/no-access-data-source-model
…ated with a project the user doesn't have access to.
…s associated with a project the user doesn't have access to." This reverts commit d4eccba.
…ated with a project the user doesn't have access to.
mknowlton89
changed the title
| {proj.name} | ||
| </span> | ||
| ); | ||
| <td className="d-flex align-items-center"> |
There was a problem hiding this comment.
Updated this to utilize the ProjectBadges component to increase continuity.
| @@ -1489,14 +1490,17 @@ export async function postApiKeyReveal( | |||
| } | |||
|
|
|||
| export async function getWebhooks(req: AuthRequest, res: Response) { | |||
| const { org } = getContextFromReq(req); | |||
| const context = getContextFromReq(req); | |||
| const webhooks = await WebhookModel.find({ | |||
There was a problem hiding this comment.
I've created a follow up ticket to refactor the Webhook model so we're not exporting the model.
|
Your preview environment pr-2074-bttf has been deployed. Preview environment endpoints are available at: |
| connection: SDKConnectionInterface, | ||
| useCloudProxy: boolean = false | ||
| ) { | ||
| if (!connectionSupportsProxyUpdate(connection, useCloudProxy)) return; | ||
|
|
||
| const job = agenda.create(PROXY_UPDATE_JOB_NAME, { | ||
| context, |
There was a problem hiding this comment.
Do we want to add the context to the message body? I know we avoided this for other jobs we've refactored by passing in the org ID instead, and letting the job create its own context. Just wanted to make sure.
There was a problem hiding this comment.
The more I think about this, the less it seems like a good idea. It seems like our context object shouldn't be persisted anywhere nor leave the scope of our express webserver. It can contain sensitive information (users' emails), or be potentially huge (e.g. the upcoming data models collection from Jeremy's PR).
Putting the context inside of a message body sent to distributed services makes our context object more sensitive to changes i.e. we have to be more careful with what we put in there. Right now, I think we just want it to be a toolbox for all things needed within the lifetime of a request within our express server without worrying about concerns like size or security. Our previous approach with embedding the only org ID seems more robust with this in mind.
There was a problem hiding this comment.
That's a great call. I've updated this job so it no longer takes in the context object, and instead just takes in the orgId, and builds a FULL_ACCESS_CONTEXT object on the fly.
…gId, and building the context object on the fly, rather than passing in the context object.
…ess (#2074) * Updates SDK Connections model to consume context and add no access filtering
Features and Changes
This PR updates the
SDKConnectionModelto ensure we're filtering SDK connections based on the user'sreadAccesslevel.SDK Connections have taken on a few different forms over the years - they've been legacy SDK endpoints, and now, SDK Connections. Given that, when testing, we need to look at both the newer
SDK Connectionsas well as the legacy SDK endpoints, and also SDK WebhooksTesting