Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

Add RBAC check to issue_batch api #1400

Merged
merged 3 commits into from
Dec 17, 2020
Merged

Add RBAC check to issue_batch api #1400

merged 3 commits into from
Dec 17, 2020

Conversation

whaught
Copy link
Contributor

@whaught whaught commented Dec 16, 2020

Proposed Changes

  • We currently perform this check on the bulk-issue page rendering, but we also need to check the API

Release Note

RBAC check for BulkIssue on the API

@google-cla google-cla bot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Dec 16, 2020
sethvargo
sethvargo suggested changes Dec 16, 2020
View reviewed changes
pkg/controller/issueapi/issue_batch.go Outdated
@@ -71,6 +72,13 @@ func (c *Controller) HandleBatchIssue() http.Handler {
return
}

if !membership.Can(rbac.CodeBulkIssue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can only do this check if it was called via the UI. There is no "membership" on the API.

I'm also pretty sure I did this - isn't there a specific handler for the UI server to server this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No the UI server remounts HandleIssue and HandleBulkIssue at another route (for ajax), but it's the same code.

What if we only check this if User is present (not authorized_app)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's fine? I just really dislike when a code path behaves dramatically different based on some other data that's a few layers of indirection away.

@whaught
Copy link
Contributor Author

whaught commented Dec 16, 2020

/hold

@whaught
Copy link
Contributor Author

whaught commented Dec 16, 2020

/unhold

sethvargo
sethvargo approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sethvargo, whaught

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot google-oss-robot merged commit bb3bdf0 into google:main Dec 17, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2020
@whaught whaught deleted the bulk-rbac branch December 22, 2020 01:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sethvargo sethvargo sethvargo approved these changes

@yegle yegle Awaiting requested review from yegle

Assignees

@sethvargo sethvargo

@mariliamelo mariliamelo

Labels
cla: yes Auto: added by CLA bot when all committers have signed a CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@whaught @google-oss-robot @sethvargo @mariliamelo