Project

General

Profile

Bug #15102

Have a "Show passphrase" option when creating persistence

Added by sajolida almost 2 years ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
Persistence
Target version:
Start date:
12/25/2017
Due date:
% Done:

100%

Feature Branch:
bugfix/15102-have-a-show-passphrase-option-when-creating-persistence, persistence-setup:bugfix/15102-have-a-show-passphrase-option-when-creating-persistence
Type of work:
Code
Blueprint:
Starter:
Yes
Affected tool:

Description

Like we have in Tails Greeter.

The (Perl + GTK) code lives in https://git-tails.immerda.ch/persistence-setup/ and more specifically in https://git-tails.immerda.ch/persistence-setup/tree/lib/Tails/Persistence/Step/Bootstrap.pm. It should be doable to work on this without having to build an ISO image: edit the Perl code in place, run it, debug, rince and repeat.

show passphrase.png View (4.96 KB) sajolida, 12/25/2017 10:34 PM


Related issues

Related to Tails - Feature #14548: Fix issues identified in the expert review done by jaster on our installation instructions Confirmed 09/28/2013

Associated revisions

Revision 5b94ca7c (diff)
Added by intrigeri about 2 months ago

Import bugfix/15102-have-a-show-passphrase-option-when-creating-persistence as a patch (refs: #15102)

Snapshot taken at commit 8ee3e23d3b68fb1bfb37827410407256a0bd29e7.

Revision 8b7b8301 (diff)
Added by intrigeri about 1 month ago

Import bugfix/15102-have-a-show-passphrase-option-when-creating-persistence as a patch (refs: #15102, #10976)

Included changes:

- Allow the user to show the passphrase they're typing when creating
a new persistent volume (#15102).
- When saving persistence.conf or its backup, also run sync(1)
on its parent directory (#10976)

Snapshot taken at commit db83629400bffc70f99a19b1df6d2e6518c60f3a.

Revision 138c2ae3
Added by intrigeri about 1 month ago

Merge branch 'bugfix/15102-have-a-show-passphrase-option-when-creating-persistence' into devel (Closes: #15102)

History

#1 Updated by sajolida almost 2 years ago

#2 Updated by sajolida almost 2 years ago

  • Related to Feature #14548: Fix issues identified in the expert review done by jaster on our installation instructions added

#3 Updated by intrigeri almost 2 years ago

  • Description updated (diff)
  • Starter set to Yes

#4 Updated by touss about 2 months ago

  • Status changed from Confirmed to In Progress
  • Target version set to Tails_4.0
  • Feature Branch set to https://salsa.debian.org/touss-guest/persistence-setup/tree/bugfix/15102-have-a-show-passphrase-option-when-creating-persistence

#5 Updated by touss about 2 months ago

  • Assignee set to touss

#6 Updated by touss about 2 months ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (touss)

#7 Updated by intrigeri about 2 months ago

  • Assignee set to intrigeri

Thanks! I'll do my best to look at your branch by the end of the week, but note that I'm super busy until our 4.0 release.

#8 Updated by intrigeri about 2 months ago

Hi @touss!

First, thanks a lot for your first (right?) contribution to Tails!
In particular, we severely lack folks who feel comfortable going closer than 1km from our Perl code, so I'm very happy you're up to it. If you would like to do more work in this area, just say so, I can easily point you to some next candidate tasks :)

I have only one question about your branch: is there any specific reason why you've used Gtk3::Box->new("horizontal", 0) instead of the higher-level Gtk3::HBox class?

Building on top of your work, I've done a little bit of cleanup (mostly code style and indentation):
https://git.tails.boum.org/persistence-setup/log/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence
I would be glad if you could review these small changes of mine.

Meanwhile, I'm going to prepare a branch in tails.git that applies these changes, so I can build a Tails image with them and run our automated test suite on it.

#9 Updated by intrigeri about 2 months ago

  • Status changed from Needs Validation to In Progress

#10 Updated by intrigeri about 2 months ago

  • Status changed from In Progress to Needs Validation
  • Feature Branch changed from https://salsa.debian.org/touss-guest/persistence-setup/tree/bugfix/15102-have-a-show-passphrase-option-when-creating-persistence to bugfix/15102-have-a-show-passphrase-option-when-creating-persistence, persistence-setup:bugfix/15102-have-a-show-passphrase-option-when-creating-persistence

#11 Updated by intrigeri about 2 months ago

  • Assignee changed from intrigeri to touss

OK, this passes our test suite!

@touss, please let me know if you can review the small changes I made on top.
If we want to get this into Tails 4.0, the review & merge must be done by Wednesday, Oct 9.

If you can't do this review, it's OK, just let me know :)

#12 Updated by touss about 1 month ago

intrigeri wrote:

Hi @touss!

First, thanks a lot for your first (right?) contribution to Tails!
In particular, we severely lack folks who feel comfortable going closer than 1km from our Perl code, so I'm very happy you're up to it. If you would like to do more work in this area, just say so, I can easily point you to some next candidate tasks :)

I have only one question about your branch: is there any specific reason why you've used Gtk3::Box->new("horizontal", 0) instead of the higher-level Gtk3::HBox class?

Building on top of your work, I've done a little bit of cleanup (mostly code style and indentation):
https://git.tails.boum.org/persistence-setup/log/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence
I would be glad if you could review these small changes of mine.

Meanwhile, I'm going to prepare a branch in tails.git that applies these changes, so I can build a Tails image with them and run our automated test suite on it.

Hello, thanks for reviewing this code.
This is, in fact, my first conintrigeri wrote:

Hi @touss!

First, thanks a lot for your first (right?) contribution to Tails!
In particular, we severely lack folks who feel comfortable going closer than 1km from our Perl code, so I'm very happy you're up to it. If you would like to do more work in this area, just say so, I can easily point you to some next candidate tasks :)

I have only one question about your branch: is there any specific reason why you've used Gtk3::Box->new("horizontal", 0) instead of the higher-level Gtk3::HBox class?

Building on top of your work, I've done a little bit of cleanup (mostly code style and indentation):
https://git.tails.boum.org/persistence-setup/log/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence
I would be glad if you could review these small changes of mine.

Meanwhile, I'm going to prepare a branch in tails.git that applies these changes, so I can build a Tails image with them and run our automated test suite on it.

Hello, thanks for reviewing this code.
This is, in fact, my first contribution writing code for Free Software. it's been a very rich experience so far. I'm learning Perl, Git and GTK3 so that i can implement these tasks.
I would like to continue working on this area, so please point me to other tasks you have in mind.
There's no specific reason that i chose to use this code, should i change to Gtk3::HBox class to keep the code consistent?
I reviewed the code, what i should do to confirm the changes?

#13 Updated by intrigeri about 1 month ago

  • Status changed from Needs Validation to In Progress

Hi touss!

This is, in fact, my first contribution writing code for Free Software. it's been a very rich experience so far. I'm learning Perl, Git and GTK3 so that i can implement these tasks.

Wow, amazing! You've been doing a great job at it.

I would like to continue working on this area, so please point me to other tasks you have in mind.

Sure thing: see you on #7002 (same code base) or #6502 (another Perl + GTK code base).

There's no specific reason that i chose to use this code, should i change to Gtk3::HBox class to keep the code consistent?

Yes, please merge my branch into yours, do this (based on my branch), manually test the updated code, commit, push and reassign to me for review once you're happy with it.

I reviewed the code, what i should do to confirm the changes?

You just did :)

#14 Updated by touss about 1 month ago

Sorry, I just saw in the gtk+ reference manual that the GtkHBox has been deprecated:[[https://developer.gnome.org/gtk3/stable/GtkHBox.html]]
Should i keep Gtk3::Box->new("horizontal", 0) instead of the higher-level @Gtk3::HBox class?

#15 Updated by intrigeri about 1 month ago

Hi @touss!

Sorry, I just saw in the gtk+ reference manual that the GtkHBox have been deprecated:[[https://developer.gnome.org/gtk3/stable/GtkHBox.html]]

Oh, good catch, I was not aware of it. Thanks!

Should i keep Gtk3::Box->new("horizontal", 0) instead of the higher-level @Gtk3::HBox class?

Yes, you're absolutely right.

But then, I think we should pass the name of the orientation property ('GTK_ORIENTATION_HORIZONTAL'), which is public API, instead of its value ("horizontal"), which is a private implementation detail of GTK: https://developer.gnome.org/gtk3/stable/gtk3-Standard-Enumerations.html#GtkOrientation.

We already have an example of using a member of the GtkOrientation enumeration for similar purposes:

lib/Tails/Persistence/Step/Configure.pm:        $list_box->insert(Gtk3::Separator->new('GTK_ORIENTATION_HORIZONTAL'), -1);

Do you prefer doing this yourself (might give you a greater feeling of accomplishment, I don't know) or should I?

#16 Updated by touss about 1 month ago

Hello @intrigeri,

Do you prefer doing this yourself (might give you a greater feeling of accomplishment, I don't know) or should I?

If that's ok, i prefer that you do it.

#17 Updated by touss about 1 month ago

But then, I think we should pass the name of the orientation property ('GTK_ORIENTATION_HORIZONTAL'), which is public API, instead of its value ("horizontal")

I made this fix.Please review.

#18 Updated by touss about 1 month ago

  • Status changed from In Progress to Needs Validation
  • Assignee changed from touss to intrigeri

#19 Updated by intrigeri about 1 month ago

  • Status changed from Needs Validation to In Progress

#20 Updated by intrigeri about 1 month ago

  • Status changed from In Progress to Needs Validation

#21 Updated by intrigeri about 1 month ago

Hi @touss,

great, thanks! I've merged your branch into the master branch of persistence-setup.git and updated the corresponding topic branch in tails.git. If our CI is happy with it, I'll merge this and it'll make it into Tails 4.0~rc1. Congrats!

To save us some overhead, I've sneaked an extra, unrelated commit on top: https://git-tails.immerda.ch/persistence-setup/commit/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence and I would appreciate if you could review it. Possibly relevant info that might help:

  • The rationale for this change comes from #10976#note-67. I don't expect you to read that entire ticket. Let's just say that for years, we've been struggling to understand that bug, we've done a few changes that may improve the robustness of the "saving a new persistence.conf file", and it's not even clear whether they're sufficient and whether the bug is in persistence-setup itself. So this commit is merely yet another attempt at making this operation more robust. I don't know if it will make a difference. I'm reasonably convinced it won't do harm.
  • The config_file_path and backup_config_file_path attributes have type AbsPath, which comes from Types::Path::Tiny, that is they're effectively objects of the Path::Tiny class (with some additional checks that Types::Path::Tiny performs when setting a new value). So we can call https://metacpan.org/pod/Path::Tiny#parent on them.
  • I know that not everyone is comfortable with Modern Perl object oriented programming and it's easy to get lost in all the options. This program is using Moo.

#22 Updated by intrigeri about 1 month ago

  • Status changed from Needs Validation to Resolved
  • % Done changed from 0 to 100

#23 Updated by touss about 1 month ago

great, thanks! I've merged your branch into the master branch of persistence-setup.git and updated the corresponding topic branch in tails.git. If our CI is happy with it, I'll merge this and it'll make it into Tails 4.0~rc1. Congrats!

I am very happy to have made my first contribution to Tails. I've been meaning to contribute to a free software project for a while and it turns out that Tails has the best documentation for me.
thank you very much @intrigeri to guide me and for all the feedback you gave me. I expect to be able to do more work on Tails.

To save us some overhead, I've sneaked an extra, unrelated commit on top: https://git-tails.immerda.ch/persistence-setup/commit/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence and I would appreciate if you could review it. Possibly relevant info that might help:

Thanks for pointing this out to me, it's very interesting! I'll investigate further and try to learn more about it.

Also available in: Atom PDF