On 11/20/2014 12:58 AM, Fraser Tweedale wrote: >Ping. Who wants to review this :) Looks short enough, I'll bite. :) The changes seem to be fine, so it's ACKed.

There are some related issues/questions. Feel free to address them as part of this ticket, or maybe file a separate ticket. 1. The exception message is not very helpful. It probably should have said something like: Max path length cannot be smaller than min path length: <maxLen value>

2. According to the ticket the ca-profile-add failed but the profile actually got added. Suppose there's a real constraint violation I think it should display the above exception message, and the profile should not be added. 3. Can we specify any negative value to indicate no min/max, or does it have to be -1? It should be consistent with the documentation.

4. The code seems to assume that the MinPathLen is already added before adding MaxPathLen. Suppose the MinPathLen is missing or added later will this constraint still be validated?

Also, maybe we should keep track the list of unreviewed patches on the top of etherpad so we don't miss anything.

-- Endi S. Dewata

New patch attached. Re point 1., despite the improved ``EPropertyException`` thrown in the new code, the error reported by the CLI is ``PKIException: Error changing profile data``. I filed https://fedorahosted.org/pki/ticket/1212 to address this later. 3. is not an issue and 2. and 4. have been addressed in the new patch.

On 11/24/2014 12:03 AM, Fraser Tweedale wrote: >New patch attached. > >Re point 1., despite the improved ``EPropertyException`` thrown in >the new code, the error reported by the CLI is ``PKIException: Error >changing profile data``. I filed >https://fedorahosted.org/pki/ticket/1212 to address this later. > >3. is not an issue and 2. and 4. have been addressed in the new >patch. I'm not sure if we should do this: String origValue = getConfig(name); cs.putString(name, value); try { validateConfig(); } catch (EPropertyException e) { cs.putString(name, origValue); throw e; } In case validateConfig() throws a different exception, the origValue may not be restored, so the new (possibly invalid) value will remain in the config store. Another thing, in the following code suppose the CONFIG_MIN_PATH_LEN contains an invalid value, the CONFIG_MAX_PATH_LEN will not be read: int minLen = -1; int maxLen = -1; try { minLen = getConfigInt(CONFIG_MIN_PATH_LEN); maxLen = getConfigInt(CONFIG_MAX_PATH_LEN); } catch (NumberFormatException e) { // one of the path len configs is empty - treat as unset } It may not make a difference now since the validation is only done if both variables are not negative, but in case the validation code is expanded to check for other things, the maxLen might not have the right value. Also, I think the original code would throw an error if the value is not a valid integer while here it's swallowed. I think the code should either validate each parameter as it gets added, or validate all parameters after all parameters get added. So in this case the setConfig() might look like this: validateParam(name, value); cs.putString(name, value); and the validateParam() might look like this: // if the new param is minLen if (name == CONFIG_MIN_PATH_LEN) { // if minLen is unset, return if (value is empty) return; // validate minLen is an integer int minLen = getInt(value); // if minLen is negative, return if (minLen < 0) return; // if maxLen is unset, return String maxLenString = getConfig(CONFIG_MAX_PATH_LEN); if (maxLenString is empty) return; // if maxLen is negative, return int maxLen = getInt(maxLenString); if (maxLen < 0) return; // validate minLen is less than maxLen if (minLen >= maxLen) throw exception } Same thing for maxLen. So the code is a bit longer, but I'm not sure it can be simplified without compromising the validation. By the way, this is a separate issue and feel free to open a ticket. I think the validation should check for minLen > maxLen instead of >=. This way the minLen can be equal to maxLen, meaning the parameter has to be a specific length. -- Endi S. Dewata

On 11/26/2014 1:02 PM, Fraser Tweedale wrote: >v3 patch. > >In this patch, validateConfig() is short-circuited until all params >have been set, to avoid repetition of logic in the validity check. > >Also, a config that causes validation to fail is now reverted on any >exception, not just EPropertyException. > >Finally, the profile still gets created when there is bad config so >I filed another ticket for that: >https://fedorahosted.org/pki/ticket/1215 This is getting much more complicated than I anticipated :) The patch still has some issues: 1. To ensure we revert all invalid config changes, the code should catch Throwable instead of Exception when calling the validateConfig(), but see below. 2. I'm not sure if allConfigsSet() is the right way to do this. Let's say we add a config with an invalid value, it may actually accept the invalid value because the validation only happens after all configs are set. If a validation error happens later, it will only revert the last config set, not the config with the invalid value. Let's say the profile doesn't specify all defined configs, the validation may never happen. It's also kind of inefficient because it's iterating through all defined configs on each config set. I think ideally the validation should happen after the loop that calls setConfig(), for example: for (String name : names) { String value = ... constraint.setConfig(name, value); } constraint.validateConfig(); We may not even need to revert the invalid config if the code discards the constraint object. If you'd like, feel free to push the original patch since it's already ACKed. We can improve the constraint config validation as a separate ticket.