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

Listing credentials authorisation check #222

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

Conversation

ChampiYann
Copy link

@ChampiYann ChampiYann commented Mar 19, 2022

There is a comment in this Github issue detailing the problem this pull requests tries to fix.
It comes down to adding a check to the getCredentials method to check if the user requesting the credentials is permitted to use the credentials of a provider.

Proposed changelog entries

  • Fixes issue where credentials are not showing up in lists when a pipeline in a folder has it's "Acces Control for Builds" set to a specific user. Credentials now show up in the list if the specified user has the CredentialsProvider.USE_ITEM permission.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

@ChampiYann ChampiYann changed the title List credentials authorisation check Listing credentials authorisation check Mar 19, 2022
@ChampiYann ChampiYann marked this pull request as ready for review March 19, 2022 15:12
@ChampiYann ChampiYann requested a review from a team as a code owner March 19, 2022 15:12
@jtnord jtnord added the bug label Mar 21, 2022
}
if (itemGroup instanceof AbstractFolder) {
final AbstractFolder<?> folder = AbstractFolder.class.cast(itemGroup);
if (folder.hasPermission(authentication, CredentialsProvider.USE_ITEM)) {
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 this should allow for listing credentials if the user can also configure the itemgroup (as they may be able to select a credential that the system (not they themselves) will use later

(note the follow syntax has not been checked)

Suggested change
if (folder.hasPermission(authentication, CredentialsProvider.USE_ITEM)) {
if (folder.hasPermission(authentication, CredentialsProvider.USE_ITEM) || folder.hasPermission(authentication, Item.CONFIGURE)) {

@daniel-beck I always have to double think this - but for listing the credential IDs it should be you have configure on the item, or the ability to use a credntials. (configure so you can select a credential that the job/system can use even if you can not use it, USE_ITEM incase you can not configure the job (e.g. for the pipeline snippet generator at the job level and the pipeline is "as-code")?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I'm assuming we're waiting for @daniel-beck 's response on this. Is there any indication on when we can expect that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants