Add checks to determine if a user can trigger jobs #13197

mirandachrist · 2019-06-27T00:25:37Z

This PR adds logic to check if a user is permitted to trigger job reruns but doesn't change the functionality. If the user is permitted to trigger reruns and the rerunCreatesJob flag is set to true, then the job will be triggered. Otherwise, it displays a popup that says the feature hasn't been implemented yet.
See design doc for more details.

/assign @cjwagner
/cc @fejta

mirandachrist · 2019-06-27T00:26:17Z

/assign @Katharine

mirandachrist · 2019-06-27T00:43:44Z

Forgot tests! Adding them now

prow/cmd/deck/main.go

stevekuznetsov · 2019-06-27T23:27:48Z

prow/cmd/deck/main.go

@@ -1172,18 +1188,28 @@ func handleRerun(prowJobClient prowv1.ProwJobInterface, createProwJob bool) http
 // On Prow instances that require auth even for viewing Deck this is okayish, because the Prowjob UUID
 // is hard to guess
 // Ref: https://github.com/kubernetes/test-infra/pull/12827#issuecomment-502850414
- if createProwJob {
- if r.Method != http.MethodPost {


So previously this would allow only a POST if we were creating a new prowjob. Now, we do nothing if it isn't POST but we don't error. Why?

If it isn't POST, we do the same thing /rerun does now (display the config YAML). This preserves the functionality for kubectl create -f "https://prow.k8s.io/rerun?prowjob=blahblah" and isn't necessarily an error. If someone tries to directly trigger a job but doesn't use a POST request, they'll just see the YAML which I think seems reasonable.

prow/cmd/deck/main.go

cjwagner · 2019-06-28T02:28:19Z

prow/cmd/deck/main.go

+ return
+ }
+ if !canTriggerJob(githubCookie.Value, cfg().Deck.RerunAuthConfig) {
+ http.Redirect(w, r, "/?rerun=denied", http.StatusForbidden)


I don't think http.Redirect can successfully redirect without a 3XX status code. In particular I think this would need to respond with 303 since this is a response to a POST request.

That being might be more idiomatic to respond with http.StatusForbidded and a brief error message. The code that sends the POST request could be responsible for handling the request error and redirecting. I'm not sure what all the pros and cons are here.

I feel like the best UX would be to go back to the prow status page and show the popup saying they don't have permission to rerun the job. If 303 is necessary for that, I'd still rather do it, even if it's a bit odd

I still want to see this happen without sending the user around with links and redirects - I don't think links and redirects are the right way to implement this feature.

If we make rerunning into an API that the frontend can use then we don't need to carry frontend state around the world or insert it into other parts of the code.

If we make rerunning into an API that the frontend can use
+123132

This will come in a later change

prow/cmd/deck/main.go

cjwagner · 2019-06-28T03:08:13Z

prow/cmd/deck/static/prow/prow.ts

- if (rerunStatus != null) {
+ if (rerunStatus === "denied") {
+ modal.style.display = "block";
+ rerunCommand.innerHTML = `You don't have permission to rerun that job. Try asking @cjwagner.`;


Thats evil 👿
Please no 🙏

Can we also recommend contacting on-cole to help resolve for any sandwich-related disputes?

I'll just respond to those now.
It's a sandwich.

prow/cmd/deck/static/prow/prow.ts

fejta

nits, otherwise LGTM

prow/cmd/deck/main.go

prow/cmd/deck/static/prow/prow.ts

prow/cmd/deck/main.go

prow/cmd/deck/main_test.go

Katharine

I continue to oppose the approach taken to providing the endpoint for this feature. Furthermore, the current implementation in this PR is wildly insecure.

In particular:

Instead of actually using the GitHub oauth token, we check the value of the github_login cookie. This is stored in plaintext on the user's machine and with this implementation can trivially be changed to be impersonate any user. Instead, we should use and verify the supplied GitHub OAuth token, or at least cryptographically sign the username cookie so it cannot be manipulated. Functionally this means there is no authentication.
There is still no CSRF protection. As discussed at some length in Deck: Optionally allow to make the rerun button create a new Job #12827 (comment), this is not acceptable once we add authentication.
The means by which the reruns are triggered is not ideal. The current approach is to redirect the user between several pages and attempt to keep our UI state by including it in query strings. I would significantly prefer to see the rerun endpoint exposed as an API that is then requested by frontend javascript.

Please do not merge without addressing these issues. In particular, the first two are absolutely hard blockers.

…d rerunning directly

Katharine

Authentication fix looks good to me.

Approving on the understanding that CSRF protection and improved UX flow / API design will be added in followup PRs.

k8s-ci-robot · 2019-07-03T22:42:59Z

LGTM label has been added.

Git tree hash: a0889d19c48411f4edab269ad545826b658dc310

k8s-ci-robot · 2019-07-03T22:43:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Katharine, mirandachrist

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:

~~prow/OWNERS~~ [Katharine]

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

k8s-ci-robot assigned cjwagner Jun 27, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 27, 2019

k8s-ci-robot requested a review from fejta June 27, 2019 00:25

k8s-ci-robot added area/prow Issues or PRs related to prow area/prow/deck Issues or PRs related to prow's deck component sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2019

k8s-ci-robot assigned Katharine Jun 27, 2019

mirandachrist force-pushed the whitelist branch from 70ab0be to b4fb020 Compare June 27, 2019 01:26

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2019

mirandachrist force-pushed the whitelist branch from b4fb020 to 8428606 Compare June 27, 2019 01:35

fejta reviewed Jun 27, 2019

View reviewed changes

prow/cmd/deck/main.go Outdated Show resolved Hide resolved

prow/cmd/deck/main.go Outdated Show resolved Hide resolved

mirandachrist force-pushed the whitelist branch from 750f865 to 2d874d1 Compare June 27, 2019 21:02

stevekuznetsov reviewed Jun 27, 2019

View reviewed changes

mirandachrist force-pushed the whitelist branch from 2d874d1 to 45c944b Compare June 28, 2019 00:55

cjwagner reviewed Jun 28, 2019

View reviewed changes

k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Jun 28, 2019

mirandachrist force-pushed the whitelist branch from 45c944b to 2f931c6 Compare June 28, 2019 18:08

fejta reviewed Jun 28, 2019

View reviewed changes

Katharine suggested changes Jun 28, 2019

View reviewed changes

mirandachrist added 2 commits July 2, 2019 11:05

Add checks to determine if a user can trigger jobs

44c45cd

Modify handleRerun to support both rerunning from the command line an…

64c4e48

…d rerunning directly

mirandachrist force-pushed the whitelist branch from 2f931c6 to 64c4e48 Compare July 2, 2019 18:11

k8s-ci-robot added the area/prow/bump Updates to the k8s prow cluster label Jul 2, 2019

mirandachrist mentioned this pull request Jul 3, 2019

Allow direct triggering of reruns #13321

Closed

Katharine approved these changes Jul 3, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2019

k8s-ci-robot merged commit 132f32f into kubernetes:master Jul 3, 2019

mirandachrist mentioned this pull request Jul 16, 2019

Reduce use of query params for /rerun #13479

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checks to determine if a user can trigger jobs #13197

Add checks to determine if a user can trigger jobs #13197

mirandachrist commented Jun 27, 2019

mirandachrist commented Jun 27, 2019

mirandachrist commented Jun 27, 2019

stevekuznetsov Jun 27, 2019

mirandachrist Jun 28, 2019

cjwagner Jun 28, 2019

mirandachrist Jun 28, 2019

Katharine Jun 28, 2019

stevekuznetsov Jul 2, 2019

mirandachrist Jul 2, 2019

cjwagner Jun 28, 2019

fejta Jun 28, 2019 •

edited

Loading

cjwagner Jun 28, 2019

fejta left a comment

Katharine left a comment •

edited

Loading

Katharine left a comment •

edited

Loading

k8s-ci-robot commented Jul 3, 2019

k8s-ci-robot commented Jul 3, 2019

Add checks to determine if a user can trigger jobs #13197

Add checks to determine if a user can trigger jobs #13197

Conversation

mirandachrist commented Jun 27, 2019

mirandachrist commented Jun 27, 2019

mirandachrist commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta left a comment

Choose a reason for hiding this comment

Katharine left a comment • edited Loading

Choose a reason for hiding this comment

Katharine left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 3, 2019

k8s-ci-robot commented Jul 3, 2019

fejta Jun 28, 2019 •

edited

Loading

Katharine left a comment •

edited

Loading

Katharine left a comment •

edited

Loading