Skip to content

Add null checks to constructors #6892

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

Closed
jzheaux opened this issue May 20, 2019 · 9 comments
Closed

Add null checks to constructors #6892

jzheaux opened this issue May 20, 2019 · 9 comments
Assignees
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented May 20, 2019

Related to #6542

RequestKey, JaasGrantedAuthority, and SwitchUserGrantedAuthority each anticipate certain final members will be non-null without requiring such in the constructor.

For example, RequestKey does the following in equals:

if (!url.equals(key.url)) {
    return false;
}

Where url is a member variable. But, the constructor does nothing to ensure this behavior will actually work:

public RequestKey(String url, String method) {
    this.url = url;
    this.method = method;
}

For RequestKey, we should change the constructor to something like:

public RequestKey(String url, String method) {
    Assert.notNull(url, "url cannot be null");
    this.url = url;
    this.method = method;
}

And we'd do similar things for the other two classes.

@jzheaux jzheaux added in: core An issue in spring-security-core in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels May 20, 2019
@jzheaux jzheaux added this to the 5.2.0.M3 milestone May 20, 2019
@jzheaux jzheaux self-assigned this May 20, 2019
@jzheaux jzheaux added the status: first-timers-only An issue that can only be worked on by brand new contributors label May 21, 2019
@clementkng
Copy link
Contributor

Hi @jzheaux, is this issue open for contribution? If so, I would like to try working on this since I'm interested in learning more about this project.

@jzheaux
Copy link
Contributor Author

jzheaux commented May 22, 2019

Yes, @clementkng, it certainly is. Thanks for your interest! The ticket is yours.

@jzheaux jzheaux removed the status: first-timers-only An issue that can only be worked on by brand new contributors label May 22, 2019
@dfcoffin
Copy link

@jzheaux is there a list of which class constructors and values should be verified as containing non-null values?

@jzheaux
Copy link
Contributor Author

jzheaux commented May 22, 2019

Thanks for the question, @dfcoffin:

RequestKey -> url
JaasGrantedAuthority -> role, principal
SwitchUserGrantedAuthority -> role, source

@clementkng
Copy link
Contributor

@jzheaux the integration test seems to be failing for me locally even though I am running ./gradlew clean build integrationTest on master with no changes. I'm not sure if this OK behavior, and whether we're supposed to run the above command or ./gradlew build suffices for testing.

@jzheaux
Copy link
Contributor Author

jzheaux commented May 24, 2019

What is the error you're seeing? Note that integrationTest on the full suite fails for JDK 11 on some of the samples.

@clementkng
Copy link
Contributor

I'm currently running the project on the Windows Subsystem for Linux using JDK 10. Just wanted to make sure it's unexpected behavior before diving in. I'm getting a selenium.NoSuchElementException and a junit.ComparisonFailure.

image

@clementkng
Copy link
Contributor

@jzheaux I've created a PR for the issue here

clementkng added a commit to clementkng/spring-security that referenced this issue May 29, 2019
RequestKey, JaasGrantedAuthority, and SwitchUserGrantedAuthority
assume certain final members are non-null.

Issue: spring-projectsgh-6892
jzheaux pushed a commit that referenced this issue May 29, 2019
RequestKey, JaasGrantedAuthority, and SwitchUserGrantedAuthority
assume certain final members are non-null.

Issue: gh-6892
@jzheaux
Copy link
Contributor Author

jzheaux commented Jun 12, 2019

Fixed via e66369f

@jzheaux jzheaux closed this as completed Jun 12, 2019
@rwinch rwinch removed the in: web An issue in web modules (web, webmvc) label Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants