-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
INT-5913 - Enhance GitLab integration to query access levels of users and groups #82
base: main
Are you sure you want to change the base?
INT-5913 - Enhance GitLab integration to query access levels of users and groups #82
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still working on understanding src/steps/projects.ts
. I'll probably add one more comment later.
src/provider/GitlabClient.ts
Outdated
} catch (err) { | ||
projectMembers = []; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we ignore errors from this endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same idea as the one mentioned below.
src/provider/GitlabClient.ts
Outdated
`/groups/${groupId}/members/all`, | ||
); | ||
} catch (err) { | ||
if (err.status === 403) groupMembers = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we ignore errors from this endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)!
src/steps/projects.ts
Outdated
to: projectEntity, | ||
}), | ||
|
||
console.log('sharedWithGroups', sharedWithGroups); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that!
dbf3d1d
to
391c4a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should switch to a Set
to make lookups easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😢
// Already processed | ||
return; | ||
} | ||
processedPairs.set(group.id, [...(groupProjects || []), project.id]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processedPairs.set(group.id, [...(groupProjects || []), project.id]); | |
if (Array.isArray(groupProjects)) { | |
groupProjects.push(project.id) | |
} else { | |
processedParis.set(group.id, [project.id]) | |
} |
@@ -30,21 +33,100 @@ export async function fetchProjects({ | |||
return projectEntity; | |||
}; | |||
|
|||
// Maps groupId to projectIds | |||
const processedPairs: Map<number, number[]> = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
}), | ||
); | ||
// Remove it from array as it's already processed | ||
sharedWithGroups.splice(originGroupIndex, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should switch to Sets.
} | ||
|
||
// Now we can safely iterate through remainder of the shared groups and make connections | ||
for (const sharedGroup of sharedWithGroups) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Added
groupId
,groupAccessLevel
,groupName
,groupFullPath
andexpiresOn
on thegitlab_group
-HAS->gitlab_project
relationships.
userAccessLevel
on thegitlab_project
-HAS->gitlab_user
relationships.userAccessLevel
on thegitlab_group
-HAS->gitlab_user
relationships.