Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add documentation for why we need CSRF protection and how the protection works. #13430

Closed
wants to merge 1 commit into from
Closed

Add documentation for why we need CSRF protection and how the protection works. #13430

wants to merge 1 commit into from

Conversation

mirandachrist
Copy link
Contributor

As requested in #13323

/assign @fejta
/cc @cjwagner @Katharine

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 12, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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. labels Jul 12, 2019
fejta
fejta approved these changes Jul 12, 2019
View reviewed changes
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/hold
How are people supposed to find this? I suspect no one will ever read it.

Can we link to it from deck's main readme, or more likely the place in deck's code where we require (when I'm reading that code is the most likely time I'll be interested in getting more info).

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 12, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 866843ba40aeb384fa3e5063b019e23afa1772c5

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, 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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2019
Katharine
Katharine reviewed Jul 12, 2019
View reviewed changes
Copy link
Member

@Katharine Katharine left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why is this in a separate PR to the actual implementation of CSRF protection?

prow/cmd/deck/csrf.md Outdated Show resolved Hide resolved
prow/cmd/deck/csrf.md Outdated Show resolved Hide resolved
prow/cmd/deck/csrf.md Show resolved Hide resolved
prow/cmd/deck/csrf.md Show resolved Hide resolved
@mirandachrist
Create csrf.md
6bd13ec 
Adds documentation for why we need CSRF protection and how the protection works.
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 15, 2019
@mirandachrist
Copy link
Contributor Author

Why is this in a separate PR to the actual implementation of CSRF protection?

My pixelbook and I were not getting along. Should I add it to the other PR?

@mirandachrist
Copy link
Contributor Author

/hold
How are people supposed to find this? I suspect no one will ever read it.

Can we link to it from deck's main readme, or more likely the place in deck's code where we require (when I'm reading that code is the most likely time I'll be interested in getting more info).

There isn't a deck readme as far as I can tell. I linked it in main.go in my other PR

@fejta
Copy link
Contributor

fejta commented Jul 15, 2019

Adding this to the PR that adds CSRF protection SGTM

@mirandachrist mirandachrist deleted the patch-2 branch July 17, 2019 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Katharine Katharine Katharine left review comments

@fejta fejta fejta approved these changes

@cjwagner cjwagner Awaiting requested review from cjwagner

Assignees

@fejta fejta

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/prow/deck Issues or PRs related to prow's deck component area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mirandachrist @k8s-ci-robot @fejta @Katharine