Autoland should respect the repository's configured ldap group for landing permissions.

ASSIGNED
Assigned to

Status

MozReview
General
P1
normal
ASSIGNED
a year ago
10 months ago

People

(Reporter: smacleod, Assigned: zalun)

Tracking

Details

(URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Currently only scm_level_3 is used. We should support other groups, allowing for more specific permissions.
(Reporter)

Updated

a year ago
Assignee: nobody → pzalewa
Status: NEW → ASSIGNED
Priority: -- → P1
(Reporter)

Comment 1

a year ago
For a little more background, the MozReview extension's hosting service that we use for all of the repositories has a field for specifying the ldap group required to land to the repository. The problem is the API resource for triggering these landings just has a hardcoded scm_level_3 check. To fix this we should remove the hardcoded check and make sure we're using the configured group.

Before putting this live though it's probably important to make sure all the current repositories have scm_level_3 set in the hosting service, and that any new repositories will default to it. Then we can adjust down for specific repos.
Comment hidden (mozreview-request)

Updated

11 months ago
Component: General → General
Product: Conduit → MozReview

Comment 3

11 months ago
mozreview-review
Comment on attachment 8849101 [details]
mozreview: respect the repository's configured ldap group for landing permissions (bug 1345927)

https://reviewboard.mozilla.org/r/121934/#review124244

This is an initial review for style and obvious problems.  I'll do a second pass soon to look for more in-depth issues.

::: pylib/mozreview/mozreview/decorators.py:53
(Diff revision 1)
> +            scm_level = force_group or DEFAULT_SCM_LEVEL
> +            if not force_group:
> +                # Try to get the SCM group specified in Repository.extra_data
> +                rr_id = request.POST.get('review_request_id', None)
> +                if rr_id and rr_id[0]:
> +                    rr = ReviewRequest.objects.get(pk=rr_id[0])
> +                    scm_level = rr.repository.extra_data.get(
> +                        'required_ldap_group', DEFAULT_SCM_LEVEL)

I think this logic is tricky to follow as it is written.  I think `scm_level` is trying to get set to some new value, and it's OK if the checks in the `if` statement on line 57 fail and no new value is set?

Is there some way to make all of the possible outcomes from this block of code explicit, to increase readability?  Maybe an `else` clause?

::: pylib/mozreview/mozreview/decorators.py:55
(Diff revision 1)
>          @webapi_response_errors(PERMISSION_DENIED)
>          def _check_groups(*args, **kwargs):
>              request = _find_httprequest(args)
> +            scm_level = force_group or DEFAULT_SCM_LEVEL
> +            if not force_group:
> +                # Try to get the SCM group specified in Repository.extra_data

I'd extract this into it's own function to increase the readability and ease of testing, and ease of inspection of both the branch code and the original function.

::: pylib/mozreview/mozreview/hooks.py:129
(Diff revision 1)
>          # which match the criteria so we can indicate which reviews
>          # actually gave the permission to land.
> -        if author_mrp.has_scm_ldap_group('scm_level_3'):
> +        scm_level = review_request.repository.extra_data.get(
> +            'required_ldap_group', 'scm_level_3')
> +        if author_mrp.has_scm_ldap_group(scm_level):
>              # In the case of a level 3 user we really only care that they've

This comment needs to be updated to remove the reference to L3, since the code now applies to any approved level.

::: pylib/mozreview/mozreview/review_helpers.py:52
(Diff revision 1)
>      """Return whether the review request has received a current L3 ship it.
>  
>      A boolean will be returned indicating if the review request has received
>      a ship-it from an L3 user that is still valid. In order to be valid the
>      ship-it must have been provided after the latest diff has been uploaded.

Docstring needs updating: it still implies L3-only, but the method applies to all levels.

::: pylib/mozreview/mozreview/tests/test-autoland-post-job.py:89
(Diff revision 1)
> +
> +        self.repository.extra_data = {'required_ldap_group': 'some_scm_level'}
> +
> +        hook = FakeApprovalHook()
> +        hook.is_approved_child(self.review_request)
> +        mock_has_level_shipit.assert_called_with(self.review_request,

Should this be `assert_called_with()` or `assert_called_once_with()`?

::: pylib/mozreview/mozreview/tests/test-autoland-post-job.py:122
(Diff revision 1)
> +        mock_profile_get_or_create.return_value = [self.profile]
> +        mock_gen_latest_reviews.return_value = [self.review]

It appears that these same two test setup lines are present in every test, and they don't appear to vary in the tests themselves.  Is it possible to pull them into a common test setup routine?

The mock library can also call the `patch()` and `patch.object()` functions directly instead of calling them as decorators.  That way you can put them into a `setUp()` routine.

::: pylib/mozreview/mozreview/tests/test-autoland-post-job.py:215
(Diff revision 1)
> +        self.request.POST = {
> +            'review_request_id': [str(self.review_request.id)]
> +        }

Might want to turn this into a helper method on FakeHttpRequest.
Attachment #8849101 - Flags: review?(mars) → review-
(Assignee)

Comment 4

11 months ago
mozreview-review-reply
Comment on attachment 8849101 [details]
mozreview: respect the repository's configured ldap group for landing permissions (bug 1345927)

https://reviewboard.mozilla.org/r/121934/#review124244

> I think this logic is tricky to follow as it is written.  I think `scm_level` is trying to get set to some new value, and it's OK if the checks in the `if` statement on line 57 fail and no new value is set?
> 
> Is there some way to make all of the possible outcomes from this block of code explicit, to increase readability?  Maybe an `else` clause?

scm_level can take 3 values:
`force_group`, `Repository.extra_data['required_ldap_group']`, `DEFAULT_SCM_LEVEL`
If (for unknown reason) there is no `ReviewRequest` id in `request.POST` then `scm_level` stays at `DEFAULT_SCM_LEVEL`.
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
Comment on attachment 8849101 [details]
mozreview: respect the repository's configured ldap group for landing permissions (bug 1345927)

https://reviewboard.mozilla.org/r/121934/#review124244

> Docstring needs updating: it still implies L3-only, but the method applies to all levels.

I've also changed a documentation string.
Comment hidden (mozreview-request)
(Reporter)

Comment 7

11 months ago
mozreview-review
Comment on attachment 8849101 [details]
mozreview: respect the repository's configured ldap group for landing permissions (bug 1345927)

https://reviewboard.mozilla.org/r/121934/#review125512

::: pylib/mozreview/mozreview/static/mozreview/js/init_rr.js:20
(Diff revision 2)
> -  MozReview.hasScmLevel1 = MozReview.scmLevel >= 1;
> -  MozReview.hasScmLevel3 = MozReview.scmLevel == 3;
> +  MozReview.hasScmLevel = {
> +    'scm_level_1': (MozReview.scmLevel >= 1),
> +    'scm_level_2': (MozReview.scmLevel >= 2),
> +    'scm_level_3': (MozReview.scmLevel == 3)
> +  };

We need to be more sophisticated. Not all of the scm ldap groups take the form `scm_level_x`, including the group we need to use for conduit. Hardcoding scm_level_1 for try is fine, since try is a gecko only thing anyways, but any other grouping can be an arbitrary string. Please make sure this change supports that.
Attachment #8849101 - Flags: review-
Comment hidden (mozreview-request)

Updated

10 months ago
Attachment #8849101 - Flags: review?(mars)
You need to log in before you can comment on or make changes to this bug.