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

[Ade Lee]

Endi. 

Comments:
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.

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)

Along the same lines, the JAXB and DAO classes should reside under
com.netscape.cms.servlet.admin.model.

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

The UserCertResourceService and UserResourceService classes should
reside under com.netscape.cms.servlet.admin

CLI should go under com.netscape.cmstools  

2. I like the use of the atom link.  Please open a task to replace the
Links in the other restful services.

3. The valueOf() function in UserData is incorrect.  Should return
UserData.

4. The getUserMessage() functions are perhaps not needed.  If anything,
they should be written as a single vargs method.  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.

Then you can call the regular CMS.getUserMessage(locale, ....) without
the getUserMessage() 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.

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.

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.

8. The code in stripBrackets() and getEncodedCert() are used in many
places in the code.  After your changes, do all these still work?

Ade

 



 
On Mon, 2012-05-21 at 10:28 -0500, Endi Sukma Dewata wrote:
> The user REST service provides an interface to manage users and user
> certificates. It is based on UsrGrpAdminServlet.
> 
> Ticket #160
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel