-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add OIDC authentication customizer for GitLab #1052
base: main
Are you sure you want to change the base?
Conversation
Tagging for awareness: |
@nscuro Would you mind doing a preliminary review and letting me know if this implementation/direction needs adjustment? Thanks! |
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.
Looks good so far. I think concurrency control of the task will become interesting.
The workflow orchestration functionality we're currently working on will provide the ability to serialize based on a concurrency group, similar to what GitHub Actions has. So this undertaking will become easier in the future, but until then we need another solution.
src/main/java/org/dependencytrack/integrations/gitlab/GitLabAuthenticationCustomizer.java
Outdated
Show resolved
Hide resolved
|
||
import static org.dependencytrack.model.ConfigPropertyConstants.GITLAB_ENABLED; | ||
|
||
public class GitLabSyncTask implements LoggableSubscriber { |
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 presume we need a way to serialize execution of these tasks? If two or more users with overlapping GitLab project accesses login in close succession, we will encounter race conditions.
Before we go into the how (there are multiple options), can you think of any property/properties of the OIDC profile that can act as event key? i.e. what would you use as concurrency group?
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.
Nothing in the OIDC profile comes to mind. The only thing I can think of is maybe the groups
claim, if they can be locked individually, but that doesn't give us full GitLab project paths.
The name of each GitLab project as it is read from the GitLab API response can be used as well, but that isn't available in the profile
@nscuro We noticed that changes in the alpine schema aren't yet reflected in hyades or the apiserver, so we created a changeset that seems to work. The changeset is now pushed to this PR. Do we need to open a PR in hyades to update schema.sql or anything? /cc @pkwiatkowski1 |
Leave it for now. We will add that migration in a separate PR anyway, since it involves a bit more non-SQL migration logic. You can keep the migration in your PR so you can test, but will likely need to rebase later down the road. |
@nscuro Just wanted to see if you had any ideas on a couple of bugs we're seeing. It looks like the event is somehow firing off twice, and getting a weird error about classes not being found in the CLASSPATH. For example:
I'll continue debugging, just seeing if anything stands out as immediately obvious to you. Thanks! |
@jhoward-lm For the redundant invokation, see my comment here: stevespringett/Alpine#755 (comment)
Admittedly not sure, I'd have to checkout the code and see if I can reproduce it. |
@nscuro Here is the full stack trace if it helps Full error output
|
@nscuro In |
Adding
So either there's a bug in the ORM, or perhaps you have some changes locally that are not included in this PR? |
for (String group : toCreate) { | ||
LOGGER.debug("Creating project " + group); | ||
|
||
Project existingProject = qm.getProject(group, null); |
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 can't find any place where the qm
being used here is set. The code should currently throw a NPE. Is there some code you haven't yet pushed?
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.
Yes I have some local code that sets the qm
and some other fixes. We think the use of UnblockedEvent
might have something to do with the Classifier
not being found. I'll push what I have shortly
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.
Is the change here acceptable?
fa505bb
to
6cad00c
Compare
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Patrick Kwiatkowski <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Patrick Kwiatkowski <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Allen Shearin <[email protected]>
Signed-off-by: Allen Shearin <[email protected]>
Signed-off-by: Ephraim Mensah <[email protected]>
Signed-off-by: Ephraim Mensah <[email protected]>
Signed-off-by: Ephraim Mensah <[email protected]>
Signed-off-by: Ephraim Mensah <[email protected]>
Signed-off-by: Ephraim Mensah <[email protected]>
Signed-off-by: Ephraim Mensah <[email protected]>
Signed-off-by: Allen Shearin <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Allen Shearin <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Allen Shearin <[email protected]>
Signed-off-by: Allen Shearin <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
d6ecd8c
to
f952260
Compare
…erver into gitlab-integration
Signed-off-by: Philippe <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
29f5a81
to
cc785c8
Compare
Signed-off-by: Jonathan Howard <[email protected]>
Description
This PR adds an implementation of the
OidcAuthenticationCustomizer
service provider interface specific to GitLab. It is meant to synchronize a user's projects and roles/max access levels per project within GitLab to a Hyades instance.Depends on stevespringett/Alpine#755.
Note
WIP: a rough to-do list with outstanding items is in a comment block in
GitLabSyncTask.java
Addressed Issue
Closes DependencyTrack/hyades#1632
Additional Details
This PR is a request for early review while still in development in order to change direction if necessary.
Checklist
This PR fixes a defect, and I have provided tests to verify that the fix is effective