[bugfix] guard against NPE in securitymanager #5158
Conversation
|
@reinhapa unfortunately, our fix for the IdFunctionTest only worked on my local machine. |
| if (!sameUserWithSameGroups(context.getRealUser(), context.getEffectiveUser())) { | ||
| final Subject effectiveUser = context.getEffectiveUser(); | ||
| if (effectiveUser != null && ( | ||
| realUser == null || !sameUserWithSameGroups(realUser, effectiveUser))) { |
There was a problem hiding this comment.
realUser will never be null
There was a problem hiding this comment.
It seems to be possible, looking at the initial bug report in #4670
There was a problem hiding this comment.
If context.realUser returns null - then that indicates a serious bug elsewhere that needs to be solved. Changing the code here will only hide that bug and make it much harder to find!
There was a problem hiding this comment.
I would consider actually throwing a RuntimeException and shutting down - as at this point the security of the system is completely compromised
There was a problem hiding this comment.
I'll try that and see if tests still pass
|
|
||
| if (!sameUserWithSameGroups(context.getRealUser(), context.getEffectiveUser())) { | ||
| final Subject effectiveUser = context.getEffectiveUser(); | ||
| if (effectiveUser != null && ( |
There was a problem hiding this comment.
It would be much cleaner to move the null check into the function sameUserWithSameGroups
| subjectToXml(builder, context.getRealUser()); | ||
| builder.endElement(); | ||
| final Subject realUser = context.getRealUser(); | ||
| if (realUser != null) { |
There was a problem hiding this comment.
realUser will never be null
There was a problem hiding this comment.
throw XPathException in this case?
There was a problem hiding this comment.
A null value should not happen but... defensive programming in a complex application is not a too bad idea I'd say.
A log to detect the situation/throw exception ... both would be OK for me.
Move null check into function sameUserWithSameGroups.
|
@reinhapa It turned out that the same fix had to be applied to the other two tests for IdFunction as well. Now they all pass. 🎉 |
|
Description:
Reference:
supersedes #4671
fixes #4670
Type of tests: