[Pki-devel] [PATCH]136 - initial framework for restful interfaces for managing profiles
Ade Lee
alee at redhat.com
Fri Jul 19 13:44:53 UTC 2013
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()) {
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-vakwetu-0136-1-Add-interfaces-for-managing-profiles.patch
Type: text/x-patch
Size: 112888 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20130719/17336b99/attachment.bin>
More information about the Pki-devel
mailing list