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-25726, JENKINS-25771] - StartedByUser and StartedByMemberOfGroup restrictions #5

Merged
merged 10 commits into from Jan 16, 2015

Conversation

Projects
None yet
3 participants
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Nov 22, 2014

The current version is just a draft

[WiP] - Non-tested draft for JENKINS-25726
Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Nov 22, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 23, 2014

Hi,

As I said in the commit message, it was just a [WiP] without any testing.
The issue appeared due to the typo in the resource package name.

I'll finalize the draft implementation today and let you know

Best regards,
Oleg Nenashev

2014-11-23 11:05 GMT+03:00 Christopher Suarez notifications@github.com:

You wan't me to wait for you or try to improve?


Reply to this email directly or view it on GitHub
#5 (comment)
.

[JENKINS-25726] - Fixed the previous implementation, added support of…
… anonymous users

Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>

@oleg-nenashev oleg-nenashev added this to the 0.4 milestone Nov 23, 2014

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 23, 2014

The new version works well on my test installation

@csms

This comment has been minimized.

Copy link

csms commented Nov 23, 2014

Hi, after looking at it on my machine after some various environment setup problems I also think it looks and works good. I also understand now why the functionality belongs in this plugin and not ownership plugin.

The following are my comments(based on personal opinions, feel free to ignore :-):

  • In boolean canTake(@nonnull List causes) shouldn't one be more defensive so that even if one user is allowed, if a later one in the list isn't then don't run the job. Make the code alot more complex on the other hand(Warning, example below not very thought through).
/**package*/ boolean canTake(@Nonnull List<Cause> causes) {
            boolean canTake=false;
        for (Cause cause : causes) {

            if (cause instanceof Cause.UserIdCause) {
                final @CheckForNull String startedBy = ((Cause.UserIdCause)cause).getUserId();
                if (acceptsUser(startedBy)) {
                    canTake=true;
                }
                else
                {
                    return false;
                }
            }
            if (checkUsersFromUpstreamProjects && cause instanceof Cause.UpstreamCause) {
                final List<Cause> upstreamCauses = ((Cause.UpstreamCause)cause).getUpstreamCauses();
                if (canTake(upstreamCauses)) { // Recursive call to iterate through all causes
                    canTake=true;
                }
                else
                {
                    return false;
                }
            }
            //TODO: check acceptAutomaticRuns
        }
        return canTake;
    }
  • If a user does not have a permission the job is stuck in waiting for next executor. Is there any way to fail the build stating that the user did not have permission so the user is notified?
  • I'll try to look at the code and see if I can contribute with a restriction for groups based on your implementation.

Awesome work.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 23, 2014

@csms
Thanks for comments and the positive feedback.
Feel free to develop new restrictions. I created this plugin in order to support Ownersip-based security on our installations, but hopefully it's generic enough to be used for other purposes.

In boolean canTake(@nonnull List causes) shouldn't one be more defensive so that even if one user is allowed, if a later one in the list isn't then don't run the job. Make the code alot more complex on the other hand(Warning, example below not very thought through).

Yes, it makes sense.
In addition, the logic should handle RebuildCause from Rebuild Plugin. This cause inherits UpstreamCause hence there's a security breach a security breach.

If a user does not have a permission the job is stuck in waiting for next executor. Is there any way to fail the build stating that the user did not have permission so the user is notified?

Newest core versions display the cause of blockage (see JENKINS-19250, since 1.567).
On the task submission It's possible to check all available executors ad check if a job has a permission to be executed on any of them. In such case it's possible to fail the build.

The downside is performance. You'll have to lock the queue for a long time to check executor statuses, hence I would recommend to avoid such approach on big installations

@csms

This comment has been minimized.

Copy link

csms commented Nov 23, 2014

Hmm, I tried a rebuild as another use(without permission on the node) with a parametrized build and it blocked the build, so that seems to work as it should - e.g the rebuild never started (if I didn't misunderstand you)

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 23, 2014

  1. Enable checkUsersFromUpstreamProjects in the node configuration
  2. Create a job
  3. Create an upstream job, which triggers your test jobs
  4. Run the upstream job using allowed user
  5. Rebuild the downstream job using another user
@csms

This comment has been minimized.

Copy link

csms commented Nov 24, 2014

Ok, cool.

I'm starting to get a bit more familiar with the code now. Based on your implementation I've made shot at (at least works in my tests with activedirectoryplugin) StartedByMemberOfGroupRestriction,
Would you like to extend the jira, or should I create a new one and make a pull request from that?

Of course it also suffers from the "upstream" security breach(going to try to investigate that).

I was thinking of adding an option(with helptext) for failing it based on your input on performance - by "task submission" do you mean I can get the executors from the cause in the cantake of Restriction?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 25, 2014

Would you like to extend the jira, or should I create a new one and make a pull request from that?

It's preferable to have a separate issue. BTW, we can easily merge PRs into a single one in order to remove code duplications

Of course it also suffers from the "upstream" security breach(going to try to investigate that).

I would just check for the class equality instead of instanceof (applies to UserIdCause as well). We can easily add the support of other causes later

I was thinking of adding an option(with helptext) for failing it based on your input on performance - by "task submission" do you mean I can get the executors from the cause in the cantake of Restriction?

QueueDecisionHandler is the right wait to implement the functionality IMO. After that you'll just need do block the build if all nodes do not satisfy JobRestrictions
http://javadoc.jenkins-ci.org/hudson/model/Queue.QueueDecisionHandler.html

@csms

This comment has been minimized.

Copy link

csms commented Nov 26, 2014

Ok. So here's an update.
I've managed to handle rebuild security so that seems to work according to my testing.
I've also backed on canceling the job in the queue since that would not give the user any appropriate feedback on the build. I'm now looking into trying to change the item.getWhy(); (That currently says "Waiting for next available executor") to something more informational around why the build doesn't start. Any ideas on how to achieve that?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 26, 2014

Thanks a lot! Looking forward for your pull request (It makes sense to use JENKINS-25726 branch as a target)

I've also backed on canceling the job in the queue since that would not give the user any appropriate feedback on the build. I'm now looking into trying to change the item.getWhy(); (That currently says "Waiting for next available executor") to something more informational around why the build doesn't start. Any ideas on how to achieve that?

What core version do you use? AFAIK @daniel-beck added the displaying of the blockage cause to the queue. I'm not sure how it works for Node-based restrictions

@csms

This comment has been minimized.

Copy link

csms commented Nov 26, 2014

Is this is what you're asking for? From my pom.xml:
org.jenkins-ci.plugins
plugin
1.588

Actually, it looks like I need to go to this version to get the stuff running, would that be ok?

@csms

This comment has been minimized.

Copy link

csms commented Dec 1, 2014

Hi(was away a few days). So now I've Implemented the check in an extension of QueueDecisionHandler. And as with the other things you were right. I also think it's the best solution. Especially since the queue will retry the job otherwise which probably will have a large impact on resources. Using QueueDecisionHandler we will check each node once and only once which minimizes calls to for example active directory.

However - I'm still challenged with trying to somehow notify the user that their build will not start on one or more nodes and why. I was hoping you would have any idea on how I could achieve that - now that I no longer need to show it in the "Build Executor Status". I have an idea that I could throw an exception with the information, but I would rather give a more graceful message. Any ideas?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Dec 3, 2014

Hi Christopher,

GitHub has been blocked in Russia. Our [foreign] company also follow the
rules. My provider also blocked my personal VPN channel due to unknown
reasons.

No idea when I'll be able to review Jenkins code and to provide suggestions.

Best regards,
Oleg Nenashev

2014-12-01 22:54 GMT+03:00 Christopher Suarez notifications@github.com:

Hi(was away a few days). So now I've Implemented the check in an extension
of QueueDecisionHandler. And as with the other things you were right. I
also think it's the best solution. Especially since the queue will retry
the job otherwise which probably will have a large impact on resources.
Using QueueDecisionHandler we will check each node once and only once which
minimizes calls to for example active directory.

However - I'm still challenged with trying to somehow notify the user that
their build will not start on one or more nodes and why. I was hoping you
would have any idea on how I could achieve that - now that I no longer need
to show it in the "Build Executor Status". I have an idea that I could
throw an exception with the information, but I would rather give a more
graceful message. Any ideas?


Reply to this email directly or view it on GitHub
#5 (comment)
.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 9, 2015

The PR should be reworked to be error-robust. I'm going to take the implementation from #6 after the merge

@oleg-nenashev oleg-nenashev self-assigned this Jan 9, 2015

Christopher Suarez and others added some commits Nov 25, 2014

Merge branch 'master' of https://github.com/csms/job-restrictions-plugin
 into JENKINS-25726

Conflicts:
	src/main/resources/com/synopsys/arc/jenkinsci/plugins/jobrestrictions/Messages.properties

Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
Use 1.509.3 as a target version
Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
[JENKINS-25771] - Fix the code formatting in StartedByMemberOfGroupRe…
…striction (the code won't be compiled)

Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
@oleg-nenashev

This comment has been minimized.

getActions() is enough IMO. It's hard to imagine a cause which comes from a transient action.
I'll modify the logic to make it compatible with 1.509.3

If you have any use-cases for transient actions, I can add their support as well

@oleg-nenashev

This comment has been minimized.

Bad practice, which strongly decreases the performance.
instanceof should be used

@oleg-nenashev

This comment has been minimized.

This comment has been minimized.

Copy link
Member

oleg-nenashev replied Jan 9, 2015

@csms
In any case, I want to switch the implementation to User::getAuthorities().
Seems it will have a better performance due to the internal caching.

Do you agree?

oleg-nenashev added some commits Jan 9, 2015

[JENKINS-25771] - Fix the implementation and the test code
Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
Refactoring: Move the shared logic to AbstractUserCauseRestriction
Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 9, 2015

@csms
The changes works well for user causes.
Could you check it for groups?

I'll definitely develop several unit tests, but now we need a manual check

@csms

This comment has been minimized.

Copy link

csms commented on f783d6b Jan 10, 2015

I agree with you I'm learning to crawl here :-).

@csms

This comment has been minimized.

Copy link

csms commented Jan 12, 2015

is it on this pull request that I should test?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 12, 2015

Yep. I merged your changes to this PR.

@csms

This comment has been minimized.

Copy link

csms commented Jan 12, 2015

Following test setup:
Plugin setup:
Mock Security Realm(used as realm)
Parameterized Trigger Plugin
Parameterized Remote Trigger Plugin
Nodelabel parameter plugin

job setup:
parent

  • child during build
    • grandchild during build
    • grandchild after build
  • child after build
    • grandchild during build
    • grandchild after build

Node setup:

  • Mocknode1
    • 10 executors
    • allow members of group mocknode1 to start jobs
  • Mocknode2
    • 10 executors
    • allow members of group mocknode2 to start jobs

Principal setup:
User mocknode1 - group mocknode1
User mocknode2 - group mocknode2

@csms

This comment has been minimized.

Copy link

csms commented Jan 12, 2015

I started parent job with nodelabel set to mocknode1 logged in as mocknode1.

It looks like the row:
final @checkfornull User usr = User.get(userId, false, null);
returns null when called in acceptUser.

Is there any better way/other way you prefer that I report this/show these?
I think I fetched the user like this:
SecurityRealm sr = Jenkins.getInstance().getSecurityRealm();
UserDetails userDetails = sr.loadUserByUsername(userId);

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 12, 2015

We can use the initial implementation as a fallback.
If I understand correctly, the user has not been registered in Jenkins when you started the test

@csms

This comment has been minimized.

Copy link

csms commented Jan 12, 2015

If you mean that they are not in "jenkin's own user database" then yes. They are in a realm, in this case the mock realm. I don't know wether requests to User.get normally get propagated to the realm but at least in this case it doesn't seem that way :-(.

(But the user did exist in the realm when I started the test, and I was logged in as it)

@csms

This comment has been minimized.

Copy link

csms commented Jan 12, 2015

As far as I understand I don't have permissions to change the code in this pr right?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jan 12, 2015

You can create a pull request to https://github.com/jenkinsci/job-restrictions-plugin/tree/JENKINS-25726.
If you want, I can grant you permissions to update this branch directly

@csms

This comment has been minimized.

Copy link

csms commented Jan 12, 2015

It's ok, whatever works best for you, I can also report my findings here if you want to change the code.

Christopher Suarez and others added some commits Jan 13, 2015

Merge pull request #8 from csms/JENKINS-25726
[JENKINS-26374] - Fallback to realm for principal groups

@oleg-nenashev oleg-nenashev changed the title [WiP] [JENKINS-25726] - StartedByUserRestriction [JENKINS-25726, JENKINS-25771] - StartedByUser and StartedByMemberOfGroup restrictions Jan 16, 2015

oleg-nenashev added a commit that referenced this pull request Jan 16, 2015

Merge pull request #5 from jenkinsci/JENKINS-25726
[JENKINS-25726, JENKINS-25771] - StartedByUser and StartedByMemberOfGroup restrictions

@oleg-nenashev oleg-nenashev merged commit 6767337 into master Jan 16, 2015

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.