Tails

#1 Updated by kytv 2015-02-25 10:45:33

• blocked by Bug #6394: Seahorse launched from the gpgApplet cannot search for public keys added

#2 Updated by kytv 2015-02-25 18:48:00

• Assignee changed from kytv to alant
• QA Check set to Ready for QA
• Feature Branch set to kytv:test/8955-seahorse-sync

#3 Updated by kytv 2015-02-25 18:48:32

• Assignee changed from alant to anonym

#4 Updated by anonym 2015-03-16 18:19:48

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed

Looks great!

Here’s a code review, and I only have style suggestions really:

> commit b2b1998

There’s some sort of process to make gpgApplet a stand-alone application, I think. Therefore I’d prefer if this (Tails-specific) change was made to config/chroot_local-includes/etc/xdg/autostart/gpgApplet.desktop instead.

Note: I was sure that we had fixed this ages ago. So this is actually a bufix, since gpgApplet will open an inferior seahorse.

> commit 7d0729a

The signature counting trick is a great idea! My only issue is that from reading I fetch the "..." OpenPGP key using the GnuPG CLI, it’s not obvious it’s done with gpg -keyserver-options import-clean. Reading the features using this step later on requires knowing this implementation detail. What about adding an option to the step, so one can say I fetch the "..." OpenPGP key without any signatures using the GnuPG CLI or similar? Or something using the clean concept?

> commit 96766cf
> +def count_signatures(key)

I’d prefer a slightly more descriptive name, like count_gpg_key_signatures() (also I’m afraid about the issue implied by Feature #9030).

> + case qualifier
> + when ‘only’
> + assert_equal(count, num.to_i)

Please fix the indentation.

Also:
> + assert_equal(count, num.to_i)
and
> + assert(count > num.to_i)

Since these assertions are the key things in this step, I’d like proper error messages.

> commit 238fdbc
> commit 5cad36a
> (and maybe more)

General comment: we’ve had a lot of issues with lost keypresses when rapidly using chains of keyboard shortcuts with GNOME/Gtk applications. Let’s keep our eyes open, perhaps it’s ok this time so we don’t have add more bloaty images.

> + step “process \”seahorse\" is running"

Just a tip. I find it more readable to not using escapes when possible, i.e. 'process "seahorse" is running'. It’s not possible if we’d use the @#{…}" construction inside, but since we don’t in this instance it’d be nicer.

> -When /^I start Seahorse$/ do
> +When /^I start Seahorse\s?(|via the GpgApplet)$/ do |startmethod|
> […]
> + case startmethod

I don’t think we expect to add a third way to start gpgApplet, and when it’s a binary option, I prefer: [...]( via the GpgApplet)? do |via_gpgapplet|@ and then if via_gpgapplet [...] else [...]. I think it’s clearer, shorter, and so on. Also, I’d drop the \s? and move it into the capturing group, for readability.

Also, we’ve been spelling it *g*pgApplet, not *G*pgApplet, so far. Also in the “I fetch the …” step.

> commit fd34bb2
> Use less greedy regexes.

I don’t get the “greedy” part of e.g.

> -When /^the “([^”]*)" OpenPGP key is not in the live user’s public keyring$/ do |
keyid|
> When /^the “([^”])" OpenPGP key is not in the live user’s public keyring$/ do |
keyid|

Generally, when I’ve heard of “greedy” in this situation it’s been about whether the parser tries to do a minimal or maximal match of the pattern.

Also, what’s the purpose of this change?

#6 Updated by kytv 2015-03-17 03:17:47

• Assignee changed from kytv to anonym
• QA Check changed from Dev Needed to Ready for QA

anonym wrote:
> Looks great!

Thanks for the review. :) (and I’m relieved that these were, IMHO, fairly small things that were pointed out…phew).

> Here’s a code review, and I only have style suggestions really:
>
> > commit b2b1998
>
> There’s some sort of process to make gpgApplet a stand-alone application, I think. Therefore I’d prefer if this (Tails-specific) change was made to config/chroot_local-includes/etc/xdg/autostart/gpgApplet.desktop instead.

Oops, I didn’t realize that (and just learned about /etc/xdg/autostart. headdesk). Done in c68602f065

>
> Note: I was sure that we had fixed this ages ago. So this is actually a bufix, since gpgApplet will open an inferior seahorse.
>
> > commit 7d0729a
>
> The signature counting trick is a great idea! My only issue is that from reading I fetch the "..." OpenPGP key using the GnuPG CLI, it’s not obvious it’s done with gpg -keyserver-options import-clean. Reading the features using this step later on requires knowing this implementation detail. What about adding an option to the step, so one can say I fetch the "..." OpenPGP key without any signatures using the GnuPG CLI or similar? Or something using the clean concept?

Done in bb66b4917b

>
> > commit 96766cf
> > +def count_signatures(key)
>
> I’d prefer a slightly more descriptive name, like count_gpg_key_signatures() (also I’m afraid about the issue implied by Feature #9030).
>

Done in 852adec76. I don’t know how to handle the encapsulation that Feature #9030 addresses. (Time to dig into Ruby modules and classes).

> > + case qualifier
> > + when ‘only’
> > + assert_equal(count, num.to_i)
>
> Please fix the indentation.

Done in 440f8f68d3.

> Also:
> > + assert_equal(count, num.to_i)
> and
> > + assert(count > num.to_i)
>
> Since these assertions are the key things in this step, I’d like proper error messages.
>

Done in 1da859e481.

> > commit 238fdbc
> > commit 5cad36a
> > (and maybe more)
>
> General comment: we’ve had a lot of issues with lost keypresses when rapidly using chains of keyboard shortcuts with GNOME/Gtk applications. Let’s keep our eyes open, perhaps it’s ok this time so we don’t have add more bloaty images.
>

After many, many runs this test hasn’t had any problems with the keypresses, but I did with my Electrum tests. I’ll definitely look out for the keypress problems—I suspect I’ll be more likely to run into them because of my nested VM setup.

> > + step “process \”seahorse\" is running"
>
> Just a tip. I find it more readable to not using escapes when possible, i.e. 'process "seahorse" is running'. It’s not possible if we’d use the @#{…}" construction inside, but since we don’t in this instance it’d be nicer.
>

Done in 61c9b8416.

> > -When /^I start Seahorse$/ do
> > +When /^I start Seahorse\s?(|via the GpgApplet)$/ do |startmethod|
> > […]
> > + case startmethod
>
> I don’t think we expect to add a third way to start gpgApplet, and when it’s a binary option, I prefer: [...]( via the GpgApplet)? do |via_gpgapplet|@ and then if via_gpgapplet [...] else [...]. I think it’s clearer, shorter, and so on. Also, I’d drop the \s? and move it into the capturing group, for readability.
>

Done in 29695cc4e.

anonym wrote:
> Also, we’ve been spelling it *g*pgApplet, not *G*pgApplet, so far. Also in the “I fetch the …” step.

intrigeri wrote:
> FYI its real name is “Tails OpenPGP Applet”.

I changed it to Tails OpenPGP Applet in ddc8c5e0c.

>
> > commit fd34bb2
> > Use less greedy regexes.
>
> I don’t get the “greedy” part of e.g.
>
> > -When /^the “([^”]*)" OpenPGP key is not in the live user’s public keyring$/ do |
> keyid|
> > When /^the “([^”])" OpenPGP key is not in the live user’s public keyring$/ do |
> keyid|
>
> Generally, when I’ve heard of “greedy” in this situation it’s been about whether the parser tries to do a minimal or maximal match of the pattern.
>
> Also, what’s the purpose of this change?

I just wanted the regex to be stricter. "([^"]*)" would allow zero or more non-quote-chars to match, making the capture group optional, but the keyid is crucial to the step’s usefulness. "([^"]+)" will allow at least one or more non-quote-characters to satisfy the requirement, but even that’s not good. I think I’d rather do the following to require at least 8 characters (the short OpenPGP ID) and as many as 50 (a fingerprint with spaces):

diff --git a/features/step_definitions/torified_gnupg.rb b/features/step_definitions/torified_gnupg.rb
index 616d702..cf2b3ae 100644
--- a/features/step_definitions/torified_gnupg.rb
+++ b/features/step_definitions/torified_gnupg.rb
@@ -4,7 +4,7 @@ def count_gpg_signatures(key)
   return output.scan(/^sig/).count
 end

-Then /^the key "([^"]+)" has (only|more than) (\d+) signatures$/ do |key, qualifier, num|
+Then /^the key "([^"]{8,50})" has (only|more than) (\d+) signatures$/ do |key, qualifier, num|
   next if @skip_steps_while_restoring_background
   count = count_gpg_signatures(key)
   case qualifier
@@ -17,13 +17,13 @@ Then /^the key "([^"]+)" has (only|more than) (\d+) signatures$/ do |key, qualif
   end
 end

-When /^the "([^"]+)" OpenPGP key is not in the live user's public keyring$/ do |keyid|
+When /^the "([^"]{8,50})" OpenPGP key is not in the live user's public keyring$/ do |keyid|
   next if @skip_steps_while_restoring_background
   assert(!@vm.execute("gpg --batch --list-keys '#{keyid}'", LIVE_USER).success?,
          "The '#{keyid}' key is in the live user's public keyring.")
 end

-When /^I fetch the "([^"]+)" OpenPGP key using the GnuPG CLI( without any signatures)?$/ do |keyid, without|
+When /^I fetch the "([^"]{8,50})" OpenPGP key using the GnuPG CLI( without any signatures)?$/ do |keyid, without|
   next if @skip_steps_while_restoring_background
   if without
     importopts = '--keyserver-options import-clean'
@@ -47,7 +47,7 @@ When /^GnuPG uses the configured keyserver$/ do
          "GnuPG's stderr did not mention keyserver #{CONFIGURED_KEYSERVER_HOSTNAME}")
 end

-When /^the "([^"]+)" key is in the live user's public keyring after at most (\d+) seconds$/ do |keyid, delay|
+When /^the "([^"]{8,50})" key is in the live user's public keyring after at most (\d+) seconds$/ do |keyid, delay|
   next if @skip_steps_while_restoring_background
   try_for(delay.to_f, :msg => "The '#{keyid}' key is not in the live user's public keyring") {
     @vm.execute("gpg --batch --list-keys '#{keyid}'", LIVE_USER).success?
@@ -94,7 +94,7 @@ Then /^I synchronize keys in Seahorse$/ do
   @screen.wait("SeahorseWindow.png", 120)
 end

-When /^I fetch the "([^"]+)" OpenPGP key using Seahorse( via the Tails OpenPGP Applet)?$/ do |keyid, withgpgapplet|
+When /^I fetch the "([^"]{8,50})" OpenPGP key using Seahorse( via the Tails OpenPGP Applet)?$/ do |keyid, withgpgapplet|
   next if @skip_steps_while_restoring_background
   if withgpgapplet
     step "I start Seahorse via the Tails OpenPGP Applet"

I pushed everything but this last diff. Depending on the verdict I can commit that or restore it to the original "([^"]*)".

#7 Updated by Tails 2015-03-19 17:29:15

• Status changed from In Progress to Fix committed
• % Done changed from 0 to 100

Applied in changeset commit:d355b9fd1fd7ec68960d3973f663c5d5c28ca698.

#8 Updated by anonym 2015-03-19 17:42:04

• Assignee deleted (anonym)
• QA Check changed from Ready for QA to Pass

Thanks for the fixes!

kytv wrote:
> > > commit fd34bb2
> > > Use less greedy regexes.
> >
> > I don’t get the “greedy” part of e.g.
> >
> > > -When /^the “([^”]*)" OpenPGP key is not in the live user’s public keyring$/ do |
> > keyid|
> > > When /^the “([^”])" OpenPGP key is not in the live user’s public keyring$/ do |
> > keyid|
> >
> > Generally, when I’ve heard of “greedy” in this situation it’s been about whether the parser tries to do a minimal or maximal match of the pattern.
> >
> > Also, what’s the purpose of this change?
>
> I just wanted the regex to be stricter. "([^"]*)" would allow zero or more non-quote-chars to match, making the capture group optional

Capture groups cannot be optional (at least not with Ruby’s regex engine) in the sense that the n:th capture group can refer to a different sub-expression depending on the string matched against. Even m = /a(b)?c/.match(s) will always create the 1st capture group in the match object, i.e. m[1] is always defined; if no b was matched then m[1] = nil. In /([^"]*)/ the empty string is still an object that would be matched.

Or am I misunderstanding you?

> but the keyid is crucial to the step’s usefulness. "([^"]+)" will allow at least one or more non-quote-characters to satisfy the requirement, but even that’s not good. I think I’d rather do the following to require at least 8 characters (the short OpenPGP ID) and as many as 50 (a fingerprint with spaces):

I get your point, but I think we can keep this simple. I don’t mind the switch from * to +, obviously.

Merged!

#9 Updated by BitingBird 2015-03-22 11:53:55

• Target version changed from Tails_1.3.2 to Tails_1.3.1

#10 Updated by BitingBird 2015-03-23 02:03:05

• Status changed from Fix committed to Resolved