Fix deleting a non-existing project tells you're unauthorized

[AnuragThePathak]

Signed-off-by: Anurag Pathak anuragpathak911@gmail.com Fixes #1750

What this PR does / why we need it: When a project delete request is sent, instead of directly checking for PROJECT_ADMIN role we'll now be checking if the project exists first so that we can avoid getting unauthorized error when the project doesn't exist.

Special notes for your reviewer:

If applicable:

• [ ] this PR contains documentation
• [ ] this PR contains unit tests
• [ ] this PR has been tested for backwards compatibility

[netlify[bot]]

✅ Deploy Preview for brigade-docs ready!

Name	Link
🔨 Latest commit	b624dff78a1abe2086274fbb13240c17ca923abd
🔍 Latest deploy log	https://app.netlify.com/sites/brigade-docs/deploys/623c859ac741830008ddbec2
😎 Deploy Preview	https://deploy-preview-1884--brigade-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[krancour]

@AnuragThePathak GitHub was experiencing issues earlier in the day and a comment I had intended to make on #1750 never went through...

What I was thinking is that only PROJECT_ADMIN and ADMIN have the privilege to delete a project.

System-level ADMIN has no specific project-level permissions.

Because if we tell them project doesn't exist users who doesn't even have read access will be able to know what projects exists.

A valid concern... to an extent...

In Brigade, read access is granted globally at the system level. This is because of a design decision very early on that we were not building SaaS, but rather a tool deployed/self-hosted by individual projects / companies / teams wherein broad read access is acceptable. (Note this does not extend to secrets, which are write-only anyway.) Making this decision early on unblocked a lot of other things. So, if you have Brigade projects whose very existence you wish to hide from your own users, you're recommended to run a separate Brigade instance with a more limited user roster.

In light of the above, I'm ok with any user with at least read access knowing that a given project does or does not exist because they already have access to that info anyway. So the only users this leaves us concerned with are users who were able to authenticate, but haven't yet been granted global read access.

So, perhaps then the best thing to do here is check for read global read access first. Then move on to checking for project existence. Then check that the user is authorized to delete the project.

So the conditions and results look like this:

• User has no read access -- not authorized
• User has read access, but project doesn't exist -- not found (not telling them anything they don't already know)
• User has read access, project exists, user is not a project admin -- not authorized
• User has read access, project exists, user is a project admin -- project is deleted

[AnuragThePathak]

Ok, I'll make the changes tomorrow. It's time for to sleep now. Even though it was a good first issue, I got to learn a lot.

[AnuragThePathak]

I have tested by revoking my READER access, it works as expected ( I was having ADMIN and PROJECT_ADMIN roles but that doesn't matter for read access as you've mentioned earlier).

[krancour]

/brig run

[AnuragThePathak]

A go unit test in github.com/brigadecore/brigade/v2/scheduler/internal/lib/queue/amqp is failing but I can't find out why.

[krancour]

ok github.com/brigadecore/brigade/v2/scheduler/internal/lib/queue/amqp 0.033s coverage: 66.7% of statements

☝️ That's actually not the failure. It's the last success.

The logs you see on GitHub are truncated. The top of the logs says this:

(Previous text omitted)

I retrieved the full log using brig event logs ... (can also use https://dashboard.brigade2.io).

Here's the actual failure:

--- FAIL: TestProjectServiceDelete (0.00s)
    --- FAIL: TestProjectServiceDelete/unauthorized (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1aad2e9]

goroutine 258 [running]:
testing.tRunner.func1.2({0x1c0d020, 0x29dc5f0})
    /usr/local/go/src/testing/testing.go:1389 +0x366
testing.tRunner.func1()
    /usr/local/go/src/testing/testing.go:1392 +0x5d2
panic({0x1c0d020, 0x29dc5f0})
    /usr/local/go/src/runtime/panic.go:844 +0x258
github.com/brigadecore/brigade/v2/apiserver/internal/api.(*projectsService).Delete(0xc000418b40, {0x2045b78, 0xc000046098}, {0x1d76a17, 0x3})
    /workspaces/brigade/v2/apiserver/internal/api/projects.go:412 +0x89
github.com/brigadecore/brigade/v2/apiserver/internal/api.TestProjectServiceDelete.func41(0x0?)
    /workspaces/brigade/v2/apiserver/internal/api/projects_test.go:733 +0x7c
testing.tRunner(0xc000263a00, 0xc0004311b0)
    /usr/local/go/src/testing/testing.go:1439 +0x214
created by testing.(*T).Run
    /usr/local/go/src/testing/testing.go:1486 +0x725
FAIL    github.com/brigadecore/brigade/v2/apiserver/internal/api    2.167s

This looks to me like you need to add / amend test cases for the TestProjectServiceDelete test.

Probably you're going to add a new test case that verifies correct behavior when the user doesn't have READ permissions, and then for the existing cases, you'll make sure the user does have this permission.

This is actually pretty easy to do by injecting mock "never authorize" or "always authorize" authorizer functions into each test case. (Have a look at that test and what I am sating will all make more sense.)

[krancour]

You might not have access to https://dashboard.brigade2.io/

I'll send you an invite to the @brigadecore org and that should grant you access.

[AnuragThePathak]

Btw I wanted to mention that whatever you told me regarding roles can be added to the docs in the authorization page or creating a new FAQ page.

[AnuragThePathak]

I was wondering if the discovered vulnerabilities during scans are because of my PR?

[krancour]

I was wondering if the discovered vulnerabilities during scans are because of my PR?

They're not. And those scans aren't required to pass. Mostly they find small problems with 3rd party libs that do not have fixes yet. Those scans were meant to keep such issues on our radar, but not meant to block PRs that didn't actually introduce the problems.

I still have to review this, but at a quick glance, it looks good.

[krancour]

fyi, I updated the names of two test cases in b624dff78a1abe2086274fbb13240c17ca923abd

The rest of this looks good. Still waiting on CI because GH seems to be having issues with delayed webhook delivery again.

[AnuragThePathak]

No problem

[krancour]

@AnuragThePathak your commits aren't verified (gpg signed).

I'm going to squash merge and sign the commit myself, but if you're able to sign future commits on other PRs it will be a huge help.

Here's a link with some instructions in case you need them:

https://github.com/brigadecore/community/blob/main/contributing.md#gpg-signature

[AnuragThePathak]

@krancour I've used both - - sign-off as well as - - gpg-sign flag while committing are all the commits not signed?

[krancour]

are all the commits not signed?

Nope

Did you add your public key to your GitHub account? If not, it's possible that you signed, but GitHub couldn't verify.

[AnuragThePathak]

@krancour as my commits in last PRs were verified, but not these, I believe that something wrong happened when I pulled upstream with --rebase when I was getting merge conflicts (can't exactly remember if it was merge conflict or I was not even able to push to origin) yesterday. Any way I can fix it right now?

[AnuragThePathak]

Unfortunately, if I rebase and sign then the last commit which was from you also gets signed my me in that process.

[AnuragThePathak]

Rebasing seems to be the only way out.

[krancour]

/brig run