JupiterOne / sdk

Home of the JupiterOne SDK
Mozilla Public License 2.0
20 stars 16 forks source link

Add `requiredPermissions` property to execution context #438

Open ndowmon opened 3 years ago

ndowmon commented 3 years ago

We should be continuously driving towards improving our dependency execution graph. We have some integrations that require multiple permissions to execute, and we should add these requirements to step metadata in order to appropriately handle these missing permissions in a consistent way across integrations.

const step = {
  id: 'step-id',
  name: 'step-name',
  entities: [],
  relationships: [],
  requiredPermissions: [permissions.bucketRead],
  executionHandler: stepHandler,
}

This change might also introduce state mutation in the validateInvocation method, or possibly in a new optional getPermissions hook that runs after validateInvocation but before step execution.

It would also allow us to create richer documentation for our jupiterone.md docs.

mknoedel commented 3 years ago

This makes a lot of sense, what are your thoughts on having it be a validateStepInvocation hook or something instead? I'm not sure how many integrations use scopes or permissions but every integration can have pre-step validation if desired. Either way, both will go a long way for making errors more clear in the job log I would imagine.

ndowmon commented 3 years ago

If it were validateStepInvocation, I guess I would expect it to run just before a single integration step. Is that what you mean?

Something like:

const step = {
  id: 'step-id',
  name: 'step-name',
  entities: [],
  relationships: [],
  validateStepInvocation: async (context) => {
    const client = new ApiClient(context.instance.config);
    const clientPermissions = await client.getPermissions();
    return clientPermissions.includes(permissions.bucketRead);
  },
  executionHandler: stepHandler,
}

That would work well, I think. A great way to keep the step atomic.

aiwilliams commented 3 years ago

I think this would be very helpful for docs generation and where possible, permissions checks ahead of executing a step. I think we'll need to continue to maintain and advance communication in the pattern where we don't know until an error response occurs in a request for resources.

Side related note: I think we should have a way to create a client once and re-use it throughout the execution. The validateInvocation function is sometimes used for that and I dislike it very much, because it's so side-effecty, violating the single responsibility principal.

ndowmon commented 3 years ago

Maybe the term enabled is better for this function:

const step = {
  id: 'step-id',
  name: 'step-name',
  entities: [],
  relationships: [],
  enabled: async (context) => {
    const client = new ApiClient(context.instance.config);
    const clientPermissions = await client.getPermissions();
    return clientPermissions.includes(permissions.bucketRead);
  },
  executionHandler: stepHandler,
}
ndowmon commented 3 years ago

But this method above doesn't allow for self-documentation about permissions, sadly.

mknoedel commented 3 years ago

You know, thinking about it again, we should be able to pull out scopes from the access tokens we are using to make api calls so long as the integrations are using JWTs (which perhaps most are?). Below is an example from MS365 project once you decode the JWT (decoded using https://jwt.io) image

I think due to the documentation benefits of having a permissions or scopes key on the step declarations, that likely makes more sense.