4:40 p.m.
On 6/27/2013 11:04 AM, Ade Lee wrote:
This adds the initial framework for viewing and managing profiles.
At this point, only the viewing of profiles is tested.
Because I make some changes to some of the objects used in Cert
enrollment, some of the current tests involving enrollment may fail
if they use pre-generated XML.
Still, this patch is getting quite large and its time to get some eyes
on it.
The next patches will add the CLI code to view profiles and then to
add/edit profiles. Following that, will be patches to clean up all the
TODOs - like adding the relevant exceptions and auditing.
Ade
Some comments:
1. The log messages in CATest should say 'Enabled:' and 'Visible:'
because those are the attribute names being logged:
log("Enabled: " + info.isEnabled());
log("Visible: " + info.isVisible() + "\n\n");
2. The following code in CATest can be simplified:
ListIterator<ProfileAttribute> iter =
entry.getValue().getAttrs().listIterator();
while (iter.hasNext()) {
ProfileAttribute attr = iter.next();
into this:
for (ProfileAttribute attr : entry.getValue().getAttrs()) {
Similar code exists in CertEnrollmentRequest, BasicProfile,
CertProcessor, and EnrollmentProcessor.
3. I don't think the ProfileAttribute should be changed to use
IDescriptor instead of Descriptor. Are there other classes implementing
IDescriptor?
The type adapter for IDescriptor is defined as Descriptor.Adapter, but
this adapter will do a simple typecast from IDescriptor to Descriptor
and vice versa, so it probably won't work with other classes. To
properly marshal any IDescriptor into Descriptor we'd have to create a
new Descriptor object and initialize it with values obtained using using
IDescriptor methods. I'm also not sure how unmarshal would be able to
restore the original object if it's not a Descriptor.
I think if there's no other types of descriptor (or if all descriptors
will inherit from Descriptor) then we should just use Descriptor in
ProfileAttribute. But if we need to support different descriptor types,
we need to serialization the class name as well (see PKIException.Data
class) and use a factory to recreate the proper object.
4. In ProfileData the inputs, outputs, and policySets attributes are
declared between the method declarations. To improve readability we
should keep the attributes in the beginning of the class before any
method declarations.
Similar issues also happens in ProfileInput and ProfileOutput.
5. The ProfileData.policySets maps a String to a Vector. I think unless
there's a special need for Vector (e.g. synchronized access) we should
use a more generic and light-weight Collection instead.
6. The following code in BasicProfile deleteAllProfileInputs() and
deleteAllProfileInputs() can be simplified:
Iterator<String> iter = mInputIds.iterator();
while (iter.hasNext()) {
String id = iter.next();
into this:
for (String id : mInputIds) {
7. In ProfileService the listProfiles() is split into listEEProfiles()
and listAdminProfiles() to return different results based on the
visibility flag. Splitting the method doesn't prevent a user from
calling a method that he's not supposed to call. To prevent that we'd
have to configure ACLs, which adds maintenance. Also we probably have to
write different CLI to call different methods for different roles.
Instead, we could use the original method but it should check the roles
of the user and determine the visibility flag based on that.
8. In ProfileService.createProfileDataInfo() the URI is constructed
using String concatenation in 2 different ways:
if (visibleOnly) {
profileBuilder.path(profilePath.value() + "/profiles/" +
profileId);
} else {
profileBuilder.path(profilePath.value() + "/agent/profiles/" +
profileId);
}
ret.setProfileURL(profileBuilder.build().toString());
If we implement #7 it can be simplified like this:
URI uri = profileBuilder.path(ProfileResource.class).path("{id}").
build(profileId);
ret.setProfileURL(uri.toString());
9. Some exceptions in ProfileService are being swallowed. I think at
least for now we should throw an exception (either make the method
throws generic Exception or wrap the exceptions in RuntimeException) so
that we know if there's a problem. Later on we can revisit this and
handle the exceptions properly.
10. The code in ProfileService.populateProfilePolicies() can be
simplified as follows:
for (Map.Entry<String,Vector<ProfilePolicy>> policySet :
data.getPolicySets().entrySet()) {
--
Endi S. Dewata