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

Folder specific access rights #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SofteqDG
Copy link

@SofteqDG SofteqDG commented Mar 6, 2018

Hello,
This PR adds an item specific access rights for folders. Everything works fine for me, but i'm not sure that code is good. @oleg-nenashev, please take a look on it.

@oleg-nenashev oleg-nenashev self-requested a review March 10, 2018 12:55
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Is there a real need to create a new 'FolderSpecificSecurity' class? Folders are subtype of Item in Jenkins, so the original class should do its job. From what I see you mostly copied the class, so probably it could be replaced by the original one

/**
* Implements folder-specific property map.
* This class relies on {@link AuthorizationMatrixProperty} from Jenkins core.
* @author Oleg Nenashev
Copy link
Member

Choose a reason for hiding this comment

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

I am not though it is largely a copy-paste from what I see

/*
* The MIT License
*
* Copyright 2013 Oleg Nenashev, Synopsys Inc.
Copy link
Member

Choose a reason for hiding this comment

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

If you lend a significant part of the code, please use 2013-2018 Oleg Nenashev, Synopsys Inc, Dmitry Gruzd or so

@SofteqDG
Copy link
Author

Hi @oleg-nenashev,
Thanks for review.

I'm not sure that i should add myself as author, cos FolderSpecificSecurity is almost a full copy of the ItemSpecificSecurity. The only difference between these classes is a class of the permissionsMatrix property. ItemSpecificSecurity uses an hudson.security.AuthorizationMatrixProperty class while FolderSpecificSecurity uses com.cloudbees.hudson.plugins.folder.properties.AuthorizationMatrixProperty class.

AuthorizationMatrixProperty from both packages has a common base class AuthorizationProperty and i was thinking to create a base class for both ItemSpecificSecurity and FolderSpecificSecurity. But AuthorizationProperty is annotated as @Restricted(NoExternalUse.class) and isn't publicy available. This is the real reason i need the FolderSpecificSecurity. Maybe you have ideas how to implement this logic in a better way?

PS: I just started to learn API of Jenkins and could miss some things.

@oleg-nenashev
Copy link
Member

OK, I will try to suggest an approach

@oleg-nenashev
Copy link
Member

We could work with @daniel-beck to lift the @Restricted(NoExternalUse.class) restriction in the AuthorizationProperty class, but it would require dependency bump and also a bump of Jenkins core to 2.60.3.

We could do such bump, but firstly I need to fix regressions in the last version of the plugin

@oleg-nenashev
Copy link
Member

I am happy to bump the dependency after #73 is integrated.
@SofteqDG WDYT?

@ssbarnea
Copy link

@SofteqDG can you please solve the conflict or close the PR if you don't want it anymore?

@oleg-nenashev
Copy link
Member

Updating & resolving merge conflicts is fine.
No need to close that if you have no time. There request is reasonable, I can mark it as "help-wanted" or so

@SofteqDG
Copy link
Author

@ssbarnea, @oleg-nenashev,
Sorry for a long delay. I will resolve conflicts in a day or two and we can go further with this PR.

@yx33
Copy link

yx33 commented Jul 29, 2021

Is there a chance that this functionality will appear in one of the next releases?

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

Successfully merging this pull request may close these issues.

None yet

4 participants