Bug 451153 - Review Request: mapbender - Geospatial portal for OGC OWS architectures
Review Request: mapbender - Geospatial portal for OGC OWS architectures
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-12 21:34 EDT by Balint Cristian
Modified: 2009-01-11 11:01 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-11 11:01:03 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Balint Cristian 2008-06-12 21:34:32 EDT
Spec URL: http://openrisc.rdsor.ro/mapbender.spec
SRPM URL: http://openrisc.rdsor.ro/mapbender-2.4.5-1.fc9.src.rpm
Description: 
Mapbender is the geospatial portal site management software for
OGC OWS architectures.Mapbender comes with a data model to manage
interfaces for displaying, navigating and querying OGC compliant
web map and feature services (WMS and transactional WFS).
Authentication and authorization are used by the OWS Security
Proxy and management interfaces for multi client capable user,
group and service administration. The embedded metadata component
follows ISO 19119 specification.
Comment 1 Balint Cristian 2008-06-12 21:37:08 EDT
This package is very similar to squirrelmail.

These rpmlint are waveable,  
640/root-apache since it contain sensitive info:
root - edit/write.
apache - must read it only.
else - cannot read.

mapbender.noarch: E: non-standard-gid /etc/mapbender/digitize_default.conf apache
mapbender.noarch: E: non-readable /etc/mapbender/digitize_default.conf 0640
mapbender.noarch: E: non-standard-gid /etc/mapbender/poi.conf apache
mapbender.noarch: E: non-readable /etc/mapbender/poi.conf 0640
mapbender.noarch: E: non-standard-gid /etc/mapbender/wfs_default.conf apache
mapbender.noarch: E: non-readable /etc/mapbender/wfs_default.conf 0640
mapbender.noarch: E: non-standard-gid /etc/mapbender/mapbender.conf apache
mapbender.noarch: E: non-readable /etc/mapbender/mapbender.conf 0640
mapbender.noarch: E: non-standard-gid /etc/mapbender/gazetteerSQL.conf apache
mapbender.noarch: E: non-readable /etc/mapbender/gazetteerSQL.conf 0640
mapbender.noarch: E: non-standard-gid /etc/mapbender/session.conf apache
mapbender.noarch: E: non-readable /etc/mapbender/session.conf 0640
Comment 2 Jason Tibbitts 2008-06-21 22:24:15 EDT
I'm not sure what the point of the %post scriptlet is.  That output doesn't go
anywhere; I'm not even sure yum displays things like that, and it certainly
won't be seen for things like kickstart installs.  It's best to put that
information into a README.Fedora file and install it as %doc.

Is there anything which restricts access to localhost by default?  The conf just
sets up the alias but I don't see any access restrictions.  Since this program
comes up with a default password (according to the information in the %post
scriptlet, at least), it would be very bad for it to allow access from the
network by default.
Comment 3 Balint Cristian 2008-07-05 07:55:50 EDT
Spec URL: http://openrisc.rdsor.ro/mapbender.spec
SRPM URL: http://openrisc.rdsor.ro/mapbender-2.4.5-2.fc9.src.rpm

Updated to fullfill requests.

* Sat Jul 05 2008 Balint Cristian <rezso@rdsor.ro> - 2.4.5-2
- allow connections only from localhost by defult
- create README.fedora doc

Comment 4 Balint Cristian 2008-12-09 11:47:24 EST
Spec URL: http://openrisc.rdsor.ro/mapbender.spec
SRPM URL: http://openrisc.rdsor.ro/mapbender-2.5-1.fc11.src.rpm

Re-Updated to latest upstream.
Comment 5 Lubomir Rintel 2009-01-04 07:10:59 EST
Just a few minor issues/notes:

1.) The license seems to be GPL 2 or newer, therefore GPLv2+, please correct the License tag

2.) What is %contentdir for?

You do:

%define contentdir /var/www
mkdir -p -m0755 %{buildroot}%{contentdir}/html

Yet you do not seem to be putting anything there, nor is it included in the resulting package.

3.) This does not look very nice:

set +x 
for f in `find . -type f` ; do
   if file $f | grep -q ISO-8859 ; then

I'm not sure whether it's safe to rely on libmagic, since as far as I know (I may be wrong as well..), it identifies the charset used by the first few (kilo?) bytes, not reading the whole file. Why not convert all files -- it may be that iconving all files may be even faster than, or at least comparable, with calling file against them.

      set -x
      iconv -f ISO-8859-1 -t UTF-8 $f > ${f}.tmp && \
         mv -f ${f}.tmp $f

I'm not sure about use of && here. I think you should break the build if iconv fails instead of leaving a stale .tmp file.

Also, a good habit is to preserve timestamp -- touch -r $f $f.tmp before mv.

      set +x
   fi
   if file $f | grep -q CRLF ; then

I'd say you don't have to call file here as well. It is safe to remove all \r-s.

      set -x
      sed -i -e 's|\r||g' $f
      set +x
   fi
done
set -x

Given none of these are serious enough to warrant a blocker, the package is

APPROVED
Comment 6 Balint Cristian 2009-01-04 07:24:11 EST
(In reply to comment #5)
> Just a few minor issues/notes:
> 
> 1.) The license seems to be GPL 2 or newer, therefore GPLv2+, please correct
> the License tag
> 
> 2.) What is %contentdir for?
> 
> You do:
> 
> %define contentdir /var/www
> mkdir -p -m0755 %{buildroot}%{contentdir}/html

- Temporary macro to allow /var/www path. Since this package deal a lot
with /var/www path, and will in the future if this piece of software will expand. RPM really miss many of some handfull macros, this is not the first 
in my opinion.

 
> Yet you do not seem to be putting anything there, nor is it included in the
> resulting package.
> 
> 3.) This does not look very nice:
> 
> set +x 
> for f in `find . -type f` ; do
>    if file $f | grep -q ISO-8859 ; then
> 
> I'm not sure whether it's safe to rely on libmagic, since as far as I know (I
> may be wrong as well..), it identifies the charset used by the first few
> (kilo?) bytes, not reading the whole file. Why not convert all files -- it may
> be that iconving all files may be even faster than, or at least comparable,
> with calling file against them.

> 
>       set -x
>       iconv -f ISO-8859-1 -t UTF-8 $f > ${f}.tmp && \
>          mv -f ${f}.tmp $f

- Yes true. However if file is not ISO-8859-1 and try force convert to UTF-8
than rpmlint will yale, so its safe for now. ~99% is always ISO-8859-2.
- I am upset all time when upstream not follow UTF-8 so this jhack is usefull,
especialy rpmlint will allways signal if something.

 
> I'm not sure about use of && here. I think you should break the build if iconv
> fails instead of leaving a stale .tmp file.

- Hmm. Never encounter such issue. However this combination never failed on
any of my packages untill now.

> 
> Also, a good habit is to preserve timestamp -- touch -r $f $f.tmp before mv.
> 
>       set +x
>    fi
>    if file $f | grep -q CRLF ; then

- Will take care in next spin %{version}+1

> 
> I'd say you don't have to call file here as well. It is safe to remove all
> \r-s.
> 
>       set -x
>       sed -i -e 's|\r||g' $f
>       set +x
>    fi
> done
> set -x

- Will proceed in next spin %{version}+1

 However these 'sanity' bits was suggested by Patrice Dumas and Mamoru Tasaka, my very first reviewers. I never investigated deeply how can be improved,
i just always do it on any of my packages.

 
> Given none of these are serious enough to warrant a blocker, the package is
> 
> APPROVED
Comment 7 Balint Cristian 2009-01-04 07:29:43 EST
New Package CVS Request
=======================
Package Name: mapbender
Short Description: Geospatial portal for OGC OWS architectures
Owners: rezso
Branches: F-8 F-9 F-10 devel
Comment 8 Kevin Fenzi 2009-01-04 15:07:09 EST
F-8 branches are no longer accepted. 

Please do remember to fix the License before import. 

cvs done for the rest.

Note You need to log in before you can comment on or make changes to this bug.