GEODE-3974: function security improvement#1287
Conversation
jinmeiliao
commented
Jan 16, 2018
jinmeiliao
commented
- function executed on a local member does not log out user accidentally
- Mark some internal functions as InternalEntity
- test refactor
jinmeiliao
requested review from
PurelyApplied,
jdeppe-pivotal,
pdxrunner and
sboorlagadda
jinmeiliao
force-pushed
the
function2
branch
3 times, most recently
from
f8c62c8 to
d7efedf
Compare
PurelyApplied
left a comment
PurelyApplied
left a comment
There was a problem hiding this comment.
Only the comment surrounding the securityService.logout is pressing.
| String[] functionArgs = null; | ||
| Object[] args = context.getArguments(); | ||
| if (args == null) { | ||
| context.getResultSender().lastResult(new CliFunctionResult(member.getId(), false, |
There was a problem hiding this comment.
Would sendException be preferable to lastResult here?
There was a problem hiding this comment.
I would lean to not messing with the previous behavior on this front for now.
| loginNeeded = subject == null || !subject.isAuthenticated(); | ||
| } catch (AuthenticationRequiredException e) { | ||
| loginNeeded = true; | ||
| } |
There was a problem hiding this comment.
This logic seems like it could belong to the SecurityService, if we think we might be able to reuse it in the future.
There was a problem hiding this comment.
let's see how it would get reused before we make it a public facing api.
| .lastResult(new CliFunctionResult(member.getId(), false, "Exception: " + e.getMessage())); | ||
| } finally { | ||
| if (loginNeeded) { | ||
| securityService.logout(); |
There was a problem hiding this comment.
This could get weird if the securityService.login (line 84) fails. A new boolean indicating successful login might be a better gate.
81:
boolean loginSuccessful = false;
try{
if (loginNeeded) {
securityService.login(credentials);
loginSuccessful = true;
}
...
} finally {
if (loginSuccessful) {
securityService.logout();
}
}
Alternatively, if the block in my other comment was a method on the SecurityService, we might be able to just check that here: if (!securityService.loginRequired()){. Although I'm not sure I like how that reads, so maybe not.
| "execute function --region=" + regionName + " --id=" + thisFunction.getId()) | ||
| .containsOutput("not authorized for " + thisMissingPermission.toString()).statusIsError(); | ||
| @ConnectionConfiguration(user = "user", password = "user") | ||
| public void functionRequireExpectedPermission() throws Exception { |
There was a problem hiding this comment.
I understand that the testing of the security service itself is rather beyond the scope of this test class, but do we have any positive-tests for permissions with region-level specificity? ExecuteFunctionCommandSecurityTest and ExecuteFunctionCommandWitSecurityDUnitTest appear to stop at the second tier, i.e., clusterManage and dataRead.
I think we need the positive-test coverage that these testValid* tests have, even if they belong in a different class / ticket.
There was a problem hiding this comment.
We have plenty of unit tests that tests implies to target level. I will add a few more here if needed.
* function executed on a local member does not log out user accidentally * Mark some internal functions as InternalEntity * test refactor
3 participants
