Skip to content

Commit

Permalink
refactor: use is_private and workspace membership to determine eligib…
Browse files Browse the repository at this point in the history 
…ility to reopen
  • Loading branch information
@setchy
setchy committed Jun 27, 2023
1 parent 3514596 commit 86361d3
Show file tree
Hide file tree
Showing 5 changed files with 185 additions and 176 deletions.
90 changes: 56 additions & 34 deletions lib/modules/platform/bitbucket/index.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,17 +828,7 @@ describe('modules/platform/bitbucket/index', () => {
expect(pr?.number).toBe(5);
});

it('finds closed pr with reopen comment from non-authorized user', async () => {
const reviewer = {
type: 'default_reviewer',
reviewer_type: 'project',
user: {
display_name: 'Bob Smith',
uuid: '{d2238482-2e9f-48b3-8630-de22ccb9e42f}',
account_id: '123',
},
};

it('finds closed pr with reopen comment on private repository', async () => {
const prComment = {
content: {
raw: 'reopen! comment',
Expand All @@ -850,7 +840,7 @@ describe('modules/platform/bitbucket/index', () => {
},
};

const scope = await initRepoMock();
const scope = await initRepoMock({}, { is_private: true });
scope
.get(
'/2.0/repositories/some/repo/pullrequests?state=OPEN&state=MERGED&state=DECLINED&state=SUPERSEDED&pagelen=50'
Expand All @@ -866,43 +856,75 @@ describe('modules/platform/bitbucket/index', () => {
},
],
})
.get(
'/2.0/repositories/some/repo/effective-default-reviewers?pagelen=100'
)
.reply(200, { values: [reviewer] })
.get('/2.0/repositories/some/repo/pullrequests/5/comments?pagelen=100')
.reply(200, { values: [prComment] });

const pr = await bitbucket.findPr({
branchName: 'branch',
prTitle: 'title',
});
expect(pr?.number).toBe(5);
expect(pr).toBeNull();
});

it('finds closed pr with reopen comment from authorized user', async () => {
const bbUser = {
user: {
display_name: 'Bob Smith',
uuid: '{d2238482-2e9f-48b3-8630-de22ccb9e42f}',
account_id: '123',
it('finds closed pr with reopen comment on public repository from workspace member', async () => {
const workspaceMember = {
display_name: 'Jane Smith',
uuid: '{90b6646d-1724-4a64-9fd9-539515fe94e9}',
account_id: '456',
};

const prComment = {
content: {
raw: 'reopen! comment',
},
user: workspaceMember,
};

const reviewer = {
type: 'default_reviewer',
reviewer_type: 'project',
user: bbUser,
const scope = await initRepoMock({}, { is_private: false });
scope
.get(
'/2.0/repositories/some/repo/pullrequests?state=OPEN&state=MERGED&state=DECLINED&state=SUPERSEDED&pagelen=50'
)
.reply(200, {
values: [
{
id: 5,
source: { branch: { name: 'branch' } },
destination: { branch: { name: 'master' } },
title: 'title',
state: 'closed',
},
],
})
.get('/2.0/repositories/some/repo/pullrequests/5/comments?pagelen=100')
.reply(200, { values: [prComment] })
.get(
'/2.0/workspaces/some/members/%7B90b6646d-1724-4a64-9fd9-539515fe94e9%7D'
)
.reply(200, { values: [workspaceMember] });

const pr = await bitbucket.findPr({
branchName: 'branch',
prTitle: 'title',
});
expect(pr).toBeNull();
});

it('finds closed pr with reopen comment on public repository from non-workspace member', async () => {
const nonWorkspaceMember = {
display_name: 'Jane Smith',
uuid: '{90b6646d-1724-4a64-9fd9-539515fe94e9}',
account_id: '456',
};

const prComment = {
content: {
raw: 'reopen! comment',
},
user: bbUser,
user: nonWorkspaceMember,
};

const scope = await initRepoMock();
const scope = await initRepoMock({}, { is_private: false });
scope
.get(
'/2.0/repositories/some/repo/pullrequests?state=OPEN&state=MERGED&state=DECLINED&state=SUPERSEDED&pagelen=50'
Expand All @@ -918,18 +940,18 @@ describe('modules/platform/bitbucket/index', () => {
},
],
})
.get('/2.0/repositories/some/repo/pullrequests/5/comments?pagelen=100')
.reply(200, { values: [prComment] })
.get(
'/2.0/repositories/some/repo/effective-default-reviewers?pagelen=100'
'/2.0/workspaces/some/members/%7B90b6646d-1724-4a64-9fd9-539515fe94e9%7D'
)
.reply(200, { values: [reviewer] })
.get('/2.0/repositories/some/repo/pullrequests/5/comments?pagelen=100')
.reply(200, { values: [prComment] });
.reply(404);

const pr = await bitbucket.findPr({
branchName: 'branch',
prTitle: 'title',
});
expect(pr).toBeNull();
expect(pr?.number).toBe(5);
});
});

Expand Down
143 changes: 125 additions & 18 deletions lib/modules/platform/bitbucket/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ import { repoFingerprint } from '../util';
import { smartTruncate } from '../utils/pr-body';
import { readOnlyIssueBody } from '../utils/read-only-issue-body';
import * as comments from './comments';
import { effectiveDefaultReviewers, sanitizeReviewers } from './reviewers';
import type {
Account,
BitbucketStatus,
BranchResponse,
Config,
EffectiveReviewer,
PagedResult,
PrResponse,
RepoBranchingModel,
Expand All @@ -48,7 +48,7 @@ import { mergeBodyTransformer } from './utils';

export const id = 'bitbucket';

export const bitbucketHttp = new BitbucketHttp();
const bitbucketHttp = new BitbucketHttp();

const BITBUCKET_PROD_ENDPOINT = 'https://api.bitbucket.org/';

Expand Down Expand Up @@ -211,6 +211,7 @@ export async function initRepo({
owner: info.owner,
mergeMethod: info.mergeMethod,
has_issues: info.has_issues,
isPrivate: info.isPrivate,
};

logger.debug(`${repository} owner = ${config.owner}`);
Expand Down Expand Up @@ -318,19 +319,22 @@ export async function findPr({
const reopenComments = await comments.reopenComments(config, pr.number);

if (is.nonEmptyArray(reopenComments)) {
const reviewers = await effectiveDefaultReviewers(config.repository);
const reviewerUuids = reviewers.map((reviewer) => reviewer.uuid);

const authorizedReopenComments = reopenComments.filter((comment) =>
reviewerUuids.includes(comment.user.uuid)
);

if (is.nonEmptyArray(authorizedReopenComments)) {
if (config.isPrivate) {
// Only workspace members could have commented on a private repository
logger.debug(
`Found '${comments.REOPEN_PR_COMMENT_KEYWORD}' comment from authorized repository member. Renovate will reopen PR ${pr.number} as a new PR`
`Found '${comments.REOPEN_PR_COMMENT_KEYWORD}' comment from workspace member. Renovate will reopen PR ${pr.number} as a new PR`
);
return null;
}

for (const comment of reopenComments) {
if (await isAccountMemberOfWorkspace(comment.user, config.repository)) {
logger.debug(
`Found '${comments.REOPEN_PR_COMMENT_KEYWORD}' comment from workspace member. Renovate will reopen PR ${pr.number} as a new PR`
);
return null;
}
}
}
}

Expand Down Expand Up @@ -737,6 +741,102 @@ export function ensureCommentRemoval(
return comments.ensureCommentRemoval(config, deleteConfig);
}

async function sanitizeReviewers(
reviewers: Account[],
err: any
): Promise<Account[] | undefined> {
if (err.statusCode === 400 && err.body?.error?.fields?.reviewers) {
const sanitizedReviewers: Account[] = [];

const MSG_AUTHOR_AND_REVIEWER =
'is the author and cannot be included as a reviewer.';
const MSG_MALFORMED_REVIEWERS_LIST = 'Malformed reviewers list';
const MSG_NOT_WORKSPACE_MEMBER =
'is not a member of this workspace and cannot be added to this pull request';

for (const msg of err.body.error.fields.reviewers) {
// Bitbucket returns a 400 if any of the PR reviewer accounts are now inactive (ie: disabled/suspended)
if (msg === MSG_MALFORMED_REVIEWERS_LIST) {
logger.debug(
{ err },
'PR contains reviewers that may be either inactive or no longer a member of this workspace. Will try setting only active reviewers'
);

// Validate that each previous PR reviewer account is still active
for (const reviewer of reviewers) {
const reviewerUser = (
await bitbucketHttp.getJson<Account>(`/2.0/users/${reviewer.uuid}`)
).body;

if (reviewerUser.account_status === 'active') {
// There are cases where an active user may still not be a member of a workspace
if (await isAccountMemberOfWorkspace(reviewer, config.repository)) {
sanitizedReviewers.push(reviewer);
}
}
}
// Bitbucket returns a 400 if any of the PR reviewer accounts are no longer members of this workspace
} else if (msg.endsWith(MSG_NOT_WORKSPACE_MEMBER)) {
logger.debug(
{ err },
'PR contains reviewer accounts which are no longer member of this workspace. Will try setting only member reviewers'
);

// Validate that each previous PR reviewer account is still a member of this workspace
for (const reviewer of reviewers) {
if (await isAccountMemberOfWorkspace(reviewer, config.repository)) {
sanitizedReviewers.push(reviewer);
}
}
} else if (msg.endsWith(MSG_AUTHOR_AND_REVIEWER)) {
logger.debug(
{ err },
'PR contains reviewer accounts which are also the author. Will try setting only non-author reviewers'
);
const author = msg.replace(MSG_AUTHOR_AND_REVIEWER, '').trim();
for (const reviewer of reviewers) {
if (reviewer.display_name !== author) {
sanitizedReviewers.push(reviewer);
}
}
} else {
return undefined;
}
}

return sanitizedReviewers;
}

return undefined;
}

async function isAccountMemberOfWorkspace(
reviewer: Account,
repository: string
): Promise<boolean> {
const workspace = repository.split('/')[0];

try {
await bitbucketHttp.get(
`/2.0/workspaces/${workspace}/members/${reviewer.uuid}`
);

return true;
} catch (err) {
// HTTP 404: User cannot be found, or the user is not a member of this workspace.
if (err.statusCode === 404) {
logger.debug(
{ err },
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`User ${reviewer.display_name} is not a member of the workspace ${workspace}. Will be removed from the PR`
);

return false;
}
throw err;
}
}

// Creates PR and returns PR number
export async function createPr({
sourceBranch,
Expand All @@ -754,7 +854,18 @@ export async function createPr({
let reviewers: Account[] = [];

if (platformOptions?.bbUseDefaultReviewers) {
reviewers = await effectiveDefaultReviewers(config.repository);
const reviewersResponse = (
await bitbucketHttp.getJson<PagedResult<EffectiveReviewer>>(
`/2.0/repositories/${config.repository}/effective-default-reviewers`,
{
paginate: true,
}
)
).body;
reviewers = reviewersResponse.values.map((reviewer: EffectiveReviewer) => ({
uuid: reviewer.user.uuid,
display_name: reviewer.user.display_name,
}));
}

const body = {
Expand Down Expand Up @@ -791,7 +902,7 @@ export async function createPr({
return pr;
} catch (err) /* istanbul ignore next */ {
// Try sanitizing reviewers
const sanitizedReviewers = await sanitizeReviewers(config, reviewers, err);
const sanitizedReviewers = await sanitizeReviewers(reviewers, err);

if (sanitizedReviewers === undefined) {
logger.warn({ err }, 'Error creating pull request');
Expand Down Expand Up @@ -845,11 +956,7 @@ export async function updatePr({
);
} catch (err) {
// Try sanitizing reviewers
const sanitizedReviewers = await sanitizeReviewers(
config,
pr.reviewers,
err
);
const sanitizedReviewers = await sanitizeReviewers(pr.reviewers, err);

if (sanitizedReviewers === undefined) {
throw err;
Expand Down
Loading

0 comments on commit 86361d3

Please sign in to comment.