Ingest the remaining entities and relationships #5

neurovelho · 2020-09-03T17:02:33Z

This PR includes the remaining of the high value entities and relationships as discussed in the issue thread.

Kindly let us know if something needs to be changed.

neurovelho · 2020-09-03T17:06:00Z

Pinging @aiwilliams @ndowmon @austinkelleher

ndowmon

@neurovelho Thanks for all the work here! All the new entities and relationships look really good.

I peppered some requests throughout for you to review. In particular, I'd like you to take another look at the artifactory_permissions entities and relationships. Let me know if any of my comments don't make sense and we can discuss in more detail.

ndowmon · 2020-09-04T00:25:42Z

src/steps/access.ts

+    id: 'fetch-access-tokens',
+    name: 'Fetch Access Tokens',
+    entities: [entities.ACCESS_TOKEN],
+    relationships: [relationships.ACCOUNT_HAS_ACCESS_TOKEN],


Looks like the particular access token based on your test data could be related to a user via the subject field:

{ "expiry": 1601555179, "issued_at": 1599135979, "issuer": "jfrt@01eg8hb1g2dqb01cdtms0e0bv1", "refreshable": false, "subject": "jfrt@01eg8hb1g2dqb01cdtms0e0bv1/users/[email protected]", "token_id": "ad667cc7-54a5-42b1-bed5-0c92c0fe0fa8", }

I think we should probably add artifactory_user--ASSIGNED-->artifactory_access_token

Looks like the admin can only create what's called admin access token, however it looks like it's actually possible to create regular access tokens that target a specific user using the rest api, so we'll change this.

ndowmon · 2020-09-04T00:34:46Z

src/instanceConfigFields.ts

+  clientPipelineAccessToken: {
+    type: 'string',
+    mask: true,
+  },


Can you update the development docs so devs also know they need to generate this token?

ndowmon · 2020-09-04T00:56:12Z

src/steps/repositories.ts

+function mapPackageTypeToStepEntityMetadata(type: string): StepEntityMetadata {
+  if (type.match(/docker|vagrant/i)) {
+    return entities.ARTIFACT_IMAGE;
+  }
+
+  return entities.ARTIFACT_CODEMODULE;
+}
+


I noticed that this matcher isn't really differentiating images from codemodules. Example from snapshot:

// this is a json but matched the repository name `/docker/i` Object { "_type": "artifactory_artifact_image", "name": "https://codeworkr.jfrog.io/artifactory/docker-repo/hello-world/1.1.1/manifest.json", ... // other properties }, // this appears to be an image Object { "_type": "artifactory_artifact_image", "name": "https://codeworkr.jfrog.io/artifactory/docker-repo/hello-world/1.1.1/sha256__0e03bdcc26d7a9a57ef3b6f1bf1a210cff6239bff7c8cac72435984032851689", ... // other properties },

Not sure if it makes sense to reconsider your regular expression matcher to avoid these types of mis-matchings. I'm personally not 100% sure it is worth differentiating between images and "code modules", considering that code module is basically a catch all for other files, including .pom, .xml, .json, .jpeg, .gz and others which can be found in the snapshot. If others have a strong opinion about differentiating artifactory_artifact_image vs artifactory_artifact_codemodule, please speak up. I'd particularly like to hear @erichs' opinions here considering his comment #3 (comment)

Yeah I'm not too excited about this matcher either, I think it would be much better if we could just roll with one type.

Spoke with @erichs about this. It really stinks that the API appears to only reliably return folder and uri properties, no additiona metadata:

{ "folder": false, "uri": "https://codeworkr.jfrog.io/artifactory/docker-repo/hello-world/1.1.1/sha256__bf756fb1ae65adf866bd8c456593cd24beb6a0a061dedf42b26a993176745f6b", }

Since there are no other properties we can rely on, it doesn't seem like a great idea for us to add any custom matcher in our code (following the philosophy that we at J1 only ingest the state of a target system, and don't build additional logic).

As long as you are pulling the projectType property on artifactory_repository entities, we can create J1QL like the following, which is good enough for now:

// look for maven builds find artifactory_repository that has artifactory_artifact where artifactory_repository.packageType = 'Maven'

ndowmon · 2020-09-04T01:00:39Z

src/steps/index.test.ts

+    await fetchRepositories(context);
+    await fetchPipelineSources(context);
+    await fetchArtifacts(context);
+    await fetchBuilds(context);


Looks like you're not testing fetchPermissions here. Is that on purpose or just overlooked?

Looks like we missed it, good catch.

ndowmon · 2020-09-04T01:07:22Z

src/constants.ts

+    _class: RelationshipClass.ALLOWS,
+    sourceType: entities.PERMISSION._type,
+    targetType: entities.REPOSITORY._type,
+  },


Is PERMISSION_DENIES_REPOSITORY a valid relationship type? Just noticed that there are both PREMISSION_ALLOWS_BUILD and PERMISSION_DENIES_BUILD.

Actually, read comments in /permissions.ts. Not sure there should be any DENIES relationships.

ndowmon · 2020-09-04T02:11:36Z

src/steps/permissions.ts

+    },
+  });
+
+  await jobState.addEntity(permissionEntity);


Looks like this code is adding two artifactory_permission entities for every permission. There should be just one artifactory_permission entity created. That single entity should have relationships to both users and groups.

We were going by this comment here: #3 (comment), but seeing as the response contains both users and groups, it would make more sense if we had both users and groups in the entity.

Just to clarify - you mean both users and groups in related to the entity, correct?

Yes, that's right.

ndowmon · 2020-09-04T02:17:46Z

src/steps/permissions.ts

+  );
+}
+
+function constructPermissionsMap(


Per artifactory docs, there can also be release bundle permissions, destination permissions, and pipeline permissions, PLUS "global permissions". Can you pull those additional permission targets in this function? I realize that some of those can't be used to target existing entities, but can you add code comments to implement others when they are implemented in the future?

The api documentation seems to indicate that only repository, build and releaseBundle are available, though it could be that it's outdated 🤔

We'll definitely see if it's possible to add the rest.

ndowmon · 2020-09-04T02:26:01Z

src/steps/permissions.ts

+  return targetPermissionsMap;
+}
+
+async function createPermissionBuildAllowsRelationships(


Jfrog docs discussing include and exclude patterns seem to indicate that both the statements are used to pattern match permission targets that are allowed by a given permission. I don't think the exclude pattern is actually a DENIES statement in reality.

I would instead suggest creating only ALLOWS statements for permissions targets that do match the include-pattern and do not match the exclude-pattern.

Also, I notice that artifactory exposes a pattern matching API - it would be better to use a matcher provided by the vendor than a nodejs utility. See https://www.jfrog.com/confluence/display/JFROG/Artifactory+REST+API

Usage: GET /api/search/pattern?pattern=repo-key:this/is/a/*pattern*.war

ndowmon · 2020-09-04T02:28:11Z

src/types.ts

+      };
+      groups?: {
+        [name: string]: string[];
+      };


I haven't used the API, but it seems to me that permission.repo also has the include-patterns and exclude-patterns properties.

ndowmon · 2020-09-04T02:31:37Z

src/types.ts

    };
+    'include-patterns': string[];
+    'exclude-patterns': string[];


You should enumerate all of the permission target types, including destinations, pipeline-sources, release-bundles, and possibly global if it exists.

neurovelho · 2020-09-09T15:09:23Z

Alrighty, we've implemented the changes you requested @ndowmon.

There's however a few complications though:

We implemented the user -> assigned -> access token relationship, however it looks like no matter what you do in the dashboard or API, the access tokens simply won't show up in the api/token response -- only the admin tokens do. Sadly this seems like a regular occurrence with this API.
After trying out multiple things it seems like API only returns repository, build and releaseBundle permissions:

Release bundle permission is difficult/impossible to implement because they aren’t available during the trial.

Pipeline permissions can’t seem to be fetched even after we create them in the permission using JFrog admin dashboard, the API simply isn’t returning those for some reason.

Destination permissions - we aren’t exactly sure what these represent, we don’t see additional field(s) for setting this on the permission setting page nor does the API returns anything about them.
The pattern matching endpoint can only be used for artifacts, nothing else. That means that when we're matching include/exclude patterns for repositories and builds we pretty much have to use the external library for matching.

ndowmon

Thanks for making the requested changes! This integration definitely looks ready for deployment to me.

I commented a few small requests but we can easily open new github issues for those and fix in the future.

ndowmon · 2020-09-09T16:13:43Z

src/steps/permissions.ts

    },
+    (buildEntity) => `${permissionEntity._key}|allows|${buildEntity._key}`,


Not a big deal since it's functionally correct, but here it should be permissionEntity.

Did you mean repositoryEntity? In any case, I renamed it since it was bothering me too much 🙂.

Yes! Thanks.

ndowmon · 2020-09-09T16:17:43Z

src/steps/permissions.ts

@@ -237,9 +272,11 @@ export const permissionsSteps: IntegrationStep<IntegrationConfig>[] = [
      relationships.PERMISSION_ASSIGNED_GROUP,
      relationships.PERMISSION_ALLOWS_REPOSITORY,
      relationships.PERMISSION_ALLOWS_BUILD,
+      relationships.PERMISSION_ALLOWS_REPOSITORY_GROUP,


Cool, thanks for adding this!

ndowmon · 2020-09-09T16:19:16Z

src/steps/index.test.ts

@@ -51,6 +52,7 @@ describe('JFrog Arrifactory', () => {
    await fetchPipelineSources(context);
    await fetchArtifacts(context);
    await fetchBuilds(context);
+    await fetchPermissions(context);


Thanks for adding. Not necessary now, but let's add a GH issue to add an expect statement for permissions here.

ndowmon

Thanks @neurovelho. This is a great integration!

Ingest the remaining entities and relationships

a71a8c8

ndowmon self-requested a review September 3, 2020 18:00

ndowmon suggested changes Sep 4, 2020

View reviewed changes

creativice added 3 commits September 9, 2020 14:25

Implement requested changes

d5048d8

Refactor user/group permission relationship generation

aa95f0a

Turn ANY, ANY LOCAL, ANY repositories to repository groups

4b5515d

ndowmon previously approved these changes Sep 9, 2020

View reviewed changes

Rename buildEntity -> repositoryEntity in arrow function

1330c05

neurovelho dismissed ndowmon’s stale review via 1330c05 September 10, 2020 12:31

ndowmon self-requested a review September 10, 2020 14:21

ndowmon approved these changes Sep 10, 2020

View reviewed changes

ndowmon merged commit 61872b8 into JupiterOne-Archives:master Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingest the remaining entities and relationships #5

Ingest the remaining entities and relationships #5

neurovelho commented Sep 3, 2020

neurovelho commented Sep 3, 2020

ndowmon left a comment

ndowmon Sep 4, 2020

neurovelho Sep 4, 2020

ndowmon Sep 4, 2020

ndowmon Sep 4, 2020 •

edited

Loading

neurovelho Sep 4, 2020

ndowmon Sep 4, 2020

ndowmon Sep 4, 2020

neurovelho Sep 4, 2020

ndowmon Sep 4, 2020

ndowmon Sep 4, 2020

ndowmon Sep 4, 2020

neurovelho Sep 4, 2020

ndowmon Sep 4, 2020

neurovelho Sep 7, 2020

ndowmon Sep 4, 2020

neurovelho Sep 4, 2020

ndowmon Sep 4, 2020

ndowmon Sep 4, 2020

ndowmon Sep 4, 2020

neurovelho commented Sep 9, 2020

ndowmon left a comment

ndowmon Sep 9, 2020

neurovelho Sep 10, 2020

ndowmon Sep 10, 2020

ndowmon Sep 9, 2020

ndowmon Sep 9, 2020

ndowmon left a comment

		},
		(buildEntity) => `${permissionEntity._key}\|allows\|${buildEntity._key}`,

Ingest the remaining entities and relationships #5

Ingest the remaining entities and relationships #5

Conversation

neurovelho commented Sep 3, 2020

neurovelho commented Sep 3, 2020

ndowmon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndowmon Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neurovelho commented Sep 9, 2020

ndowmon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndowmon left a comment

Choose a reason for hiding this comment

ndowmon Sep 4, 2020 •

edited

Loading