INT-5913 - Enhance GitLab integration to query access levels of users and groups #82

eXtremeX · 2022-11-21T18:54:20Z

Added

Added the following properties: groupId, groupAccessLevel, groupName,
groupFullPath and expiresOn on the gitlab_group -HAS-> gitlab_project
relationships.
Added the following property: userAccessLevel on the gitlab_project -HAS->
gitlab_user relationships.
Added the following property: userAccessLevel on the gitlab_group -HAS->
gitlab_user relationships.

zemberdotnet

I'm still working on understanding src/steps/projects.ts. I'll probably add one more comment later.

zemberdotnet · 2022-11-29T20:36:23Z

src/provider/GitlabClient.ts

+    } catch (err) {
+      projectMembers = [];
+    }


Why do we ignore errors from this endpoint?

Same idea as the one mentioned below.

zemberdotnet · 2022-11-29T20:36:26Z

src/provider/GitlabClient.ts

+        `/groups/${groupId}/members/all`,
+      );
+    } catch (err) {
+      if (err.status === 403) groupMembers = [];


Why do we ignore errors from this endpoint?

Hey @zemberdotnet, it's for handling a corner case where the the account has viewing access to a group or project but its authentication level is not enough to access members.The API will return an error 403 which is a valid response but we don't want it to cause the entire step to fail, but instead skip generating user entities for those particular groups/projects because there's no access to that information. With the account we're using it's happening for some groups, but not all - example.

In more detail, take the code inside fetchUsers for example (https://github.com/Creativice-Oy/graph-gitlab/blob/feature/users-groups-accesses/src/steps/users.ts#L26).

The function is iterating through different groups and attempts to fetch each group's members. Sometimes it can legitimately fail in doing so:

"code": "PROVIDER_AUTHORIZATION_ERROR", "fatal": false, "endpoint": "https://gitlab.com/api/v4/groups/1809245/members/all?page=1&per_page=100", "status": 403, "statusText": "Forbidden"

If that happens with how the client code was previously setup, the function will fail and the step as a whole too. We simply wanted to ignore those members and fetch what we can.

There's probably multiple ways in which this could be tackled 🤔

One thing we could add to this current idea is the following:

} catch (err) { if (err.status === 403) { projectMembers = []; this.logger.warn( { projectId }, 'Could not fetch members for project', ); } }

Another idea is to handle this entirely different but there is this problem described above that we should think about while doing so!

Okay, thank you for the detailed explanation. Adding a warn log on our side it probably a good first step. We probably also need a non-spam way to alert customers that they may not be getting all the data, but that's a separate issue.

@zemberdotnet The warn logs have been added and the PR has been re-based against main. Let me know if you want us to tackle that separate issue in this PR or in additional one (if this one is otherwise merge ready)!

zemberdotnet · 2022-11-29T20:38:49Z

src/steps/projects.ts

-            to: projectEntity,
-          }),
+
+        console.log('sharedWithGroups', sharedWithGroups);


We should remove this.

Sorry about that!

zemberdotnet

@eXtremeX I'm a little confused about the work being done in the projects.ts file. I think we can make a few improvements, but an explanation of the work being done in the step would be great.

zemberdotnet · 2023-01-17T21:58:26Z

src/steps/projects.ts

+        processedPairs.set(group.id, [...(groupProjects || []), project.id]);
+
+        // Check if the group used for finding this project exists in the sharedWithGroups section
+        const originGroupIndex = sharedWithGroups.findIndex(


We should switch to a Set to make lookups easier.

Hmm did you mean Set (set of Ids)? or Set?

Option 1
Set<Number> allows for super easy lookups, but later we pull some properties that'd be missing in this case:

const originGroupIndex = sharedWithGroups.findIndex( (sharedGroup) => sharedGroup.group_id === group.id, ); if (originGroupIndex !== -1) { const sharedGroup = sharedWithGroups[originGroupIndex]; const groupAccessLevel = getAccessLevel( sharedGroup.group_access_level, );

Set would lose them..

Option 2
Set<GitLabGroupRef> would make above possible but we're searching for shared group based on the one we're iterating through (they're not the same object types), so unless we create a new (temporary, that might be missing some data) object that looks like GitLabGroupRef based off of project, we can't compare the two.

Hope this makes sense, it's a bit confusing unfortunately 😢

zemberdotnet · 2023-01-18T16:08:12Z

src/steps/projects.ts

+          // Already processed
+          return;
+        }
+        processedPairs.set(group.id, [...(groupProjects || []), project.id]);


Suggested change

processedPairs.set(group.id, [...(groupProjects || []), project.id]);

if (Array.isArray(groupProjects)) {

groupProjects.push(project.id)

} else {

processedParis.set(group.id, [project.id])

}

zemberdotnet · 2023-01-18T16:11:33Z

src/steps/projects.ts

@@ -30,21 +33,100 @@ export async function fetchProjects({
    return projectEntity;
  };

+  // Maps groupId to projectIds
+  const processedPairs: Map<number, number[]> = new Map();


I suspect a Map<number, Set> may be easier. We would not need to do any kind of findIndex work below and can just directly .has().

Can you explain what the new work being done in this step is? I do not understand the logic as I'm not very familiar with the integration or GitLab.

zemberdotnet · 2023-01-18T16:12:23Z

src/steps/projects.ts

+            }),
+          );
+          // Remove it from array as it's already processed
+          sharedWithGroups.splice(originGroupIndex, 1);


We should switch to Sets.

zemberdotnet · 2023-01-18T16:13:10Z

src/steps/projects.ts

+        }
+
+        // Now we can safely iterate through remainder of the shared groups and make connections
+        for (const sharedGroup of sharedWithGroups) {


Echoing my above comment, I don't understand why we had to do the work above to make this safe. Can you explain the logic for this step.

eXtremeX requested a review from a team as a code owner November 21, 2022 18:54

zemberdotnet reviewed Nov 29, 2022

View reviewed changes

zemberdotnet self-assigned this Dec 1, 2022

eXtremeX requested a review from zemberdotnet December 6, 2022 13:52

llravelo and others added 3 commits December 13, 2022 10:34

Add access level properties on the relationships

c893bc9

Remove console.log

0eb2090

Add warn logging for project/group members

391c4a5

eXtremeX force-pushed the feature/users-groups-accesses branch from dbf3d1d to 391c4a5 Compare December 13, 2022 09:35

Run prettier on codeql-analysis.yml

caa5a18

zemberdotnet reviewed Jan 18, 2023

View reviewed changes

Add comments to project.ts

842af37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INT-5913 - Enhance GitLab integration to query access levels of users and groups #82

INT-5913 - Enhance GitLab integration to query access levels of users and groups #82

eXtremeX commented Nov 21, 2022

zemberdotnet left a comment

zemberdotnet Nov 29, 2022

eXtremeX Dec 6, 2022

zemberdotnet Nov 29, 2022

eXtremeX Dec 6, 2022

zemberdotnet Dec 7, 2022

eXtremeX Dec 13, 2022

zemberdotnet Nov 29, 2022

eXtremeX Dec 6, 2022

zemberdotnet left a comment

zemberdotnet Jan 17, 2023

eXtremeX Mar 13, 2023

zemberdotnet Jan 18, 2023

zemberdotnet Jan 18, 2023

zemberdotnet Jan 18, 2023

zemberdotnet Jan 18, 2023

-        processedPairs.set(group.id, [...(groupProjects || []), project.id]);
+        if (Array.isArray(groupProjects)) {
+            groupProjects.push(project.id)
+        } else {
+           processedParis.set(group.id, [project.id])
+        }

INT-5913 - Enhance GitLab integration to query access levels of users and groups #82

Are you sure you want to change the base?

INT-5913 - Enhance GitLab integration to query access levels of users and groups #82

Conversation

eXtremeX commented Nov 21, 2022

Added

zemberdotnet 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zemberdotnet 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

Choose a reason for hiding this comment