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

Status: RESOLVED INVALID

(MozReview Graveyard :: General, enhancement, P1)

Description

[Steven MacLeod [:smacleod]]

Currently only scm_level_3 is used. We should support other groups, allowing for more specific permissions.

Comment 1

[Steven MacLeod [:smacleod]]

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 2

[Piotr Zalewa [:zalun]]

Attachment: mozreview: respect the repository's configured ldap group for landing permissions (bug 1345927)

MozReview forces user to have SCM Level 3 permission to be able to land any repository. Since we've got a `required_ldap_group` stored in repository `extra_data`, we should use it.

Backend allows any string to be applied as a permission, but in the frontend allowed permissions are limited to "scm_level_1", "scm_level_2" and "scm_level_3".  The `webapi_scm_groups_required` decorator has now an optional `force_group` argument. Repository `required_ldap_group` is read if called without arguments. By default group is set to "scm_level_3".
Autoland itself is not checking if user has enough privileges. All changes are applied in MozReview. Unit tests are written to all methods created or modified. Since LDAP and Autoland is tested separately no integration test is provided.

Review commit: https://reviewboard.mozilla.org/r/121934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121934/

Comment 3

[Māris Fogels [:mars] (please needinfo)]

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.

Comment 4

[Piotr Zalewa [:zalun]]

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`.

Comment 5

[Piotr Zalewa [:zalun]]

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 6

[Piotr Zalewa [:zalun]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121934/diff/1-2/

Comment 7

[Steven MacLeod [:smacleod]]

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.

Comment 8

[Piotr Zalewa [:zalun]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121934/diff/2-3/

Comment 9

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.