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.

Sign up for GitHub

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

Jump to bottom

setting paramName only when it is not null #6258

Closed

shabari004 wants to merge 7 commits into spring-projects:master from shabari004:gh-17565-npe

shabari004 commented Dec 7, 2018

#checking whether paramNames[i] is null, if it is not null then only we call StandardEvaluationContext.setVariable method


          setting paramName only when it is not null

9c3fbce

pivotal-issuemaster commented Dec 7, 2018

@shabari004 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented Dec 7, 2018

@shabari004 Thank you for signing the Contributor License Agreement!

rwinch mentioned this pull request

avoid set null variable with @PreAuthorize and @P Fixes: gh-6300 #6301

Closed

rwinch requested changes

View reviewed changes

Member

rwinch left a comment

Thank you for the PR @shabari004!

Can you please update the commit message to align with the Spring Security conventions? Specifically, please ensure you have Fixes: gh-6223
Can you please add a test? You should be able to
- Remove super from here
- In the test extend MethodSecurityEvaluationContext and override setVariable method to throw an Exception if a null value is passed in.

rwinch self-assigned this

rwinch added the status: waiting-for-feedback label

shabari004 added 2 commits

December 18, 2018 16:12


          setting paramName only when it is not null

67adf30

Fixes : spring-projectsgh-6223


          Merge branch 'gh-17565-npe' of https://github.com/shabari004/spring-s…

…ecurity into gh-17565-npe

Author

shabari004 commented Dec 18, 2018

I updated the commit message and submit again (sorry about that).
For creating test class, I created a test class namedMethodSecurityAddArgumentsTests. I added the file below, I'm not sure how much help i can ask, if you dont mind can you check it please
MethodSecurityAddArgumentsTests.txt

Member

rwinch commented Dec 19, 2018

Thanks for the feedback. It is probably best to just include the files in the PR. We can modify them as needed. That makes it easier to review and provide feedback.

For the tests:

I'd start by creating a standard test named MethodSecurityEvaluationContextTests.
Then I would create a static inner class named NotNullVariableMethodSecurityEvaluationContext that behaves exactly as you have defined MethodSecurityAddArgumentsTests.setVariable
Then create a test that creates a NotNullVariableMethodSecurityEvaluationContext instance and injects a mock ParameterNameDiscoverer that returns a new String[] { null }.
Next invoke NotNullVariableMethodSecurityEvaluationContext.lookupVariable on the instance created in the previous step. Before your change this would then invoke the setVariable method with a null variable name. Since this test overrides the setVariable method to forbid a null value, if there is no Exception at this point, we know the code is fixed. You can validate that the test is real by temporarily removing the null check and see that it will fail.


          adding Junit test MethodSecurityEvaluationContext setVariable

Fixes: spring-projectsgh-6223

Author

shabari004 commented Dec 19, 2018

I created the test cases as you mentioned. can you review it please. Thank you.

rwinch requested changes

View reviewed changes

Member

rwinch left a comment

Thanks! The code/tests look good.

I have a few stylistic comments inline that need fixed.
Can you please squash your commits?
Can you please update the commit message to align with the Spring Security conventions? Specifically can you ensure to have a meaningful subject and Fixes: gh-17565 at the end of the commit message?

.../springframework/security/access/expression/method/MethodSecurityEvaluationContextTests.java Outdated

+              		ParameterNameDiscoverer paramNameDiscoverer = mock(ParameterNameDiscoverer.class);
+              		when(paramNameDiscoverer.getParameterNames(method)).thenReturn( new String[] {null});
+              		NotNullVariableMethodSecurityEvaluationContext NNVMSEC= new NotNullVariableMethodSecurityEvaluationContext(mock(Authentication.class), mock(MethodInvocation.class), paramNameDiscoverer);

Member

rwinch Dec 20, 2018

We don't use all caps unless it is a final member variable. You could rename this to something like Context

.../springframework/security/access/expression/method/MethodSecurityEvaluationContextTests.java Outdated

+              		when(paramNameDiscoverer.getParameterNames(method)).thenReturn( new String[] {null});
+              		NotNullVariableMethodSecurityEvaluationContext NNVMSEC= new NotNullVariableMethodSecurityEvaluationContext(mock(Authentication.class), mock(MethodInvocation.class), paramNameDiscoverer);
+              		try {

Member

rwinch Dec 20, 2018

There is no need need for this try/catch since it would fail with an uncaught exception

.../springframework/security/access/expression/method/MethodSecurityEvaluationContextTests.java Outdated

+               * @author shabarijonnalagadda
+               *
+               */
+              public class MethodSecurityEvaluationContextTests{

Member

rwinch Dec 20, 2018

Try to pay attention to whitespace (i.e. add a space so it isMethodSecurityEvaluationContextTests {)

.../springframework/security/access/expression/method/MethodSecurityEvaluationContextTests.java Outdated

+              	@Mock
+              	private Method method;
+              	public  static class  NotNullVariableMethodSecurityEvaluationContext extends MethodSecurityEvaluationContext {

Member

rwinch Dec 20, 2018

Just a few style guides to be consistent with our code base.

Place the static class below the method that uses it
Do not make the class public (you can make it private or package private)
Fix the whitespace (there are extra spaces in the declaration above)

.../springframework/security/access/expression/method/MethodSecurityEvaluationContextTests.java Outdated Show resolved Hide resolved

shabari004 added 3 commits

December 20, 2018 14:41


          MethodSecurityEvaluationContext should check for parameters Name are …

78002b6

…not null before calling setVariable

Fixes: 17565

adding Junit test MethodSecurityEvaluationContext setVariable
Fixes: spring-projectsgh-6223

Update MethodSecurityEvaluationContextTests.java

changing the code to be consistent with the existing code base.


          MethodSecurityEvaluationContextTests is the JUnit test for checking M…

59497d3

…ethodSecurityEvaluationContext should check Parameter Names are not null

Fixes:17565


          Merge branch 'gh-17565-npe' of https://github.com/shabari004/spring-s…

c850644

…ecurity into gh-17565-npe

Member

rwinch commented Dec 21, 2018

Thanks for the updates. It appears we now have a few problems:

There are test failures. You can run ./gradlew check to find the errors
The branch needs to be rebased off of master due to the conflicts
The commits need to be squashed

Author

shabari004 commented Dec 21, 2018

@rwinch i think i lost it, is there anyway somebody can work on this bug. I think i dont have enough knowledge, Thanks for guidance till this point.

Contributor

ankurpathak commented Dec 24, 2018

@rwinch i think i lost it, is there anyway somebody can work on this bug. I think i dont have enough knowledge, Thanks for guidance till this point.

@rwinch @shabari004 I would like to take it forward.

Contributor

ankurpathak commented Dec 24, 2018

Here is new pull request for the issue:
#6332

rwinch removed the status: waiting-for-feedback label

Member

rwinch commented Jan 7, 2019

@shabari004 Thanks for following up. I will proceed with #6332

rwinch closed this

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

Labels

None yet