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

export slave remote fs for json api #3206

Merged
merged 14 commits into from May 26, 2018

Conversation

4 participants
@LinuxSuRen
Member

LinuxSuRen commented Dec 26, 2017

Put SlaveComputer's field absoluteRemoteFs export, so we can get the remove fs info.

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Jan 3, 2018

@daniel-beck please help to review this, thank you.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 3, 2018

I am not sure the default READ permission is enough to allow exposing this info.

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Jan 3, 2018

So what is youre advice? @oleg-nenashev

i used to think it is a normal info

@LinuxSuRen LinuxSuRen changed the title from export slave remove fs for json api to export slave remote fs for json api Jan 5, 2018

@oleg-nenashev oleg-nenashev self-requested a review Jan 6, 2018

@oleg-nenashev

@LinuxSuRen I have checked the code, and it appears that this info is exposed only to users with Computer.CONNECT permission. I recommend introducing a new API-only method which checks the permission

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Jan 7, 2018

@oleg-nenashev yea, it is my mistake, I will fixed it later.

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Jan 15, 2018

@oleg-nenashev Already add some code for check permission.

public String getAbsoluteRemoteFs() {
return channel == null ? null : absoluteRemoteFs;
}
/**
* Just for restFul api

This comment has been minimized.

@daniel-beck

daniel-beck Jan 15, 2018

Member

So, @Restricted(DoNotUse.class)?

This comment has been minimized.

@LinuxSuRen

LinuxSuRen Jan 15, 2018

Member

Yes it should be.

This comment has been minimized.

@LinuxSuRen

LinuxSuRen Jan 16, 2018

Member

It's done.

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Jan 17, 2018

@daniel-beck any issues?

@daniel-beck daniel-beck self-requested a review Jan 17, 2018

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Jan 19, 2018

@oleg-nenashev Change request already do well. Please help me to recheck.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jan 19, 2018

Note that there are no automatic permission checks on method call AFAICT, you will need to do that yourself. So just handling an exception that never gets thrown won't work.

I'd feel a lot more confident about this change if it included a test that shows that low privilege users don't get to see the value.

@oleg-nenashev

Javadoc description does not seem to be a production code one. I would rather remove it OR fix the text and @return

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Jan 21, 2018

I'am writing some unit test code for this, and javadoc.

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Mar 28, 2018

@oleg-nenashev I already added javadoc.

@oleg-nenashev

Looks good to me, excepting squashed imports. Could you please revert them? if you use IDE like IDEA, it's possible to disable automatic import squashing

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Mar 28, 2018

It's done.

public JenkinsRule j = new JenkinsRule();
@Test
public void testGetAbsoluteRemotePath() throws Exception {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 28, 2018

Member

I doubt this test actually tests anything, but works as a smoke test

This comment has been minimized.

@LinuxSuRen

LinuxSuRen Mar 28, 2018

Member

I want put some permission test case, but haven't figure it out who to do it.

@daniel-beck

This will throw an exception if a user does not have the permission, breaking the API.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 28, 2018

Test failures definitely from the permission check as in my review:

hudson.security.AccessDeniedException2: has-security-clearance is missing the Agent/Connect permission

This needs actual tests actually testing API output with and without Agent/Connect permission.

@daniel-beck

I really want WebClient based tests for this demonstrating it works as expected.

String path = computer.getAbsoluteRemotePath();
Node nodeA = j.createOnlineSlave();
String path = ((DumbSlave) nodeA).getComputer().getAbsoluteRemotePath();
assert(path != null);

This comment has been minimized.

@daniel-beck

daniel-beck Mar 29, 2018

Member

We're not using raw asserts (does that even work?), but Assert.* from JUnit.

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented Apr 28, 2018

@oleg-nenashev The unit test case is running well after the change.

nodeA = j.createOnlineSlave();
path = ((DumbSlave) nodeA).getComputer().getAbsoluteRemotePath();
Assert.assertNull(path);
Assert.assertEquals(getRemoteFS(nodeA), "null");

This comment has been minimized.

@daniel-beck

daniel-beck Apr 30, 2018

Member

Is null (the string) a reasonable output to have in the remote API? Or is that just a side effect of how #getRemoteFS(Node) works?

This comment has been minimized.

@LinuxSuRen

LinuxSuRen May 1, 2018

Member

null is returned by the remote API. That's the reason why I do this.

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

Then you did something wrong. IIUC getRemoteFS in this case should be throwing JSONException from getString.

This comment has been minimized.

@LinuxSuRen

LinuxSuRen May 1, 2018

Member

It will not throw JSONExcpetion that under my test.

private void setAsAnonymous() {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new AuthorizationStrategy(){

This comment has been minimized.

@daniel-beck

daniel-beck Apr 30, 2018

Member

This seems like a very error prone way to handle these permissions. MockAuthorizationStrategy would be a much better and more canonical way to implement this.

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

Right.

This comment has been minimized.

@LinuxSuRen

LinuxSuRen May 1, 2018

Member

Already fix this. Thanks for your tip.

/**
* @author suren
*/
public class SlaveComputerTest implements Serializable {

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

Serializable??

This comment has been minimized.

@LinuxSuRen

LinuxSuRen May 1, 2018

Member

Already remove it.

nodeA = j.createOnlineSlave();
path = ((DumbSlave) nodeA).getComputer().getAbsoluteRemotePath();
Assert.assertNull(path);
Assert.assertEquals(getRemoteFS(nodeA), "null");

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

Then you did something wrong. IIUC getRemoteFS in this case should be throwing JSONException from getString.

private void setAsAnonymous() {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
j.jenkins.setAuthorizationStrategy(new AuthorizationStrategy(){

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

Right.

nodeA = j.createOnlineSlave();
path = ((DumbSlave) nodeA).getComputer().getAbsoluteRemotePath();
Assert.assertNull(path);
Assert.assertEquals(getRemoteFS(nodeA, userAlice), "null");

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

This is clearly not right. Print response.getContentAsString() or include as an assertion message. We would expect either

"absoluteRemotePath":"/some/actual/value"

or nothing. (Stapler export omits reference-valued attributes when the getter returns null.)

This comment has been minimized.

@LinuxSuRen

LinuxSuRen May 1, 2018

Member

Bellow is my test sample data:

{"_class":"hudson.slaves.SlaveComputer","actions":[],"assignedLabels":[{"name":"slave1"}],"description":"dummy","displayName":"slave1","executors":[{}],"icon":"computer.png","iconClassName":"icon-computer","idle":true,"jnlpAgent":false,"launchSupported":true,"loadStatistics":{"_class":"hudson.model.Label$1"},"manualLaunchAllowed":true,"monitorData":{},"numExecutors":1,"offline":false,"offlineCause":null,"offlineCauseReason":"","oneOffExecutors":[],"temporarilyOffline":false,"absoluteRemotePath":null}

The value of 'absoluteRemotePath' is null.

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

Well, there may be a bug in Stapler then, but at any rate :null is tolerable, meaning the test code is wrong. Maybe JSONObject.getString does not do what you think.

This comment has been minimized.

@LinuxSuRen

LinuxSuRen May 1, 2018

Member

I'am have the same question at first, but the truth is

slave

This comment has been minimized.

@jglick

jglick May 1, 2018

Member

So do not use that method. The existence of JSONObject.isNullObject() indicates that get(String) is going to return something which is neither null nor a String in this case.

This comment has been minimized.

@LinuxSuRen

LinuxSuRen May 2, 2018

Member

The type of 'absoluteRemotePath' is not JSONObject, just a String.class. So, I can't call method 'getJSONObject'.

This comment has been minimized.

@jglick

jglick May 2, 2018

Member

Maybe try optString, or just opt and check for JSONString. There is some way to fix it. I do not know what it is offhand.

@jglick jglick dismissed their stale review May 1, 2018

more about misleading test code than any actual bug

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented May 4, 2018

@jglick Can you help me to review again? Thanks a lot!

@oleg-nenashev

LGTM

@oleg-nenashev oleg-nenashev requested review from daniel-beck and jglick May 5, 2018

@jglick

jglick approved these changes May 10, 2018

@jglick jglick added ready-for-merge and removed needs-review labels May 10, 2018

nodeA = j.createOnlineSlave();
path = ((DumbSlave) nodeA).getComputer().getAbsoluteRemotePath();
Assert.assertNull(path);
Assert.assertNull(getRemoteFS(nodeA, userAlice));

This comment has been minimized.

@daniel-beck

daniel-beck May 14, 2018

Member

So the point here is that, because alice does not have Computer.CONNECT, she does not see the path?

Would be nice to extend this to bob, who does have Computer.CONNECT, and can see the path. It's sort of part of the first part of this test, but since that has no security at all, there's no assertion for the correct permission check.

obsolete, just commenting now

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented May 21, 2018

It's been a long time, any problem with?

@oleg-nenashev oleg-nenashev merged commit 9680086 into jenkinsci:master May 26, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented May 26, 2018

Merged since there is no negative feedback about the production code.
@LinuxSuRen It would be great if you follow-up regarding #3206 (comment) in a new pull request

@LinuxSuRen LinuxSuRen deleted the suren-jenkins:slave-feature branch May 26, 2018

@LinuxSuRen

This comment has been minimized.

Member

LinuxSuRen commented May 26, 2018

@oleg-nenashev Get it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment