Skip to content

InMemoryReactiveClientRegistrationRepository should not use ConcurrentReferenceHashMap #7299

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 Aug 22, 2019 · 5 comments · Fixed by #7308
Closed
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Aug 22, 2019

ConcurrentReferenceHashMap is a cache-style map that uses weak references.

Since InMemoryReactiveClientRegistrationRepository is intended to be persistent, it should instead use ConcurrentHashMap.

The change to be made is in the InMemoryReactiveClientRegistrationRepository constructor that instantiates a ConcurrentReferenceHashMap:

Assert.notEmpty(registrations, "registrations cannot be empty");
this.clientIdToClientRegistration = new ConcurrentReferenceHashMap<>(); // <-- this line
for (ClientRegistration registration : registrations) {
	Assert.notNull(registration, "registrations cannot contain null values");
	this.clientIdToClientRegistration.put(registration.getRegistrationId(), registration);
}

should instead be

Assert.notEmpty(registrations, "registrations cannot be empty");
this.clientIdToClientRegistration = new ConcurrentHashMap<>();  // <-- this line
for (ClientRegistration registration : registrations) {
	Assert.notNull(registration, "registrations cannot contain null values");
	this.clientIdToClientRegistration.put(registration.getRegistrationId(), registration);
}

This ticket should also be backported to 5.1.x.

@jzheaux jzheaux added type: bug A general bug in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors labels Aug 22, 2019
@jzheaux jzheaux added this to the 5.2.0.RC1 milestone Aug 22, 2019
@eberttc
Copy link
Contributor

eberttc commented Aug 23, 2019

I can make that change.

@jzheaux
Copy link
Contributor Author

jzheaux commented Aug 23, 2019

Sounds great, @eberttc, it's yours!

@jzheaux jzheaux self-assigned this Aug 23, 2019
@jzheaux jzheaux removed the status: first-timers-only An issue that can only be worked on by brand new contributors label Aug 23, 2019
@eberttc
Copy link
Contributor

eberttc commented Aug 23, 2019

I'm getting this error at pushing:

remote: Permission to spring-projects/spring-security.git denied to eberttc.
fatal: unable to access 'https://github.com/spring-projects/spring-security.git/': The requested URL returned error: 403

Do I need permission in the project?

@jzheaux
Copy link
Contributor Author

jzheaux commented Aug 23, 2019

Correct, @eberttc, you will need to fork the repository first. Check out this GitHub Guide about forking.

The basic idea is that you fork the repository and that fork is yours - you commit to that, and then form a pull request. It may seem like a lot for just a little change like this, but having it this way will assist you with doing more sophisticated PRs down the road.

@eberttc
Copy link
Contributor

eberttc commented Aug 23, 2019

Thanks for the support @jzheaux .
I will do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants