[cp-patches] Re: KerberosPrincipal class

[Mark Wielaard]

Hi Jeff,

On Sat, 2005-11-19 at 18:53 -0500, Jeff Bailey wrote:
> Thanks, here goes. =)
> 
> 2005-11-18  Jeff Bailey  <address@hidden>
> 
>         * javax/security/auth/kerberos/KerberosPrincipal.java: New file.
>           (Thanks to Mark Wielaard for reviewing this!)

Sorry I just did I quick syntax check before. Attached is a real
"review" (actually just a quick diff). It is mostly OK, but I would like
to see something done about the FIXMEs I added. Or at least hear your
comments. If anything is unclear please yell and I explain a bit more.
Just wanted to give you some quick feedback.

Thanks,

Mark

--- KerberosPrincipal.java.jeff 2005-11-20 23:31:03.000000000 +0100
+++ KerberosPrincipal.java      2005-11-20 23:43:53.000000000 +0100
@@ -1,8 +1,6 @@
 /* KerberosPrincipal.java -- a single entity in the system.
    Copyright (C) 2005 Free Software Foundation, Inc.
 
-Written by Jeff Bailey <address@hidden>
-   
 This file is part of GNU Classpath.
 
 GNU Classpath is free software; you can redistribute it and/or modify
@@ -40,13 +38,15 @@
 package javax.security.auth.kerberos;
 
 import java.io.Serializable;
+import java.security.Principal;
 import java.util.StringTokenizer;
 
-public final class KerberosPrincipal implements java.security.Principal, 
Serializable
+/**
+ * FIXME public class needs documentation.
+ * @author Jeff Bailey (address@hidden)
+ */
+public final class KerberosPrincipal implements Principal, Serializable
 {
-  // Fields.
-  // -------------------------------------------------------------------------
-
   private static final long serialVersionUID = -8308522755600156056L;
 
   /**
@@ -65,7 +65,7 @@
   private int nameType;
 
   // Constants from RFC 1510 Section 8.3
-  
+  // FIXME: Public fields need documentation
   public static final int KRB_NT_UNKNOWN = 0;
   public static final int KRB_NT_PRINCIPAL = 1;
   public static final int KRB_NT_SRV_INST = 2;
@@ -74,9 +74,6 @@
   public static final int KRB_NT_UID = 5;
   
 
-  // Constructors.
-  // -------------------------------------------------------------------------
-
   /**
    * <p>Default Constructor for KerberosPrincipal</p>
    * 
@@ -88,13 +85,14 @@
     this(name, KRB_NT_PRINCIPAL);
   }
 
+  // FIXME: Public constructor needs documentation on how name is parsed.
   public KerberosPrincipal(String name, int nameType)
   {
     StringTokenizer st = new StringTokenizer(name, "@", false);
 
     // FIXME: What do we do if multiple @'s are present?
     // FIXME: Does this function cope if it's handed an empty string?
-    
+    // Try using String.indexof('@') and String.substring()
     principal = st.nextToken();
     if (st.hasMoreTokens())
       realm = st.nextToken();
@@ -104,18 +102,17 @@
     this.nameType = nameType;
   }
 
-  // Instance methods.
-  // -------------------------------------------------------------------------
-
   /**
-   * <p>Return the default realm</p>
-   *
-   * <p>The default realm is either set through the system property
+   * Return the default realm.
+   * The default realm is either set through the system property
    * java.security.krb5.realm, located in the /etc/krb5.conf file
-   * or is blank.</p>
+   * or is blank.
    */
   private String getDefaultRealm()
   {
+    // FIXME: Check whether "untrusted" code may know the realm.
+    // If untrusted code may and they can call this method (indirectly)
+    // use gnu.classpath.SystemProperties.getProperty().
     String a = System.getProperty("java.security.krb5.realm");
     if (a != null)
       return a;
@@ -124,11 +121,11 @@
   // Implement this when it's obvious how the rest is going to 
   // get implemented.
 
-    return new String("");
+    return "";
   }
   
   /**
-   * <p>Returns the value of the name type</p>
+   * Returns the value of the name type.
    */
   public int getNameType()
   {
@@ -136,7 +133,7 @@
   }
 
   /**
-   * <p>Returns the realm</p>
+   * Returns the realm.
    */
   public String getRealm()
   {
@@ -146,7 +143,7 @@
   // Methods from Interface Principal
 
   /**
-   * <p>From Section 7.2 of rfc1510: When comparing names, a name of type
+   * From Section 7.2 of rfc1510: When comparing names, a name of type
    * UNKNOWN will match principals authenticated with names of any type.
    * A principal authenticated with a name of type UNKNOWN, however, will
    * only match other names of type UNKNOWN.
@@ -172,7 +169,7 @@
   }
 
   /**
-   * <p>Provide a human readable version of the principal.</p>
+   * Provide a human readable version of the principal.
    */
   public String getName()
   {
@@ -184,11 +181,14 @@
     return principal;
   }
 
+  // FIXME - public method needs documentation.
   public int hashCode()
   {
+    // FIXME - Cannot be right because it uses other logic then equals().
     return getName().hashCode();
   }
 
+  // FIXME - public method needs documentation.
   public String toString()
   {
     return getName();

signature.asc
Description: This is a digitally signed message part