8:44 a.m.
I've attached a patch that addresses most of the comments below and also
also adds CLI and fixes adding/removing/editing and changing the state
of profiles.
As we discussed on #irc, the authentication problem is still unresolved.
That is, I want to be able to do the following:
GET /profiles and then in the method itself determine whether or not I
have already logged in (I will have a principal) and return different
results accordingly.
Right now, that is not working. The only way that I can guarantee that
client auth takes place and the credential is provided is by putting in
a security constraint that requires /profiles/* to use client
authentication. But then, I cannot do GET /profiles without
authentication. It seems clientAuth=want is not working.
If we cannot resolve this issue, then I will need to keep using agent/*
and admin/*, which are url patterns that have security contexts. And I
will have to add back GET /agent/profiles and GET /agent/profiles/{id}
to see all profiles that are not visible to the EE user.
Thanks,
Ade
On Thu, 2013-06-27 at 16:40 -0500, Endi Sukma Dewata wrote:
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()) {