FtpServer
  1. FtpServer
  2. FTPSERVER-181

The UserManager#authenticate() method lacks of documentation

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0-M3
    • Fix Version/s: 1.0.0-M4
    • Component/s: Ftplets
    • Labels:
      None

      Description

      The UserManager interface documentation should be a form of contract between the implementor and the user of the interface.
      After reading the documentation the implementor still has these question:

      • when I should throw a AuthenticationFailedException?
      • when I should return a null as a result?
      • if the object receive two subsequent calls withe the same arguments the objects returned should be the same (same reference), only equals, or they can be different?

        Activity

        Hide
        David Latorre added a comment -

        You can check the current implementations of UserManager to solve these questions so I don't know if this should be "major priority". Anyway, If you provided better documentation Im sure Niklas will be glad to add it.

        My understanding is that you're only trying to point a deficiency in the documentation and not that you don't know the answers to those questions yourself. Otherwise, feel free to ask - the mailing list is too quiet anyway.

        Show
        David Latorre added a comment - You can check the current implementations of UserManager to solve these questions so I don't know if this should be "major priority". Anyway, If you provided better documentation Im sure Niklas will be glad to add it. My understanding is that you're only trying to point a deficiency in the documentation and not that you don't know the answers to those questions yourself. Otherwise, feel free to ask - the mailing list is too quiet anyway.
        Hide
        Niklas Gustavsson added a comment -

        Patches are of course welcome, if not, a general overhaul of the Javadocs is planned before 1.0.

        Show
        Niklas Gustavsson added a comment - Patches are of course welcome, if not, a general overhaul of the Javadocs is planned before 1.0.
        Hide
        Andrea Francia added a comment -

        @David
        Thanks for the hints about searching in the source code. I done so but I'm not still sure about when it should return null.
        I can provide a patch with my understanding of the method but I'm not sure that my idea is the same that the orginal designer had.
        Maybe there some instruction how to create and submit patches.

        In general I think that the documentation of the interfaces (and the interfaces API designing) are really a important issues.
        A library become useful when its API is usable. I don't know if this apply to yours project.

        I'll subscribe the mailling list anyway.

        Please put my name (Andrea Francia) but not my email address somewhere in the credits if you want to use my contribution.

        /**

        • Authenticate user (or not).
        • This method shall:<ul>
        • <li>return a not null {@see User}

          object reference if the

        • authentication succeeds</li>
        • <li>throws a {@see AuthenticationFailedException}

          if the authentication

        • fails</li>
        • </ul>
          *
        • @param authentication could be:<ul>
        • <li> {@see org.apache.ftpserver.usermanager.AnonymousAuthentication}

          :

        • in the case of anonymous authentication</li>
        • <li> {@see org.apache.ftpserver.usermanager.UsernamePasswordAuthentication}

          :

        • in the case of a normal authentication</li>
        • </ul>
          *
        • @throws AuthenticationFailedException if the authentications fails (e.g.
        • wrong password, unexistent username, anonymous authentication
        • not accepted).
        • @return the authenticated user (must be not null)
          */
          User authenticate(Authentication authentication)
          throws AuthenticationFailedException;
        Show
        Andrea Francia added a comment - @David Thanks for the hints about searching in the source code. I done so but I'm not still sure about when it should return null. I can provide a patch with my understanding of the method but I'm not sure that my idea is the same that the orginal designer had. Maybe there some instruction how to create and submit patches. In general I think that the documentation of the interfaces (and the interfaces API designing) are really a important issues. A library become useful when its API is usable. I don't know if this apply to yours project. I'll subscribe the mailling list anyway. Please put my name (Andrea Francia) but not my email address somewhere in the credits if you want to use my contribution. /** Authenticate user (or not). This method shall:<ul> <li>return a not null {@see User} object reference if the authentication succeeds</li> <li>throws a {@see AuthenticationFailedException} if the authentication fails</li> </ul> * @param authentication could be:<ul> <li> {@see org.apache.ftpserver.usermanager.AnonymousAuthentication} : in the case of anonymous authentication</li> <li> {@see org.apache.ftpserver.usermanager.UsernamePasswordAuthentication} : in the case of a normal authentication</li> </ul> * @throws AuthenticationFailedException if the authentications fails (e.g. wrong password, unexistent username, anonymous authentication not accepted). @return the authenticated user (must be not null) */ User authenticate(Authentication authentication) throws AuthenticationFailedException;
        Hide
        Niklas Gustavsson added a comment -

        Thanks!

        This page describes how to contribute, note that it is important that if you provide a patch that you check the radio button to allow your patch for inclusion in ASF software.
        http://apache.org/dev/contributors.html#patches

        We do not normally put contributor names in source files, but I'll happily list you as a contributor in our POM (I'll make sure to not include your email address). Could you please provide the above improvment as patch and add it as an attachment to this issue and I'll commit it.

        Again, thanks for your work!

        Show
        Niklas Gustavsson added a comment - Thanks! This page describes how to contribute, note that it is important that if you provide a patch that you check the radio button to allow your patch for inclusion in ASF software. http://apache.org/dev/contributors.html#patches We do not normally put contributor names in source files, but I'll happily list you as a contributor in our POM (I'll make sure to not include your email address). Could you please provide the above improvment as patch and add it as an attachment to this issue and I'll commit it. Again, thanks for your work!
        Hide
        Niklas Gustavsson added a comment -

        Patch provided by Andrea Francia on the mailing list, with approval for inclusion in the code:

        Index: core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java
        ===================================================================
        — core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java (revision 701896)
        +++ core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java (working copy)
        @@ -47,6 +47,7 @@
        *

        • @see junit.framework.TestCase#setUp()
          */
          + @Override
          protected void setUp() throws Exception { userManager = createUserManager(); }

          @@ -86,7 +87,7 @@
          .authenticate(new UsernamePasswordAuthentication("user3", null)));
          }

        • public static class FooAuthentication implements Authentication {
          + private static class FooAuthentication implements Authentication {
          }

        public void testAuthenticateNullUser() throws Exception

        { Index: ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java =================================================================== --- ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java (revision 701896) +++ ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java (working copy) @@ -29,48 +29,69 @@ /** * Get user by name. + * + * @param username the name to search for. + * @throws FtpException when the UserManager can't fulfill the request. + * @return the user with the specified name, or null if a such user does + * not exist. */ - User getUserByName(String login) throws FtpException; + User getUserByName(String username) throws FtpException; /** * Get all user names in the system. + * + * @throws FtpException when the UserManager can't fulfill the request. + * @return an array of username strings, note that the result should never + * be null, if there is no users the result is an empty array. */ String[] getAllUserNames() throws FtpException; /** * Delete the user from the system. * + * @throws FtpException when the UserManager can't fulfill the request. * @throws UnsupportedOperationException * if UserManager in read-only mode */ - void delete(String login) throws FtpException; + void delete(String username) throws FtpException; /** * Save user. If a new user, create it else update the existing user. - * + * + * @param user the Uset to save + * @throws FtpException when the UserManager can't fulfill the request. * @throws UnsupportedOperationException * if UserManager in read-only mode */ void save(User user) throws FtpException; /** - * User existance check. + * Check if the user exists. + * @param username the name of the user to check. + * @return true if the user exist, false otherwise. */ - boolean doesExist(String login) throws FtpException; + boolean doesExist(String username) throws FtpException; /** * Authenticate user + * @throws FtpException when the UserManager can't fulfill the request. + * @param authentication + * @return the autheticated account. */ User authenticate(Authentication authentication) throws AuthenticationFailedException; /** * Get admin user name + * @return the admin user name + * @throws FtpException when the UserManager can't fulfill the request. */ String getAdminName() throws FtpException; /** + * Check if the user is admin. * @return true if user with this login is administrator + * @throws FtpException when the UserManager can't fulfill the request. */ - boolean isAdmin(String login) throws FtpException; + boolean isAdmin(String username) throws FtpException; }

        Index: pom.xml
        ===================================================================
        — pom.xml (revision 701896)
        +++ pom.xml (working copy)
        @@ -120,6 +120,10 @@
        <contributor>
        <name>Nick Vincent</name>
        </contributor>
        + <contributor>
        + <name>Andrea Francia</name>
        + <url>http://andreafrancia.blogspot.com</url>
        + </contributor>
        </contributors>

        <scm>

        Show
        Niklas Gustavsson added a comment - Patch provided by Andrea Francia on the mailing list, with approval for inclusion in the code: Index: core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java =================================================================== — core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java (revision 701896) +++ core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java (working copy) @@ -47,6 +47,7 @@ * @see junit.framework.TestCase#setUp() */ + @Override protected void setUp() throws Exception { userManager = createUserManager(); } @@ -86,7 +87,7 @@ .authenticate(new UsernamePasswordAuthentication("user3", null))); } public static class FooAuthentication implements Authentication { + private static class FooAuthentication implements Authentication { } public void testAuthenticateNullUser() throws Exception { Index: ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java =================================================================== --- ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java (revision 701896) +++ ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java (working copy) @@ -29,48 +29,69 @@ /** * Get user by name. + * + * @param username the name to search for. + * @throws FtpException when the UserManager can't fulfill the request. + * @return the user with the specified name, or null if a such user does + * not exist. */ - User getUserByName(String login) throws FtpException; + User getUserByName(String username) throws FtpException; /** * Get all user names in the system. + * + * @throws FtpException when the UserManager can't fulfill the request. + * @return an array of username strings, note that the result should never + * be null, if there is no users the result is an empty array. */ String[] getAllUserNames() throws FtpException; /** * Delete the user from the system. * + * @throws FtpException when the UserManager can't fulfill the request. * @throws UnsupportedOperationException * if UserManager in read-only mode */ - void delete(String login) throws FtpException; + void delete(String username) throws FtpException; /** * Save user. If a new user, create it else update the existing user. - * + * + * @param user the Uset to save + * @throws FtpException when the UserManager can't fulfill the request. * @throws UnsupportedOperationException * if UserManager in read-only mode */ void save(User user) throws FtpException; /** - * User existance check. + * Check if the user exists. + * @param username the name of the user to check. + * @return true if the user exist, false otherwise. */ - boolean doesExist(String login) throws FtpException; + boolean doesExist(String username) throws FtpException; /** * Authenticate user + * @throws FtpException when the UserManager can't fulfill the request. + * @param authentication + * @return the autheticated account. */ User authenticate(Authentication authentication) throws AuthenticationFailedException; /** * Get admin user name + * @return the admin user name + * @throws FtpException when the UserManager can't fulfill the request. */ String getAdminName() throws FtpException; /** + * Check if the user is admin. * @return true if user with this login is administrator + * @throws FtpException when the UserManager can't fulfill the request. */ - boolean isAdmin(String login) throws FtpException; + boolean isAdmin(String username) throws FtpException; } Index: pom.xml =================================================================== — pom.xml (revision 701896) +++ pom.xml (working copy) @@ -120,6 +120,10 @@ <contributor> <name>Nick Vincent</name> </contributor> + <contributor> + <name>Andrea Francia</name> + <url> http://andreafrancia.blogspot.com </url> + </contributor> </contributors> <scm>
        Hide
        Niklas Gustavsson added a comment -

        Patch applied

        svn -m "Improved UserManager JavaDoc. Patch by Andrea Francia (FTPSERVER-181)" commit
        Sending core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java
        Sending ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java
        Sending pom.xml
        Transmitting file data ...
        Committed revision 702282.

        Show
        Niklas Gustavsson added a comment - Patch applied svn -m "Improved UserManager JavaDoc. Patch by Andrea Francia ( FTPSERVER-181 )" commit Sending core/src/test/java/org/apache/ftpserver/usermanager/UserManagerTestTemplate.java Sending ftplet-api/src/main/java/org/apache/ftpserver/ftplet/UserManager.java Sending pom.xml Transmitting file data ... Committed revision 702282.

          People

          • Assignee:
            Niklas Gustavsson
            Reporter:
            Andrea Francia
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development