User can self approved by removing entries in .codeowner

[pang-wu]

The current implementation check code owner from the PR's branch, but that introduce a problem: The user who make the PR can remove entries in the existing code owner file and add himself/herself in the file, then self-approved the PR.

Right now we work around this by usng github's native CODEOWNERS file to monitor .codeowners file changes, but this is not ideal. Can we check the ownership based on files in the base branch, rather than working branch?

[BakerNet]

Thanks for submitting the Issue.

Yeah, I had thought about this in the past, but we have enough trust internally that we didn't change it. That said, it probably should be at least configurable to prevent abuse.

There are some tradeoffs that come with using the base branch for ownership, though - when files are moved, or new code is added with a new .codeowners file for example you would not have owners enforced on that PR.

A couple optional paths forward:

1. Allow using base branch as configuration option
2. Sum owners from base branch and current (require both)
3. Do one of the above only when changes detected to a .codeowners file

[pang-wu]

or new code is added with a new .codeowners file for example you would not have owners enforced on that PR.

I think that behavior is expected isn't it -- if we add a new code owner file and there is no existing rules in base branch gating the file change, then the system should allow the change go in first. Then next round the owner rule should take effect.

[BakerNet]

BTW @pang-wu
You can achieve this now by just changing the actions/checkout to check out the base branch:

jobs:
  codeowners:
    name: 'Run Codeowners Plus'
    runs-on: ubuntu-latest
    steps:
      - name: 'Checkout Code Repository'
        uses: actions/checkout@v5
        with:
          ref: ${{ github.base_ref }}
          fetch-depth: 0

So you don't have to wait for a release

[pang-wu]

@BakerNet If we checkout the base, will that impact codeownerplus' ability to make diff?

[BakerNet]

If we checkout the base, will that impact codeownerplus' ability to make diff?

Not if you're using fetch-depth: 0

If you are using a different fetch-depth to have a light/greedy checkout, you would need to do something different, like git switch to the base ref in another step, or git fetch the head after checking out the base.

The important thing is that the history from base -> head is fetched by git.

[BakerNet]

or new code is added with a new .codeowners file for example you would not have owners enforced on that PR.

I think that behavior is expected isn't it -- if we add a new code owner file and there is no existing rules in base branch gating the file change, then the system should allow the change go in first. Then next round the owner rule should take effect.

Reflecting, I think you're correct, so changed Issue type to Bug. This is how Github CODEOWNERS works, it is definitely safer, and the trade-off I mentioned is much lower importance.

So I agree that this is how Codeowners Plus should work by default.