feat(job_token_scope): support job token access allowlist API #2767

nejch · 2024-01-19T11:26:54Z

Changes

Closes #2762.
Adds https://docs.gitlab.com/ee/api/project_job_token_scopes.html#get-a-projects-cicd-job-token-inbound-allowlist

Documentation and testing

Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated:

Documentation in the matching docs section
Unit tests and/or functional tests

nejch · 2024-01-19T11:27:48Z

docs/gl_objects/job_token_scope.rst

+
+Get a projec's CI/CD job token inbound allowlist::
+
+    allowlist = scope.allowlist.list()


Not sure yet about this naming here, but it follows both the endpoint URL and our convention 😕

yeah it would definetly be clearer if it were scope.projects_allowlist since gitlab have added groups_allowlist https://docs.gitlab.com/ee/api/project_job_token_scopes.html#get-a-projects-cicd-job-token-allowlist-of-groups

But scope.allowlist does absolutely match the API design doc so it is probably best overall to be consistent with their docs 🤷

nejch · 2024-01-19T11:29:36Z

gitlab/v4/objects/job_token_scope.py

+        try:
+            return cast(int, getattr(self, self._id_attr))
+        except AttributeError:
+            return cast(int, getattr(self, "id"))


GitLab at it again - create response:

{ "source_project_id": 1, "target_project_id": 2 }

list response:

{ "id": 4, "description": null, "name": "Diaspora Client", "name_with_namespace": "Diaspora / Diaspora Client", "path": "diaspora-client", ... }

I'm kind of thinking for the create to not return anything. As the caller appears to use all the values when creating that will be returned. Not sure if that would simplify things or not.

Convention in REST and so far in python-gitlab would be to return the object that has been created. Means that in future if Gitlab add optional things into the contract which get returned back in the POST, user's would get insight into that

codecov · 2024-01-19T11:44:54Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (72e1aa7) 88.25% compared to head (7b0b22d) 96.42%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2767      +/-   ##
==========================================
+ Coverage   88.25%   96.42%   +8.17%     
==========================================
  Files          90       90              
  Lines        5866     5880      +14     
==========================================
+ Hits         5177     5670     +493     
+ Misses        689      210     -479

Flag	Coverage Δ
api_func_v4	`82.29% <71.42%> (?)`
cli_func_v4	`83.55% <71.42%> (?)`
unit	`88.21% <71.42%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
gitlab/v4/objects/job_token_scope.py	`85.18% <71.42%> (-14.82%)`	⬇️

... and 46 files with indirect coverage changes

Sjord · 2024-01-30T11:31:09Z

This currently assumes a job token scope as the parent of the allowlist, but I don't think that is correct. I would expect the allowlist to work without retrieving the job token scope first:

# Works, retrieves job_token_scope:
print(project.job_token_scope.get().allowlist.list())

# Doesn't work without job_token_scope:
print(project.job_token_scope.allowlist.list())

Test could look something like this:

"""
GitLab API: https://docs.gitlab.com/ee/api/project_job_token_scopes.html#get-a-projects-cicd-job-token-inbound-allowlist
"""

import pytest
import responses
from gitlab.v4.objects.job_token_scope import AllowlistedProjectManager

allowlist_small = {"source_project_id":1,"target_project_id":2}

allowlist_large = {
    "id": 2,
    "description": None,
    "name": "testproject",
    "name_with_namespace": "testgroup / testproject",
    "path": "testproject",
    "path_with_namespace": "testgroup/testproject",
    "created_at": "2024-01-21T19:18:38.829Z",
    "default_branch": "main",
    "tag_list": [],
    "topics": [],
    "ssh_url_to_repo": "git@localhost:testgroup/testproject.git",
    "http_url_to_repo": "http://localhost/testgroup/testproject.git",
    "web_url": "http://localhost/testgroup/testproject",
    "readme_url": "http://localhost/testgroup/testproject/-/blob/main/README.md",
    "forks_count": 0,
    "avatar_url": None,
    "star_count": 0,
    "last_activity_at": "2024-01-23T18:22:16.321Z",
    "namespace": {
        "id": 3,
        "name": "testgroup",
        "path": "testgroup",
        "kind": "group",
        "full_path": "testgroup",
        "parent_id": None,
        "avatar_url": None,
        "web_url": "http://localhost/groups/testgroup",
    },
}


@pytest.fixture
def resp_get_allowlist():
    with responses.RequestsMock(assert_all_requests_are_fired=False) as rsps:
        rsps.add(
            method=responses.GET,
            url="http://localhost/api/v4/projects/1/job_token_scope/allowlist",
            json=[allowlist_large],
            content_type="application/json",
            status=200,
        )
        yield rsps


@pytest.fixture
def resp_post_allowlist():
    with responses.RequestsMock(assert_all_requests_are_fired=False) as rsps:
        rsps.add(
            method=responses.POST,
            url="http://localhost/api/v4/projects/1/job_token_scope/allowlist",
            json=allowlist_small,
            content_type="application/json",
            status=201,
        )
        yield rsps


@pytest.fixture
def allowlist(project):
    return project.job_token_scope.allowlist

def test_get_allowlist(allowlist, resp_get_allowlist):
    projects = allowlist.list()
    assert projects[0].get_id() == 2

def test_create_allowlist(allowlist, resp_post_allowlist):
    resp = allowlist.create({"target_project_id": 2})
    assert resp.get_id() == 2

nejch · 2024-02-02T21:33:51Z

@Sjord very true, I would at least want to add support for lazy=True for GetWithoutIDMixin first so that this wouldn't require 2 API calls.

The reason I did it this way was I wanted to follow GitLab's REST API path segment hierarchy. But lately it's making less and less sense to me, so we could also flatten this potentially - I'm not sure, also part of why this is still in draft 😅 thanks for the feedback!

TimKnight-DWP · 2024-02-07T12:43:10Z

@nejch FYI if you haven't seen:

Gitlab have just merged an update to GraphQL endpoint to add support for adding groups to the CI_JOB_TOKEN allowlist: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143132

And hopefully will be adding the same support to the REST API: https://gitlab.com/gitlab-org/gitlab/-/issues/435903#note_1761348177

POST /projects/:id/job_token_scope/allowlist

nejch · 2024-02-07T12:56:19Z

@nejch FYI if you haven't seen:

Gitlab have just merged an update to GraphQL endpoint to add support for adding groups to the CI_JOB_TOKEN allowlist: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143132

And hopefully will be adding the same support to the REST API: https://gitlab.com/gitlab-org/gitlab/-/issues/435903#note_1761348177

POST /projects/:id/job_token_scope/allowlist

Thanks @TimKnight-DWP! 🙇 I'm pretty sure they will, because their terraform provider relies on REST. I'll get back to this PR this week.

TimKnight-DWP · 2024-02-07T13:46:04Z

Cool - let me know if there's anything I can do to help with this MR @nejch , it's something that's high on our agenda, adding this here and then using from gitlabform to manage our state

I'll take a look through Gitlabs MR and see how they intend to handle "Groups" whether it's taking group id (consistent with how projects are added) or something else. And feedback here.

TimKnight-DWP · 2024-02-07T14:02:13Z

Design from Gitlab if the pattern is followed from GraphQl, either a new or the existing allowlist endpoint will use

source_project_id, target_group_id

POST I suspect they will add target_group_id as a supported parameter in the body of the request, in GQL they created a new mutation AddGroup

DELETE probably takes group id into the
DELETE /projects/:id/job_token_scope/allowlist/:target_group_id

TimKnight-DWP · 2024-02-13T16:18:18Z

@nejch - given I've had my head in the integration tests recently 😄 , I've raised a PR into here for adding integration tests for allowlist: #2797

TimKnight-DWP · 2024-03-04T09:33:03Z

Group Allowlist REST API -> https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145069

nejch · 2024-05-17T10:03:48Z

@Sjord very true, I would at least want to add support for lazy=True for GetWithoutIDMixin first so that this wouldn't require 2 API calls.

The reason I did it this way was I wanted to follow GitLab's REST API path segment hierarchy. But lately it's making less and less sense to me, so we could also flatten this potentially - I'm not sure, also part of why this is still in draft 😅 thanks for the feedback!

Sorry @Sjord this was already done in another PR as is, but I think we could look into adding a lazy get at least, so we don't require another API call to use the endpoint.

nejch commented Jan 19, 2024

View reviewed changes

feat(job_token_scope): support job token access allowlist API

7b0b22d

nejch force-pushed the feat/job-token-scope-allowlist branch from 0b3e199 to 7b0b22d Compare January 19, 2024 11:30

TimKnight-DWP mentioned this pull request Feb 14, 2024

test: add api integration test for ci_cd_token allowlist #2797

Closed

This was referenced Mar 4, 2024

Manage CI/CD job token scope allowlist gitlabform/gitlabform#571

Closed

feat(job_token_scope): support Groups in job token allowlist API #2816

Merged

nejch closed this May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(job_token_scope): support job token access allowlist API #2767

feat(job_token_scope): support job token access allowlist API #2767

nejch commented Jan 19, 2024

nejch Jan 19, 2024

TimKnight-DWP Mar 5, 2024

nejch Jan 19, 2024

JohnVillalovos Jan 19, 2024

TimKnight-DWP Feb 13, 2024

codecov bot commented Jan 19, 2024

Sjord commented Jan 30, 2024

nejch commented Feb 2, 2024

TimKnight-DWP commented Feb 7, 2024

nejch commented Feb 7, 2024

TimKnight-DWP commented Feb 7, 2024

TimKnight-DWP commented Feb 7, 2024

TimKnight-DWP commented Feb 13, 2024

TimKnight-DWP commented Mar 4, 2024

nejch commented May 17, 2024


		Get a projec's CI/CD job token inbound allowlist::

		allowlist = scope.allowlist.list()

feat(job_token_scope): support job token access allowlist API #2767

feat(job_token_scope): support job token access allowlist API #2767

Conversation

nejch commented Jan 19, 2024

Changes

Documentation and testing

nejch Jan 19, 2024

Choose a reason for hiding this comment

TimKnight-DWP Mar 5, 2024

Choose a reason for hiding this comment

nejch Jan 19, 2024

Choose a reason for hiding this comment

JohnVillalovos Jan 19, 2024

Choose a reason for hiding this comment

TimKnight-DWP Feb 13, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2024

Codecov Report

Sjord commented Jan 30, 2024

nejch commented Feb 2, 2024

TimKnight-DWP commented Feb 7, 2024

nejch commented Feb 7, 2024

TimKnight-DWP commented Feb 7, 2024

TimKnight-DWP commented Feb 7, 2024

TimKnight-DWP commented Feb 13, 2024

TimKnight-DWP commented Mar 4, 2024

nejch commented May 17, 2024