[PATCH] Provides validation for the image file argument.

[Darryl L. Pierce]

A file must have minimally a path and filename.

If a file contains authentication information then it must also have a
hostname.

Paths must be absolute.

If the file includes a protocol then that is dropped.

Signed-off-by: Darryl L. Pierce <dpierce(a)redhat.com&gt;
---
 cobbler/item_image.py |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/cobbler/item_image.py b/cobbler/item_image.py
index 98e0255..481951d 100644
--- a/cobbler/item_image.py
+++ b/cobbler/item_image.py
@@ -113,11 +113,32 @@ class Image(item.Item):
     def set_file(self,filename):
         """
         Stores the image location.  This should be accessible on all nodes
-        that need to access it.  Format: either /mnt/commonpath/foo.iso or 
-        nfs://host/path/foo.iso
+        that need to access it.  Format: can be one of the following:
+        * username:password@hostname:/path/to/the/filename.ext
+        * username@hostname:/path/to/the/filename.ext
+        * hostname:/path/to/the/filename.ext
+        * /path/to/the/filename.ext
         """
-        # FIXME: this should accept NFS paths or filesystem paths
-        self.file = filename
+        uri = ""
+        auth = hostname = path = ""
+        # we'll discard the protocol if it's supplied, for legacy support
+        if filename.find("://") != -1:
+            ignored, uri = filename.split("://")
+            filename = uri
+        else:
+            uri = filename
+
+        if filename.find("@") != -1: auth, filename =
filename.split("@")
+        if filename.find(":") != -1: hostname, filename =
filename.split(":")
+        # raise an exception if we don't have a valid path
+        if filename[0] != '/': raise CX(_("file contains an invalid
path"))
+        if filename.find("/") != -1: path, filename =
filename.rsplit("/", 1)
+
+        if len(filename) == 0:  raise CX(_("missing filename"))
+        if len(auth) > 0 and len(hostname) == 0:
+            raise CX(_("a hostname must be specified with authentication
details"))
+
+        self.file = uri
         return True
 
     def set_os_version(self,os_version):
-- 
1.6.0.6

[Darryl L. Pierce]

On Mon, Dec 22, 2008 at 04:27:13PM -0500, Darryl L. Pierce wrote:
>  A file must have minimally a path and filename.
>  
>  If a file contains authentication information then it must also have a
>  hostname.
>  
>  Paths must be absolute.
>  
>  If the file includes a protocol then that is dropped.
>  
>  Signed-off-by: Darryl L. Pierce <dpierce(a)redhat.com&gt;
>  ---
>   cobbler/item_image.py |   29 +++++++++++++++++++++++++----
>   1 files changed, 25 insertions(+), 4 deletions(-)
>  
>  diff --git a/cobbler/item_image.py b/cobbler/item_image.py
>  index 98e0255..481951d 100644
>  --- a/cobbler/item_image.py
>  +++ b/cobbler/item_image.py
>  @@ -113,11 +113,32 @@ class Image(item.Item):
>       def set_file(self,filename):
>           """
>           Stores the image location.  This should be accessible on all nodes
>  -        that need to access it.  Format: either /mnt/commonpath/foo.iso or 
>  -        nfs://host/path/foo.iso
>  +        that need to access it.  Format: can be one of the following:
>  +        * username:password@hostname:/path/to/the/filename.ext
>  +        * username@hostname:/path/to/the/filename.ext
>  +        * hostname:/path/to/the/filename.ext
>  +        * /path/to/the/filename.ext
>           """
>  -        # FIXME: this should accept NFS paths or filesystem paths
>  -        self.file = filename
>  +        uri = ""
>  +        auth = hostname = path = ""
>  +        # we'll discard the protocol if it's supplied, for legacy support
>  +        if filename.find("://") != -1:
>  +            ignored, uri = filename.split("://")
>  +            filename = uri
>  +        else:
>  +            uri = filename
>  +
>  +        if filename.find("@") != -1: auth, filename =
> filename.split("@")
>  +        if filename.find(":") != -1: hostname, filename =
> filename.split(":")
>  +        # raise an exception if we don't have a valid path
>  +        if filename[0] != '/': raise CX(_("file contains an invalid
> path"))
>  +        if filename.find("/") != -1: path, filename =
> filename.rsplit("/", 1)
>  +
>  +        if len(filename) == 0:  raise CX(_("missing filename"))
>  +        if len(auth) > 0 and len(hostname) == 0:
>  +            raise CX(_("a hostname must be specified with authentication
> details"))
>  +
>  +        self.file = uri
>           return True
>   
>       def set_os_version(self,os_version):
>  -- 
>  1.6.0.6 
Haven't heard any feedback on this patch in a month. I'm going to step
up and push it if nobody objects.

-- 
Darryl L. Pierce, Sr. Software Engineer @ Red Hat, Inc.
Virtual Machine Management - http://www.ovirt.org/
"What do you care what other people think, Mr. Feynman?"

[Michael DeHaan]

Darryl L. Pierce wrote:
>  On Mon, Dec 22, 2008 at 04:27:13PM -0500, Darryl L. Pierce wrote:
>    
> > A file must have minimally a path and filename.
> >
> > If a file contains authentication information then it must also have a
> > hostname.
> >
> > Paths must be absolute.
> >
> > If the file includes a protocol then that is dropped.
> >
> > Signed-off-by: Darryl L. Pierce <dpierce(a)redhat.com&gt;
> > ---
> >  cobbler/item_image.py |   29 +++++++++++++++++++++++++----
> >  1 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/cobbler/item_image.py b/cobbler/item_image.py
> > index 98e0255..481951d 100644
> > --- a/cobbler/item_image.py
> > +++ b/cobbler/item_image.py
> > @@ -113,11 +113,32 @@ class Image(item.Item):
> >      def set_file(self,filename):
> >          """
> >          Stores the image location.  This should be accessible on all nodes
> > -        that need to access it.  Format: either /mnt/commonpath/foo.iso or 
> > -        nfs://host/path/foo.iso
> > +        that need to access it.  Format: can be one of the following:
> > +        * username:password@hostname:/path/to/the/filename.ext
> > +        * username@hostname:/path/to/the/filename.ext
> > +        * hostname:/path/to/the/filename.ext
> > +        * /path/to/the/filename.ext
> >          """
> > -        # FIXME: this should accept NFS paths or filesystem paths
> > -        self.file = filename
> > +        uri = ""
> > +        auth = hostname = path = ""
> > +        # we'll discard the protocol if it's supplied, for legacy support
> > +        if filename.find("://") != -1:
> > +            ignored, uri = filename.split("://")
> > +            filename = uri
> > +        else:
> > +            uri = filename
> > +
> > +        if filename.find("@") != -1: auth, filename =
> filename.split("@")
> > +        if filename.find(":") != -1: hostname, filename =
> filename.split(":")
> > +        # raise an exception if we don't have a valid path
> > +        if filename[0] != '/': raise CX(_("file contains an invalid
> path"))
> > +        if filename.find("/") != -1: path, filename =
> filename.rsplit("/", 1)
> > +
> > +        if len(filename) == 0:  raise CX(_("missing filename"))
> > +        if len(auth) > 0 and len(hostname) == 0:
> > +            raise CX(_("a hostname must be specified with authentication
> details"))
> > +
> > +        self.file = uri
> >          return True
> >  
> >      def set_os_version(self,os_version):
> > -- 
> > 1.6.0.6
> >     
>
>  Haven't heard any feedback on this patch in a month. I'm going to step
>  up and push it if nobody objects.
>
>    
>  ------------------------------------------------------------------------
>
>  _______________________________________________
>  cobbler mailing list
>  cobbler(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/cobbler
>     
It got lost in the patch queue.

A couple of things you need to do if you push it:

(A) remove the one line if's
(B) add unit tests to test_basic.py
(C) verify that it accepts "/path/to/foo.iso" and 
"nfs://user@path:/path/to/foo.iso" and "nfs://path:/path/to/foo.iso"
in 
terms of input.
(D) verify it does not accept relative paths

Writing something in test_basic.py for this would probably be a good idea.

--Michael