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

[JENKINS-41004] Do not report credentials with IDs masked by nearer folders #83

Merged
merged 6 commits into from Jan 27, 2017

Conversation

@stephenc
Copy link
Member

commented Jan 23, 2017

stephenc added 2 commits Jan 23, 2017
[JENKINS-41004] Do not report credentials with IDs masked by nearer f…
…olders

- Also fix a permission but where folder credentials were only available to SYSTEM and not available to authentications with USE_ITEM permission
[JENKINS-41004] Revert the 'also fix' from 6beb07a
- Added tests that show it was unnecessary, so reverting back to original behaviour as that matches more closely the System store in credentials plugin
if (ACL.SYSTEM.equals(authentication)) {
while (itemGroup != null) {
if (itemGroup instanceof AbstractFolder) {
if (!((AbstractFolder) itemGroup).getACL().hasPermission(authentication, USE_ITEM)) {

This comment has been minimized.

Copy link
@rsandell

rsandell Jan 25, 2017

Member

Well, you just verified three lines up that authentication is equal to ACL.SYSTEM so this seems redundant to me.

@rsandell

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

🐛 missing test that verifies a folder credential is chosen when there is a system credential with the same id.

@jglick
jglick approved these changes Jan 26, 2017
CredentialsMatchers.always())) {
if (!(c instanceof IdCredentials) || ids.add(((IdCredentials) c).getId())) {
// if IdCredentials, only add if we havent added already
// if not IdCredentials, always add

This comment has been minimized.

Copy link
@jglick

jglick Jan 26, 2017

Member

Are there any non-IdCredentials, really? Should we just deprecate that possibility?

This comment has been minimized.

Copy link
@stephenc

stephenc Jan 27, 2017

Author Member

I've see one implementation - cannot recall where - so I am not going to pull the plug on them until I have the implementation guidelines written and published

}
}
if (itemGroup instanceof Item) {
itemGroup = Item.class.cast(itemGroup).getParent();

This comment has been minimized.

Copy link
@jglick

jglick Jan 26, 2017

Member
itemGroup = ((Item) itemGroup).getParent();
stephenc added 2 commits Jan 27, 2017
[JENKINS-41004] Add tests that verify correct sequencing of resolution
- Also need to bump credentials plugin to 2.1.11 to ensure that the stores are identified in the correct sequence
@stephenc

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2017

@rsandell we should be good now! (assuming the CI server agrees)

@rsandell
Copy link
Member

left a comment

🐝

@stephenc stephenc merged commit ff0c530 into jenkinsci:master Jan 27, 2017

1 check passed

Jenkins This pull request looks good
Details

@stephenc stephenc deleted the stephenc:jenkins-41004 branch Jan 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.