JENKINS-60116: Allow 'EXTENDED_READ' permission to select credentials #165
Conversation
Although JENKINS-60116 already has been closed in November 2019, the faulted behaviour has not actually been fixed yet as reported by users in that ticket this year and reproduced by myself. The method `BitbucketSCMSource.DescriptorImpl.doFillCredentialsIdItems` was obviously still doing a check for the global 'CONFIGURE' permissions (which already explains the wrong behaviour and is redundant to the checks in the called method `BitbucketScmFormFillDelegate.doFillCredentialsIdItems` anyway). This commit adds the wanted behaviour of a context (item) based permission check. If a context is given, only the `Item.EXTENDED_READ` permission is required which follows the approach used by other SCM plugins (Git, Subversion). Furthermore, the redundant checks are removed and the tests have been adjusted, to use a mock of the now necessary context parameter.
|
Hello @alibk0rd, @dkjellin, @mhenschke-atl any feedback on this? Me and my client really would like to see this small but important fix in the next release. Thank you for your time! |
There was a problem hiding this comment.
Hello @pathob
Thank you for this PR. We have seen it and will review it shortly.
I don't think it will make the next release (which is due very soon) but I think we can make a new release with this fix shortly after.
The changes looks good to me, and once one more team member has looked at it and approved I think we can merge this, and build a release with it.
|
Hello @dkjellin,
Thank you, that sounds good! |
| @@ -45,8 +48,8 @@ public void testDoCheckServerId() { | |||
|
|
|||
| @Test | |||
| public void testDoFillCredentialsIdItems() { | |||
| descriptor.doFillCredentialsIdItems("myBaseUrl", "myCredentialsId"); | |||
| verify(formFill).doFillCredentialsIdItems("myBaseUrl", "myCredentialsId"); | |||
| descriptor.doFillCredentialsIdItems(item, "myBaseUrl", "myCredentialsId"); | |||
There was a problem hiding this comment.
I think here (and other POST methods we're testing) we should be mocking item privileges to ensure they're insufficient privileges are being rejected. That's a blind spot in the code base in general though, and I don't think it's needed to merge this PR.
ghost
mentioned this pull request
Although JENKINS-60116 already has been closed in November 2019, the faulted
behaviour has not actually been fixed yet as reported by users in that ticket
this year and reproduced by myself.
The method
BitbucketSCMSource.DescriptorImpl.doFillCredentialsIdItemswasobviously still doing a check for the global 'CONFIGURE' permissions (which
already explains the wrong behaviour and is redundant to the checks in the
called method
BitbucketScmFormFillDelegate.doFillCredentialsIdItemsanyway).This commit adds the wanted behaviour of a context (item) based permission
check. If a context is given, only the
Item.EXTENDED_READpermission isrequired which follows the approach used by other SCM plugins (Git, Subversion).
Furthermore, the redundant checks are removed and the tests have been adjusted,
to use a mock of the now necessary context parameter.