Re: [PATCH 1/9] pakfire: Refactor dblist seperating UI and logic

[Robin Roevens <robin.roevens@disroot.org>]

[-- Attachment #1: Type: text/plain, Size: 32987 bytes --]

Hi Michael

Michael Tremer schreef op ma 21-03-2022 om 16:18 [+0000]:
> Hello Robin,
> 
> Thank you for looking into Pakfire and making this thing more robust.
> 
> I have been meaning to give you a good review on this, but it is a
> lot of code with a lot of changes, which cannot introduce any
> regressions, because if something breaks we cannot fix it in an
> update...
> 
> > On 9 Mar 2022, at 22:56, Robin Roevens <robin.roevens(a)disroot.org>
> > wrote:
> > 
> > - Removed UI code from dblist function and refactor it making it
> > return
> >  a hash representing the pak db for easier handling of this data.
> 
> Yes. This is a good idea.
> 
> > - Moved core update check in dblist to new seperate dbcoreinfo
> > function
> >  making it return a hash with current and possibly available core
> >  version info.
> > - Update existing calls to dblist
> > - Bring UI parts previously in dblist to pakfire program itself,
> >  pakfire.cgi and index.cgi with a few small enhancements:
> >  - Add currently installed version numbers to installed paks list
> > in
> >    pakfire.cgi
> >  - Add 'Installed: yes/no' to pakfire list output so people not
> > using
> >    colors have this information too.
> >  - Add update available details to pakfire list output if package
> > has
> >    updates available.
> 
> Again, this would have been easier to review if it was split into
> smaller changes.

I'm doing my best :-) .. but I'm not sure how I should have split this
up more ? Assuming every patch should leave the system in a fully
functioning state ? By reworking the dblist function, I had to include
the changes to the calls to that function, and due to the removal of
the ui parts from it, I also had to include the ui additions in other
places
I only see those small visual enhancements in the ui parts that are now
outside of the dblist function as candidate changes for separate
patches, but then, as that code is already new in this patch, just
changing exactly what is displayed UI-wise didn't look invasive enough
to separate it ?

> 
> > 
> > Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> > ---
> > html/cgi-bin/index.cgi       |   6 +-
> > html/cgi-bin/pakfire.cgi     |  24 +++++-
> > src/pakfire/lib/functions.pl | 147 ++++++++++++++++----------------
> > ---
> > src/pakfire/pakfire          |  99 +++++++++++++++++------
> > 4 files changed, 168 insertions(+), 108 deletions(-)
> > 
> > diff --git a/html/cgi-bin/index.cgi b/html/cgi-bin/index.cgi
> > index 18c26942e..6fecae1ff 100644
> > --- a/html/cgi-bin/index.cgi
> > +++ b/html/cgi-bin/index.cgi
> > @@ -604,7 +604,11 @@ if ($warnmessage) {
> >         &Header::closebox();
> > }
> > 
> > -&Pakfire::dblist("upgrade", "notice");
> > +my %coredb = &Pakfire::coredbinfo();
> > +if (defined $coredb{'AvailableRelease'}) {
> > +       print "<br /><br /><br /><a
> > href='pakfire.cgi'>$Lang::tr{'core notice 1'} $coredb{'Release'}
> > $Lang::tr{'core notice 2'} $coredb{'AvailableRelease'}
> > $Lang::tr{'core notice 3'}</a>";
> > +}
> > +
> > if ( -e "/var/run/need_reboot" ) {
> >         print "<div style='text-align:center; color:red;'>";
> >         print "<br/><br/>$Lang::tr{'needreboot'}!";
> > diff --git a/html/cgi-bin/pakfire.cgi b/html/cgi-bin/pakfire.cgi
> > index 65c67fb90..30a23128b 100644
> > --- a/html/cgi-bin/pakfire.cgi
> > +++ b/html/cgi-bin/pakfire.cgi
> > @@ -370,7 +370,17 @@ print <<END;
> >                                         <select name="UPDPAKS"
> > class="pflist" size="5" disabled>
> > END
> > 
> > -       &Pakfire::dblist("upgrade", "forweb");
> > +       my %coredb = &Pakfire::coredbinfo();
> > +       if (defined $coredb{'AvailableRelease'}) {
> > +               print "<option value=\"core\">Core-Update --
> > $coredb{'CoreVersion'} -- Release: $coredb{'Release'} ->
> > $coredb{'AvailableRelease'}</option>\n";
> > +       }
> 
> The strings “Core-Update” (without the dash) and “Release” should be
> translated so that they make sense in other languages. You can do
> that with $Lang::tr (I am sure you know).
Agreed, I don't think it was originally in this place as this came out
of the previous dblist function. But you are right, I will use Lang::tr

> 
> > +
> > +       my %upgradelist = &Pakfire::dblist("upgrade");
> > +       foreach my $pak (sort keys %upgradelist) {
> > +               print "<option value=\"$pak\">Update: $pak --
> > Version: $upgradelist{$pak}{'ProgVersion'} ->
> > $upgradelist{$pak}{'AvailableProgVersion'} -- Release:
> > $upgradelist{$pak}{'Release'} ->
> > $upgradelist{$pak}{'AvailableRelease'}</option>\n";
> > +       }
> 
> Same again here.
> 
> As a UI element, <select> is a very stupid idea. I have no idea why I
> ever picked that - presumably because there was the intention that
> users can pick what they want to install and what not. We should move
> away from this I believe.
Completely agree :-).. I was already trying to completely redesign the
look of the pakfire webui page, to for example also include the summary
now available in pak metadata.
But I did not yet have a satisfying design in mind .. but I will
probably propose something for this here with some mockup screenshots
soon.
So for now I kept it like it was.

> 
> > +
> > +
> >         print <<END;
> >                                         </select>
> >                                         <input type='hidden'
> > name='ACTION' value='upgrade' />
> > @@ -386,7 +396,11 @@ END
> >                                         <select name="INSPAKS"
> > class="pflist" size="10" multiple>
> > END
> > 
> > -       &Pakfire::dblist("notinstalled", "forweb");
> > +       my %notinstalledlist = &Pakfire::dblist("notinstalled");
> > +       foreach my $pak (sort keys %notinstalledlist) {
> > +               print "<option value=\"$pak\">$pak-
> > $notinstalledlist{$pak}{'ProgVersion'}-
> > $notinstalledlist{$pak}{'Release'}</option>\n";
> > +       }
> > +
> >         print <<END;
> >                                         </select>
> >                                         <input type='hidden'
> > name='ACTION' value='install' />
> > @@ -398,7 +412,11 @@ END
> >                                         <select name="DELPAKS"
> > class="pflist" size="10" multiple>
> > END
> > 
> > -       &Pakfire::dblist("installed", "forweb");
> > +       my %installedlist = &Pakfire::dblist("installed");
> > +       foreach my $pak (sort keys %installedlist) {
> > +               print "<option value=\"$pak\">$pak-
> > $installedlist{$pak}{'ProgVersion'}-
> > $installedlist{$pak}{'Release'}</option>\n";
> > +       }
> > +
> >         print <<END;
> >                                         </select>
> >                                         <input type='hidden'
> > name='ACTION' value='remove' />
> > diff --git a/src/pakfire/lib/functions.pl
> > b/src/pakfire/lib/functions.pl
> > index d4e338f23..f08f43622 100644
> > --- a/src/pakfire/lib/functions.pl
> > +++ b/src/pakfire/lib/functions.pl
> > @@ -2,7 +2,7 @@
> > ###################################################################
> > ############
> > #                                                                  
> >            #
> > # IPFire.org - A linux based
> > firewall                                         #
> > -# Copyright (C) 2007-2021   IPFire Team  
> > <info(a)ipfire.org>                   #
> > +# Copyright (C) 2007-2022   IPFire Team  
> > <info(a)ipfire.org>                   #
> > #                                                                  
> >            #
> > # This program is free software: you can redistribute it and/or
> > modify        #
> > # it under the terms of the GNU General Public License as published
> > by        #
> > @@ -44,7 +44,7 @@ my @VALID_KEY_FINGERPRINTS = (
> > );
> > 
> > # A small color-hash :D
> > -my %color;
> > +our %color;
> >         $color{'normal'}      = "\033[0m";
> >         $color{'black'}       = "\033[0;30m";
> >         $color{'darkgrey'}    = "\033[1;30m";
> > @@ -426,108 +426,93 @@ sub dbgetlist {
> >         }
> > }
> > 
> > +sub coredbinfo {
> > +       ### This subroutine returns core db version information in
> > a hash.
> > +       # Usage is without arguments
> > +
> > +       eval(`grep "core_" $Conf::dbdir/lists/core-list.db`);
> 
> I know the eval() statement has been used in the past, but since we
> touched this code now, can we remove this and just parse the line
> that we are looking for?
Done in patch 4/9. Due to splitting up my changes in separate patches
as much as I thought possible :-)

Robin
> 
> I consider this a security problem as it allows code injection.
> 
> > +
> > +       my %coredb = (
> > +               CoreVersion => $Conf::version,
> > +               Release => $Conf::core_mine,
> > +       );
> > +
> > +       $coredb{'AvailableRelease'} = $core_release if
> > ("$Conf::core_mine" < "$core_release");
> > +
> > +       return %coredb;
> > +}
> > +
> > sub dblist {
> > -       ### This subroutine lists the packages.
> > -       #   You may also pass a filter: &Pakfire::dblist(filter)
> > -       #   Usage is always with two arguments.
> > -       #   filter may be: all, notinstalled, installed
> > +       ### This subroutine returns the packages from the
> > packages_list db in a hash.
> > +       #   It uses the currently cached version of packages_list.
> > To ensure latest 
> > +       #   data, run Pakfire::dbgetlist first.
> > +       #   You may also pass a filter: &Pakfire::dblist(filter) 
> > +       #   Usage is always with one argument.
> > +       #   filter may be: all, notinstalled, installed, upgrade
> >         my $filter = shift;
> > -       my $forweb = shift;
> > -       my @updatepaks;
> > +       my %paklist = ();
> >         my $file;
> >         my $line;
> > -       my $prog;
> >         my %metadata;
> >         my @templine;
> > -
> > -       ### Make sure that the list is not outdated.
> > -       #dbgetlist("noforce");
> > -
> > +       
> >         open(FILE, "<$Conf::dbdir/lists/packages_list.db");
> >         my @db = <FILE>;
> >         close(FILE);
> > 
> > -       if ("$filter" eq "upgrade") {
> > -               if ("$forweb" ne "forweb" && "$forweb" ne "notice"
> > ) {getcoredb("noforce");}
> > -               eval(`grep "core_" $Conf::dbdir/lists/core-
> > list.db`);
> > -               if ("$core_release" > "$Conf::core_mine") {
> > -                       if ("$forweb" eq "forweb") {
> > -                               print "<option value=\"core\">Core-
> > Update -- $Conf::version -- Release: $Conf::core_mine ->
> > $core_release</option>\n";
> > -                       }
> > -                       elsif ("$forweb" eq "notice") {
> > -                               print "<br /><br /><br /><a
> > href='pakfire.cgi'>$Lang::tr{'core notice 1'} $Conf::core_mine
> > $Lang::tr{'core notice 2'} $core_release $Lang::tr{'core notice
> > 3'}</a>";
> > -                       } else {
> > -                               my $command = "Core-Update
> > $Conf::version\nRelease: $Conf::core_mine -> $core_release\n";
> > -                               if ("$Pakfire::enable_colors" eq
> > "1") {
> > -                                       print
> > "$color{'lila'}$command$color{'normal'}\n";
> > -                               } else {
> > -                                       print "$command\n";
> > -                               }
> > -                       }
> > -               }
> > -
> > +       if ("$filter" ne "notinstalled") {
> >                 opendir(DIR,"$Conf::dbdir/installed");
> >                 my @files = readdir(DIR);
> >                 closedir(DIR);
> > +
> >                 foreach $file (@files) {
> >                         next if ( $file eq "." );
> >                         next if ( $file eq ".." );
> >                         next if ( $file =~ /^old/ );
> >                         %metadata =
> > parsemetafile("$Conf::dbdir/installed/$file");
> > 
> > -                       foreach $prog (@db) {
> > -                               @templine = split(/\;/,$prog);
> > -                               if (("$metadata{'Name'}" eq
> > "$templine[0]") && ("$metadata{'Release'}" < "$templine[2]" &&
> > "$forweb" ne "notice")) {
> > -
> >                                        push(@updatepaks,$metadata{'N
> > ame'});
> > -                                       if ("$forweb" eq "forweb")
> > {
> > -                                               print "<option
> > value=\"$metadata{'Name'}\">Update: $metadata{'Name'} -- Version:
> > $metadata{'ProgVersion'} -> $templine[1] -- Release:
> > $metadata{'Release'} -> $templine[2]</option>\n";
> > -                                       } else {
> > -                                               my $command =
> > "Update: $metadata{'Name'}\nVersion: $metadata{'ProgVersion'} ->
> > $templine[1]\nRelease: $metadata{'Release'} -> $templine[2]\n";
> > -                                               if
> > ("$Pakfire::enable_colors" eq "1") {
> > -                                                       print
> > "$color{'lila'}$command$color{'normal'}\n";
> > -                                               } else {
> > -                                                       print
> > "$command\n";
> > -                                               }
> > -                                       }
> > +                       foreach $line (@db) {
> > +                               next unless ($line =~ /.*;.*;.*;/
> > );
> > +                               @templine = split(/\;/,$line);
> > +                               if (("$metadata{'Name'}" eq
> > "$templine[0]") && ("$metadata{'Release'}" < "$templine[2]")) { #&&
> > "$forweb" ne "notice")) {
> > +                                       # Add all upgradable paks
> > to list
> > +                                       $paklist{"$metadata{'Name'}
> > "} = {
> > +                                               ProgVersion =>
> > $metadata{'ProgVersion'},
> > +                                               Release =>
> > $metadata{'Release'},
> > +                                               AvailableProgVersio
> > n => $templine[1],
> > +                                               AvailableRelease =>
> > $templine[2],
> > +                                               Installed => "yes"
> > +                                       };
> > +                                       last;
> > +                               } elsif (("$metadata{'Name'}" eq
> > "$templine[0]") && ("$filter" ne "upgrade")) {
> > +                                       # Add installed paks
> > without an upgrade available to list
> > +                                       $paklist{"$metadata{'Name'}
> > "} = {
> > +                                               ProgVersion =>
> > $metadata{'ProgVersion'},
> > +                                               Release =>
> > $metadata{'Release'},
> > +                                               Installed => "yes"
> > +                                       };
> > +                                       last;
> >                                 }
> >                         }
> >                 }
> > -               return @updatepaks;
> > -       } else {
> > -               my $line;
> > -               my $use_color;
> > -               my @templine;
> > -               my $count;
> > -               foreach $line (sort @db) {
> > +       }
> > +
> > +       # Add all not installed paks to list
> > +       if (("$filter" ne "upgrade") && ("$filter" ne "installed"))
> > {
> > +               foreach $line (@db) {
> >                         next unless ($line =~ /.*;.*;.*;/ );
> > -                       $use_color = "";
> >                         @templine = split(/\;/,$line);
> > -                       if ("$filter" eq "notinstalled") {
> > -                               next if ( -e
> > "$Conf::dbdir/installed/meta-$templine[0]" );
> > -                       } elsif ("$filter" eq "installed") {
> > -                               next unless ( -e
> > "$Conf::dbdir/installed/meta-$templine[0]" );
> > -                       }
> > -                       $count++;
> > -                       if ("$forweb" eq "forweb")
> > -                        {
> > -                               if ("$filter" eq "notinstalled") {
> > -                                       print "<option
> > value=\"$templine[0]\">$templine[0]-$templine[1]-
> > $templine[2]</option>\n";
> > -                               } else {
> > -                                       print "<option
> > value=\"$templine[0]\">$templine[0]</option>\n";
> > -                               }
> > -                       } else {
> > -                               if ("$Pakfire::enable_colors" eq
> > "1") {
> > -                                       if
> > (&isinstalled("$templine[0]")) {
> > -                                               $use_color =
> > "$color{'red'}"
> > -                                       } else {
> > -                                               $use_color =
> > "$color{'green'}"
> > -                                       }
> > -                               }
> > -                               print "${use_color}Name:
> > $templine[0]\nProgVersion: $templine[1]\nRelease:
> > $templine[2]$color{'normal'}\n\n";
> > -                       }
> > +                       next if ((defined $paklist{"$templine[0]"})
> > || (&isinstalled($templine[0]) == 0));
> > +
> > +                       $paklist{"$templine[0]"} = {
> > +                               ProgVersion => "$templine[1]",
> > +                               Release => "$templine[2]",
> > +                               Installed => "no"
> > +                       };
> >                 }
> > -               print "$count packages total.\n" unless ("$forweb"
> > eq "forweb");
> >         }
> > +
> > +       return %paklist;
> > }
> 
> This all looks good to me.
> 
> > sub resolvedeps_one {
> > @@ -896,10 +881,10 @@ sub progress_bar {
> > 
> > sub updates_available {
> >         # Get packets with updates available
> > -       my @upgradepaks = &Pakfire::dblist("upgrade", "noweb");
> > +       my %upgradepaks = &Pakfire::dblist("upgrade");
> > 
> > -       # Get the length of the returned array
> > -       my $updatecount = scalar @upgradepaks;
> > +       # Get the length of the returned hash
> > +       my $updatecount = keys %upgradepaks;
> > 
> >         return "$updatecount";
> > }
> > diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire
> > index 6c77695c8..b4930e85d 100644
> > --- a/src/pakfire/pakfire
> > +++ b/src/pakfire/pakfire
> > @@ -270,14 +270,25 @@
> >                 &Pakfire::getcoredb("$force");
> > 
> >         } elsif ("$ARGV[0]" eq "upgrade") {
> > +               my $use_color = "";
> > +               my $reset_color = "";
> > +
> > +               if ("$Pakfire::enable_colors" eq "1") {
> > +                       $reset_color = "$Pakfire::color{'normal'}";
> > +                       $use_color =
> > "$Pakfire::color{'lightpurple'}";
> > +               }
> > +
> >                 &Pakfire::upgradecore();
> > -               my @upgradepaks = &Pakfire::dblist("upgrade",
> > "noweb");
> > +               
> >                 my @deps = ();
> > -
> > -               if (@upgradepaks) {
> > +               if (my %upgradepaks = &Pakfire::dblist("upgrade"))
> > {
> >                         # Resolve the dependencies of the to be
> > upgraded packages
> > -                       @deps =
> > &Pakfire::resolvedeps_recursive(@upgradepaks);
> > +                       @deps =
> > &Pakfire::resolvedeps_recursive(keys %upgradepaks);
> > 
> > +                       foreach $pak (sort keys %upgradepaks) {
> > +                               print "${use_color}Update:
> > $pak\nVersion: $upgradepaks{$pak}{'ProgVersion'} ->
> > $upgradepaks{$pak}{'AvailableProgVersion'}\n";
> > +                               print "Release:
> > $upgradepaks{$pak}{'Release'} ->
> > $upgradepaks{$pak}{'AvailableRelease'}${reset_color}\n";
> > +                       }
> >                         &Pakfire::message("");
> >                         &Pakfire::message("PAKFIRE UPGR: We are
> > going to install all packages listed above.");
> >                         if ($interactive) {
> > @@ -290,36 +301,78 @@
> >                                   exit 1;
> >                                 }
> >                         }
> > -               }
> > +               
> > +                       # Download packages
> > +                       foreach $pak (sort keys %upgradepaks) {
> > +                               &Pakfire::getpak("$pak", "");
> > +                       }
> > 
> > -               # Download packages
> > -               foreach $pak (@upgradepaks) {
> > -                       &Pakfire::getpak("$pak", "");
> > +                       # Download dependencies
> > +                       foreach $pak (@deps) {
> > +                               &Pakfire::getpak("$pak", "");
> > +                       }
> > +
> > +                       # Install dependencies first
> > +                       foreach $pak (@deps) {
> > +                               &Pakfire::setuppak("$pak");
> > +                       }
> > +
> > +                       # Install all upgrades
> > +                       foreach $pak (sort keys %upgradepaks) {
> > +                               &Pakfire::upgradepak("$pak");
> > +                       }
> > +               } else {
> > +                       &Pakfire::message("PAKFIRE WARN: No new
> > package upgrades available.");
> >                 }
> > 
> > -               # Download dependencies
> > -               foreach $pak (@deps) {
> > -                       &Pakfire::getpak("$pak", "");
> > +       } elsif ("$ARGV[0]" eq "list") {
> > +               my $count;
> > +               my $use_color = "";
> > +               my $reset_color = "";
> > +               my $filter = "all";
> > +
> > +               if ("$ARGV[1]" =~ /installed|notinstalled/) {
> > +                       $filter = "$ARGV[1]";
> > +               } else {
> > +                       &Pakfire::message("PAKFIRE WARN: Not a
> > known option $ARGV[1]") if ($ARGV[1]); 
> >                 }
> > 
> > -               # Install dependencies first
> > -               foreach $pak (@deps) {
> > -                       &Pakfire::setuppak("$pak");
> > +               my $pak;
> > +               my %paklist = &Pakfire::dblist($filter);
> > +
> > +               if ("$Pakfire::enable_colors" eq "1") {
> > +                       $reset_color = "$Pakfire::color{'normal'}";
> > +                       $use_color =
> > "$Pakfire::color{'lightgreen'}";
> >                 }
> > 
> > -               # Install all upgrades
> > -               foreach $pak (@upgradepaks) {
> > -                       &Pakfire::upgradepak("$pak");
> > +               foreach $pak (sort keys %paklist) {
> > +                       if ("$Pakfire::enable_colors" eq "1") {
> > +                               if ("$paklist{$pak}{'Installed'}"
> > eq "yes") {
> > +                                       if (defined
> > $paklist{$pak}{'AvailableProgVersion'}) {
> > +                                               $use_color =
> > "$Pakfire::color{'lightgreen'}";
> > +                                       } else {
> > +                                               $use_color =
> > "$Pakfire::color{'green'}";
> > +                                       }
> > +                               } else {
> > +                                       $use_color =
> > "$Pakfire::color{'red'}"; 
> > +                               }
> > +                       }
> > +
> > +                       print "${use_color}Name: $pak\nProgVersion:
> > $paklist{$pak}{'ProgVersion'}\n";
> > +                       print "Release:
> > $paklist{$pak}{'Release'}\nInstalled:
> > $paklist{$pak}{'Installed'}\n";
> > +                       if (defined
> > $paklist{$pak}{'AvailableProgVersion'}) {
> > +                               print "Update available:\n Version:
> > $paklist{$pak}{'ProgVersion'} ->
> > $paklist{$pak}{'AvailableProgVersion'}\n Release:
> > $paklist{$pak}{'Release'} -> $paklist{$pak}{'AvailableRelease'}\n";
> > +                       }
> > +                       print "${reset_color}\n";
> > +                       
> >                 }
> > 
> > -       } elsif ("$ARGV[0]" eq "list") {
> > -               if ("$ARGV[1]" =~ /installed|notinstalled/) {
> > -                       &Pakfire::dblist("$ARGV[1]", "noweb");
> > +               $count = keys %paklist;
> > +               if ($count > 0) {
> > +                       print "$count packages total.\n";
> >                 } else {
> > -                       &Pakfire::message("PAKFIRE WARN: Not a
> > known option $ARGV[1]") if ($ARGV[1]);
> > -                       &Pakfire::dblist("all", "noweb");
> > +                       &Pakfire::message("PAKFIRE WARN: No
> > packages where found using filter $filter.");
> >                 }
> > -
> >         } elsif ("$ARGV[0]" eq "resolvedeps") {
> >                 foreach (@ARGV) {
> >                         next if ("$_" eq "resolvedeps");
> > -- 
> > 2.34.1
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
> -Michael
> 
> 

-- 
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.