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.

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

Add more permission descriptions #5534

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Oct 2, 2022

The permission descriptions are copied from tooltips in Jenkins global security configuration.

Direct preview link: https://deploy-preview-5534--jenkins-io-site-pr.netlify.app/doc/book/security/access-control/permissions/

@zbynek zbynek requested a review from a team as a code owner October 2, 2022 07:08
@probot-autolabeler probot-autolabeler bot added the documentation Jenkins documentation, including user and developer docs, solution pages, etc. label Oct 2, 2022
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks very much for spending the time to extract those additional definitions. I've made some optional comments. I approve whether you take any action on the optional comments or not.

This will also need review by a member of the security team, since the change is in a section where they are a code owner.

Thanks again for the contribution

content/doc/book/security/access-control/permissions.adoc Outdated Show resolved Hide resolved
content/doc/book/security/access-control/permissions.adoc Outdated Show resolved Hide resolved
content/doc/book/security/access-control/permissions.adoc Outdated Show resolved Hide resolved
content/doc/book/security/access-control/permissions.adoc Outdated Show resolved Hide resolved
Co-authored-by: Mark Waite <[email protected]>
@MarkEWaite
Copy link
Contributor

@jenkins-infra/security could one of you review and approve the added role descriptions?

Copy link
Contributor

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Thanks for your interest in the security docs!

The reason I originally left this as a TODO is that the idea of this page was to go in-depth about permissions; and some of the implications of (lack of) permissions explained here were documented nowhere previously. Permissions are a difficult topic in Jenkins as they're not really well defined in many cases, and used on a best effort basis.

While some (barely above) placeholder-level content copied from Jenkins can be useful when docs are used standalone rather than as an additional tool when using Jenkins, there are problems with this content. These problems apply to Jenkins as well of course.

So I don't reject this documentation, but I also don't think it adds much value. It probably depends on whether you think listing stuff with minimal documentation that's listed elsewhere already is useful to have or not.

If you're willing to spend some more time on this topic I'd be happy to collaborate on a more extensive permission documentation that goes into more detail. Some of that we'd probably be able to adapt directly into Jenkins, and the more surprising aspects might even justify adding more form validation/admin monitors to inform admins what it is they're doing.

@@ -141,6 +234,33 @@ Learn more in jep:223[].
NOTE: This permission was added in Jenkins 2.222.
Some features, especially those provided by plugins, may not yet support this permission.

=== Credentials permissons
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be placed here as it's not an optional permission as explained in the introductory paragraph.

Also, while Credentials Plugin is as much of a "core" plugin as is possible, what's the threshold for putting plugin docs here?

content/doc/book/security/access-control/permissions.adoc Outdated Show resolved Hide resolved
Comment on lines 111 to 135
==== Agent/Build

This permission allows users to run jobs as them on agents.

==== Agent/Configure

This permission allows users to configure agents.

==== Agent/Connect

This permission allows users to connect agents or mark agents as online.

This permission is implied by Agent/Disconnect.

==== Agent/Create

This permission allows users to create agents.

==== Agent/Delete

This permission allows users to delete existing agents.

==== Agent/Disconnect

This permission allows users to disconnect agents or mark agents as temporarily offline.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably makes more sense as a list (e.g. definition list?), given the current lack of details? (Also applies to all other lists at this level.)

content/doc/book/security/access-control/permissions.adoc Outdated Show resolved Hide resolved
This permission grants discover access to jobs. Lower than read permissions, it allows you to redirect anonymous users to the login page when they try to access a job url.
Without it they would get a 404 error and wouldn't be able to discover project names.

This permission is implied by Job/Read.
Copy link
Contributor

Choose a reason for hiding this comment

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

This content is actually generated by Matrix Authorization Plugin 😄

Comment on lines 161 to 162
This permission grants discover access to jobs. Lower than read permissions, it allows you to redirect anonymous users to the login page when they try to access a job url.
Without it they would get a 404 error and wouldn't be able to discover project names.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here an explanation could make sense that this permission only makes sense when anonymous users are granted Overall/Read permission.

Comment on lines 115 to 127
==== Agent/Configure

This permission allows users to configure agents.

==== Agent/Connect

This permission allows users to connect agents or mark agents as online.

This permission is implied by Agent/Disconnect.

==== Agent/Create

This permission allows users to create agents.
Copy link
Contributor

Choose a reason for hiding this comment

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

How sensitive are these permissions? Can this be given to just anyone, or are there security considerations? This doesn't say. (Not that it would inside Jenkins, but I'd expect more from docs that exist. IMO of course.)


==== Agent/Build

This permission allows users to run jobs as them on agents.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically useless unless you already know what it does (it's related to https://www.jenkins.io/doc/book/security/build-authorization/ ).

This permission allows users to delete existing views.

==== View/Read
This permission allows users to see views (implied by generic read access).
Copy link
Contributor

Choose a reason for hiding this comment

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

implied by generic read

That's a permission that exists but hasn't been shown on the UI pretty much ever, so it's not particularly helpful.

@kmartens27
Copy link
Contributor

suggestions appear to be updated, thanks so much @zbynek!

@daniel-beck when you have a moment, would you be able to review the updates here and let us know if there's anything else that needs to be cleared up? Thanks!

@daniel-beck
Copy link
Contributor

Besides two minor formatting issues that have had suggested changes applied, my feedback remains unaddressed (neither acted on, nor explicitly rejected).

@zbynek zbynek marked this pull request as draft October 12, 2022 14:37
@zbynek
Copy link
Contributor Author

zbynek commented Oct 19, 2022

@daniel-beck I went through your review notes and extended some paragraphs a bit.

I also don't think it adds much value

Agreed. My goal was mostly to put the information somewhere where it's accessible and searchable. If you have some more pointers how to make this useful, please let me know.

@daniel-beck
Copy link
Contributor

If you have some more pointers how to make this useful, please let me know.

I don't think useful docs exist yet, therefore my offer to collaborate on creating that.

@zbynek zbynek requested a review from daniel-beck October 22, 2022 08:41
@kmartens27
Copy link
Contributor

Hi @daniel-beck, when you have a moment, would you be able to confirm and review the changes on this PR? Thanks!

@zbynek
Copy link
Contributor Author

zbynek commented Mar 10, 2023

@daniel-beck I know this is still pretty minimal, but do you think it's already worth merging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Jenkins documentation, including user and developer docs, solution pages, etc. hacktoberfest Hacktoberfest hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants