[WFCORE-4395] Move the JDBCRealm Mapper validation to model time check #6172
Conversation
|
|
||
| return keyMapper; | ||
| } | ||
|
|
There was a problem hiding this comment.
I added the Mapper validation, but I don't know if we still need to have this code on we can just remove the InvalidKeyException from the interface, and call this without those validations (both if). I guess that we can keep the throw new OperationFailedException("Invalid key type.", e); only at the checkKeyMappers. Or do we need those validations on Runtime time as well?
There was a problem hiding this comment.
Can you move this method back where it was? AFAICT it doesn't need to move, and moving it makes this diff very hard to read.
There was a problem hiding this comment.
Unless @fjuma disagrees, +1 to getting rid of 'throws InvalidKeyException'. None of the impls throw it.
If we keep it we should fix the i18 of this OFE. Exceptions should not have hard coded English messages.
There was a problem hiding this comment.
+1 for getting rid of the throws InvalidKeyException.
|
This is the error when try to startup with more than one mapper: |
| } | ||
|
|
||
| try { | ||
| keyMapper = mapperResource.toPasswordKeyMapper(context, propertyNode); |
There was a problem hiding this comment.
This is not allowed. The toPasswordKeyMapper implementations do work that is not meant for Stage.MODEL (all the resolveModelAttribute calls).
There was a problem hiding this comment.
That said, my impression reading this is that it's not necessary to call this in Stage.MODEL. This entire try/catch can go away. Looking (quickly) at all the impls of this interface, none of them do anything that is relevant to Stage.MODEL.
There's separate logic called in populateModel that validates the individual mapper configs, so the various things done in the toPasswordKeyMapper impls is not a necessary part of the Stage.MODEL validation. All that's needed here is checking that there is only config element.
pedro-hos
force-pushed
the
WFCORE-4395
branch
from
ede2e24 to
b0340bb
Compare
|
@bstansberry I have made the changes to check that we have only one mapper by |
pedro-hos
force-pushed
the
WFCORE-4395
branch
from
b0340bb to
17e3306
Compare
|
@bstansberry / @fjuma I have made the changes. Please, let me know if I need to make any other improvements. Thanks |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
|
||
| @Override | ||
| public void execute(OperationContext context, ModelNode operation) throws OperationFailedException { | ||
| List<ModelNode> principalQueries = PrincipalQueryAttributes.PRINCIPAL_QUERIES_7_0.resolveModelAttribute(context, operation).asList(); |
There was a problem hiding this comment.
@pedro-hos Unfortunately 'resolveModelAttribute' shouldn't be called in Stage.MODEL. Expressions meant to be resolved from secure sources (i.e. Elytron credential stores) can't be resolved in MODEL. We don't ban their use in MODEL (i.e. throw an exception) because there are a couple valid cases, but this isn't one. (The odds of such an expression being used here are probably minuscule, but we shouldn't rely on that.)
I think the thing to do is:
ModelNode model = context.readResource(PathAddress.EMPTY_ADDRESS, false).getModel();
ModelNode queries = model.get(PrincipalQueryAttributes.PRINCIPAL_QUERIES_7_0.getName());
And then inspect 'queries'.
The 'ModelNode model = context.readResource(PathAddress.EMPTY_ADDRESS, false).getModel();' is because by the time this runs the 'operation' contents have already been processed and stored into the model. It is better to use the results of that work (which may have somehow transformed things) that to rely on the original operation.
There was a problem hiding this comment.
Sure, I have made the changes. Thank you for the feedback
pedro-hos
force-pushed
the
WFCORE-4395
branch
from
17e3306 to
04f9193
Compare
|
Thank you |
yersan
added
the
ready-for-merge
|
Thanks @fjuma @pedro-hos @bstansberry |
https://issues.redhat.com/browse/WFCORE-4395