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-45942] - Attempt to solve issue #49

Merged
merged 7 commits into from Dec 30, 2018

Conversation

3 participants
@deepanshnagaria
Copy link
Contributor

commented Dec 26, 2018

Proposing a fix to bug #45942 ("Bad error message/silent death with incorrectly setup permissions in the RoleBasedAuthorizationStrategy") by introducing a check on the permissions collected and prompting an error message if there's a problem.

https://issues.jenkins-ci.org/browse/JENKINS-45942

#49

@runzexia
Copy link
Member

left a comment

I think you could format the code.
If you could add an NPE check here,should be able to avoid some problems.

@oleg-nenashev oleg-nenashev changed the title Attempt to solve issue #45942 [JENKINS-45492] - Attempt to solve issue Dec 26, 2018

@oleg-nenashev oleg-nenashev added the gsoc label Dec 26, 2018

@oleg-nenashev
Copy link
Member

left a comment

My recommendation would be to just ignore the null role and to print a warning to the System log instead of failing the serialization. Otherwise we may get into the situation when the in-memory model is updated but Jenkins cannot persist it to the disk. It may impact stability

writer.endNode();
}
else{
throw new NullPointerException("Cannot process the permissions as they have been incorrectly configured in the script");

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Dec 26, 2018

Member

I would rather recommend explicitly saying that null permission was discovered in the list of permission, preferable with a reference to the Role type and name.

This comment has been minimized.

Copy link
@deepanshnagaria

deepanshnagaria Dec 26, 2018

Author Contributor

I understand the above stated concern and concur that the situation is possible. In my view, throwing an error earlier, when the list was being built ( line=295 ) would be better. I shall include the role name along with type as you mentioned.

Please correct me if i am wrong or if any changes would improve the solution proposed.

Thanks.

This comment has been minimized.

Copy link
@deepanshnagaria

deepanshnagaria Dec 26, 2018

Author Contributor

screenshot 45
i am proposing this, i it fine? @oleg-nenashev @runzexia

@oleg-nenashev oleg-nenashev changed the title [JENKINS-45492] - Attempt to solve issue [JENKINS-45942] - Attempt to solve issue Dec 28, 2018

@oleg-nenashev
Copy link
Member

left a comment

This patch looks good to me in general. I am not sure it actually addresses JENKINS-45942 which likely creates the role in the Groovy code directly. But the patch definitely addresses a case which is probably more important.

I would recommend putting Permission.fromId(p) to a local field so that you do not invoke it twice in the code. The method is quite heavy

@oleg-nenashev
Copy link
Member

left a comment

Code itself looks good to me. Would be nice to get formatting fixed before the merge though. It is better to use 4-space separators like in the file + it is nice to have brackets even within 1-line conditions

@deepanshnagaria

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

I took the suggestion of storing the permission in a local variable.
As far the addressing the issue JENKINS-45942 is concerned, in my view its more a kind of protection which highlights the incorrectly set permissions causing a null permission in the array.
I shall do the changes you suggested. @oleg-nenashev actually i am formatting the code as you may see in the screenshot but i an still figuring out why it is not being highlighted in the commit.

Thanks.

deepanshnagaria and others added some commits Dec 28, 2018

final commit
Did basic formatting suggested.
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

Fixed the formatting + Improved the exception type and the text. Does it look good to you @deepanshnagaria ?

@deepanshnagaria

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

Looks fine to me but just to know was there any reason to switch to IOException? The compiler would have thrown a NPE in this case if we wouldn't have checked for the same. In my view if it was during the time of setting the permissions IOException would have made sense but for now while seeing a null permission I thought NPE should be thrown.
Please correct me if I am wrong.
Thanks.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

actually i am formatting the code as you may see in the screenshot but i an still figuring out why it is not being highlighted in the commit.

Tabs vs. Spaces, I'd guess

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

W.r.t the original question, I used IOException, because it is defined in the method interface. Using such checked exceptions requires API users to handle them, and I think it would be a positive improvement. But we can revert it or replace by new IOException(new NullPointerException(...)) if you prefer

@deepanshnagaria

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

actually i am formatting the code as you may see in the screenshot but i an still figuring out why it is not being highlighted in the commit.

Tabs vs. Spaces, I'd guess

i got that :-)

@deepanshnagaria

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

W.r.t the original question, I used IOException, because it is defined in the method interface. Using such checked exceptions requires API users to handle them, and I think it would be a positive improvement. But we can revert it or replace by new IOException(new NullPointerException(...)) if you prefer

No sir i don't have any issues as such but was just asking the reason for the change. Please tell me if any further changes are required or if there's another more effective way for the same issue.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

I think it's fine as is. Thanks for your contribution @deepanshnagaria !

@oleg-nenashev oleg-nenashev merged commit a2069c1 into jenkinsci:master Dec 30, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit 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.