[Pki-devel] [PATCH] 60 Added user REST service.

[Ade Lee]

On Tue, 2012-05-29 at 16:37 -0500, Endi Sukma Dewata wrote:
> New patch attached.
> 
> On 5/24/2012 2:00 PM, Ade Lee wrote:
> > 1. First of all, the location of the new classes.  Earlier in the
> > project, we made the decision to integrate the new RESTful classes
> > within the existing servlet structure.  This is why the Profile RESTful
> > classes show up under com.netscape.cms.servlet.profile and the requests
> > (key and cert) show up under com.netscape.cms.servlet.request.
> 
> The current packaging structure is no longer adequate for REST client 
> and CLI. The packages are supposed to be reorganized later in ticket 
> #114. As mentioned in the last meeting, I'm going to write a proposal 
> about this on a wiki page.
> 
> Here are the package names that I used in the original patch:
> - com.netscape.cms.user (common classes: REST interface & data model)
> - com.netscape.cms.user.client (client classes: REST client & CLI)
> - com.netscape.cms.user.server (server classes: REST services)
> 
> After some more consideration, I tend to go with a slightly different 
> structure which will be simpler to build and package into jar files:
> - com.netscape.cms.user
> - com.netscape.cms.client.user
> - com.netscape.cms.server.user
> 
> > In this case, if we are following the existing structure and being
> > consistent with the other RESTful servlets, we expect the REST servlets
> > for users and groups to show up where the UsrGroupAdminServlet resides -
> > which is under - com.netscape.cms.servlet.admin (like the
> > SystemCertificateResource)
> 
> This is OK except that the REST services aren't servlets even though 
> they might be running inside the RESTEasy servlet. The REST services 
> don't implement the Servlet interface and they don't follow servlet's 
> life cycle. So having a "servlet" in the package name is inaccurate. I 
> would suggest that we rename it to "server" because these classes (both 
> REST services and servlets) are only used by the server. So the 
> com.netscape.cms.servlet is actually identical with 
> com.netscape.cms.server that I'm proposing.
> 
> > Along the same lines, the JAXB and DAO classes should reside under
> > com.netscape.cms.servlet.admin.model.
> 
> This would not be correct because the current "cms/servlet" package is 
> intended for server only. The data model classes are used for data 
> transfer, so they should be shared by both client and server. Right now 
> we don't have a "common" package (the jar file, not the folder). The 
> closest thing is "certsrv", but this package is intended to store server 
> plugin interfaces which the client would never use. The "certsrv" might 
> also have some dependencies that should not be required on the client side.
> 
> > After looking at the package layout doc, it appears that both your
> > structure and the structure of the other new servlets is wrong.  Rather,
> > in this particular case, the interfaces (UserResource.java and
> > UserCertResource.java as well as the JAXB classes UserData.java et. al.
> > should reside under com.netscape.certsrv.usrgrp
> 
> Same as above, they should belong to the "common" package, but "certsrv" 
> is the closest thing that's available now, so for now I'll put it there.
> 
> > The UserCertResourceService and UserResourceService classes should
> > reside under com.netscape.cms.servlet.admin
> 
> This is not a big issue, but "admin" package contains a mix of servlets, 
> some of which are subsystem-specific. It would be better to put them in 
> separate Java packages (e.g. cms.server.user) to simplify packaging.
> 
We appear to have come full circle.  When I originally created the first
drm servlets, I put them under their own package separate from the old
"servlets" because they did not follow the servlet lifecycle - other
than going through the RESTEasy servlets.  Based on review discussions,
we decided then to move them to be integrated with the existing code
structure.

We need to rethink how all of this is packaged, keeping in mind some
sort of continuity between the old and new interfaces so that it is easy
to maintain.

So, lets go ahead and check in for now, and then rearrange as makes
sense once we have carefully considered the packaging.

> > CLI should go under com.netscape.cmstools
> 
> This doesn't work because the REST interfaces and the data models are 
> stored in "certsrv" or "cms", and the "cmstools" is compiled before 
> them. So if we put the CLI in "cmstools" there will be a dependency 
> issue. I couldn't put it in "certsrv" either because the CLI depends on 
> the CMSRestClient which is in "cms" and the "certsrv" isn't suppose to 
> depend on "cms".
> 
> We could move CMSRestClient to "certsrv" too, but the CLI should really 
> belong to a new "client" package. For now I'm going to leave the CLI in 
> com.netscape.cms.client.user under the "cms" package. We can move this 
> again if necessary in ticket #114.

agreed.
> > 2. I like the use of the atom link.  Please open a task to replace the
> > Links in the other restful services.
> 
> The Atom links are now still created manually. We should utilize link 
> injection. Ticket #192.
> 
> > 3. The valueOf() function in UserData is incorrect.  Should return
> > UserData.
> 
> Fixed.
> 
> > 4. The getUserMessage() functions are perhaps not needed.  If anything,
> > they should be written as a single vargs method.
> 
> Changed to use varargs. This method is used to simplify the code, compare:
> - old: CMS.getUserMessage(getLocale(), "CMS_ADMIN_SRVLT_NULL_RS_ID")
> - new: getUserMessage("CMS_ADMIN_SRVLT_NULL_RS_ID")
> 
> > But a better way to do
> > this would be to specify locale the local as a protected variable in
> > CMSResourceService.java.  Then getLocale() would return this variable if
> > it is non-null, or would find it from the headers.
> 
> You mean caching the value returned by getLocale()? That can be done, 
> but the "locale" variable will be null until we call getLocale() at 
> least once.
> 
> > Then you can call the regular CMS.getUserMessage(locale, ....) without
> > the getUserMessage() calls.
> 
> For this to work we'd have to set the "locale" first, so we need to call 
> getLocale() somewhere. I'm not sure if calling it in the 
> CMSResourceService constructor will work because the "headers" is 
> injected, which I suppose can only happen after the object is 
> constructed. We can call it in the beginning of each REST method, but 
> that's not pretty. So for now I'm keeping the getUserMessage().
> 
The problem with getUserMessage() is that you cannot call it from a
non-RESTful servlet context.  That is, you cannot call it from the old
non-RESTful servlets.  This is not important when you have implemented
code that is completely independent of the old code (as in the case of
the Users and Groups), but becomes a problem when you try to create
modules that can be used by both the old and servlets.

Maintaining two completely independent structures of code for the old
servlets and the new ones will quickly become a maintenance nightmare.
And the old servlets will stay around for a long time.  

Here is how I have been approaching this problem with regards to
certificate issuance.
1. Create a class which contains the business logic of the process.  In
my case, I called it ProfileProcessor.  The relevant servlet config and
things like the locale are passed into the constructor of this object.
2. The object makes use of an Auditor, and maybe other classes like an
Authenticator/ Authorizer.  Ultimately, the Authenticator/Authorizer
code will be moved into the Realm.
3.  This class is instantiated by both the RESTful services and the old
non-RESTful services.  In both cases, we need to construct the
ProfileProcessor, call the relevant function(s) and parse the results in
the appropriate way.  

So, while I'm all for getMessage() functions, they need to be done in a
way that can be called from a common class like the ProfileProcessor.
Right now, you have them in a place where only the RESTful servlets can
invoke them.  Maybe, like in the case of the Auditor, we need a Logger
or Utility class which takes the Locale on instantiation, and provides
getMessage() calls.
 

> > Its important we do it this way because when we are writing RESTful
> > servlets for things like cert issuance and revocation, we want to try to
> > reduce code duplication as much as possible.
> >
> > 5. Similar considerations exist for the audit() functions.  We want to
> > use existing functions so that code can be extracted and reused by both
> > restful and restful servlets.
> 
> It's been updated based on the new Auditor patch.
> 
> > 6. Right now, the Delete user operation fails if the user is part of any
> > groups.  As discussed on #irc, we should create a resource
> > at /users/{id}/groups for the client to check for group membership.  And
> > then allow the DELETE to remove users from groups automatically.
> 
> Fixed. The user-group membership will be done in ticket #190.
> 
> > 7. The paging mechanism in findUser() is not really the best - in that
> > you basically get all of the results back from ldap each time.  We
> > really need paged searches instead.  Its unlikely that this will be big
> > concern for users/groups - but we should add a task to do proper paging.
> 
> Right now it's using a very simple, maybe slower, but correct 
> implementation. To do paging properly we will need VLV or simple paged 
> results, which have their own limitations and possibly require sessions 
> (not RESTful). The UGSubsystem doesn't seem to support paging either. 
> How many users/groups do we need to be able to support? If its not large 
> probably the current solution is adequate.
> 
agreed

> > 8. The code in stripBrackets() and getEncodedCert() are used in many
> > places in the code.  After your changes, do all these still work?
> 
> As discussed on IRC, the change in getEncodedCert() fixes the extra 
> blank line in certificates. The blank line got added when we change the 
> Base64 encoder/decoder sometime ago, so it's now back to the correct 
> behavior.
> 
> The change in stripBrackets() has been removed. The 
> UserCertResourceService now normalizes the cert first.
>