Project

General

Profile

Feature #5684

Screen locker

Added by Tails almost 5 years ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
12/03/2014
Due date:
% Done:

100%

QA Check:
Pass
Feature Branch:
segfault:feature/5684-screen-locker
Type of work:
Code
Starter:
No
Affected tool:

Description

Tails is currently lacking a screen locker and this has been a frequent feature request. For example, as Tails is been adopted more and more by journalists, they want to be able to leave their computer unattended in their office to go to the toilets for a minute and have their screen locked.

This has also been mentioned as a requirement for Tails Server: https://tails.boum.org/blueprint/server_edition/

Team: sajolida, segfault, anonym

tails-lock.sh View (2.04 KB) segfault, 02/20/2016 09:33 PM

double password entry.png View (15.6 KB) sajolida, 02/21/2016 02:13 PM

tails_screen_locker.png View (10.7 KB) segfault, 04/01/2016 04:03 PM

triple-trouble.png View (11.5 KB) anonym, 11/28/2016 05:15 PM


Subtasks

Feature #5588: Do not create auto-login text consolesResolved

Feature #5589: Have a password-less amnesia account by defaultDuplicate

Feature #5878: Add a lock screen action to the system menuResolved

Feature #11256: Replace "Debian Live user" with a more descriptive full user nameResolved

Feature #6017: Ensure that the emergency shutdown works while the screen is lockedResolved

Feature #8385: Investigate if other distros are interested in our screen locking solutionResolved

Feature #8384: Investigate how other live/privacy distros do screen lockingResolved

Feature #8383: Research technical possibilities to implement a password prompt for screen lockingResolved

Feature #10970: Document screen lockerResolved

Bug #12449: Fix multiple lock screen buttonsResolved

Feature #15368: tails-screen-locker should display the full title of the dialogResolved

Feature #15369: Improve buttons of tails-screen-lockerResolved


Related issues

Related to Tails - Feature #5660: wheezy: hide lock screen launcher Resolved
Related to Tails - Feature #9569: Research available protections against rogue USB devices Confirmed 06/13/2015
Related to Tails - Bug #12098: Spurious screensaver activation while synchronizing the system clock Rejected 12/29/2016
Related to Tails - Feature #5799: Deactivate screensaver until time is set Confirmed
Related to Tails - Feature #14556: Show a suspend to RAM button in the status menu Confirmed 08/30/2017
Duplicated by Tails - Feature #7045: Lock screen icon in taskbar if admin password entered at startup Duplicate 04/08/2014
Duplicated by Tails - Feature #7719: Add a screen locker Duplicate 08/01/2014

Associated revisions

Revision 814d2b02 (diff)
Added by Tails over 3 years ago

Install and enable our custom shutdown-helper GNOME Shell extension (Closes: #8302, #5684 and #5878).

It adds a Lock Screen and Restart button to the GNOME user menu,
removes the press-Alt-to-turn-shutdown-button-into-Suspend
functionality, and makes Restart and Shutdown immediate, without
further user interaction.

It's based on laserb's gnome-shell-extension-suspend-button:
https://github.com/laserb/gnome-shell-extension-suspend-button

Revision e55f05d8 (diff)
Added by Tails over 3 years ago

Install and enable our custom shutdown-helper GNOME Shell extension (Closes: #8302, #5684 and #5878).

It adds a Lock Screen and Restart button to the GNOME user menu,
removes the press-Alt-to-turn-shutdown-button-into-Suspend
functionality, and makes Restart and Shutdown immediate, without
further user interaction.

It's based on laserb's gnome-shell-extension-suspend-button:
https://github.com/laserb/gnome-shell-extension-suspend-button

Revision 19fb574d (diff)
Added by anonym over 1 year ago

Make GNOME's default Lock Screen keyboard shortcut use our locker.

Refs: #5684

Revision 31c3cb82 (diff)
Added by anonym 7 months ago

Make GNOME's default Lock Screen keyboard shortcut use our locker.

Refs: #5684

Revision 3ec58a90 (diff)
Added by bertagaz 4 months ago

Fix wrong screenlocker ui path in POTFILE.skip.

It has been moved and so the build is broken when refresh-translation
comes in play.

Refs: #5684

Revision 834777f3 (diff)
Added by bertagaz 4 months ago

Fix wrong screenlocker ui path in POTFILE.skip.

It has been moved and so the build is broken when refresh-translation
comes in play.

Refs: #5684

Revision ec6b2626 (diff)
Added by bertagaz 4 months ago

Ship screen locker python dependencies into the ISO.

python3-pamy and python3-pydbus seems to be needed in Tails for the
screenlocker to work.

Refs: #5684

Revision 76f56ca6 (diff)
Added by bertagaz 4 months ago

Screen locker: install missing python3-pampy dependency.

Refs: #5684

Revision ac4eaba8 (diff)
Added by bertagaz 4 months ago

Clarify python3-systemd requirements.

Needed for both automated test suite and tails-screen-locker, let's not
remove it by mistake if one of them don't dep on it anymore.

Refs: #5684

Revision 091cd938
Added by bertagaz 4 months ago

Merge remote-tracking branch 'origin/feature/5684-screen-locker' into devel

Fix-committed: #5684

Revision eb31488d
Added by bertagaz 4 months ago

Merge remote-tracking branch 'origin/feature/5684-screen-locker' into devel

Fix-committed: #5684

Revision abcce04c (diff)
Added by bertagaz 4 months ago

Fix tails-screen-locker wording.

Refs: #5684

History

#1 Updated by Tails almost 5 years ago

  • Priority changed from Normal to Elevated

#2 Updated by Tails almost 5 years ago

  • Priority changed from Normal to Elevated

#3 Updated by Tails almost 5 years ago

  • Priority changed from Normal to Elevated

#4 Updated by matsa over 4 years ago

From the old forum thread, tested with Tails 0.21 :

The lockscreen function with gnome-screensaver is bypassable.
Here is how to reproduce:

  • Setup an administration password in the greeter
  • Install the package gnome-screensaver
  • Launch it with sudo -u amnesia gnome-screensaver
  • Lock your screen

Now, to unlock the screen without entering password:

  • Open a text-console with "Ctrl+Alt+F2"
  • Run the command killall -9 gnome-screensaver
  • Go back with "Ctrl+Alt+F7"

The screen is unlocked

#5 Updated by intrigeri over 4 years ago

  • Starter set to No

matsa wrote:

The lockscreen function with gnome-screensaver is bypassable.

That's why there's the #5588 subtask..

#6 Updated by intrigeri about 4 years ago

  • Duplicated by Feature #7045: Lock screen icon in taskbar if admin password entered at startup added

#7 Updated by BitingBird about 4 years ago

  • Subject changed from screen locker to Screen locker

#8 Updated by intrigeri almost 4 years ago

#9 Updated by mercedes508 almost 4 years ago

When doing the following:

  • Enable persistence.
  • Add gnome-screensaver to
    /live/persistence/TailsData_unlocked/live-additional-software.conf
  • Restart Tails.
  • Set an administrative password.
  • Start the screensaver by running:

    gnome-screensaver-command --activate

The screen is not locked. No password will be asked to
disable the screensaver.

The administrative passsword should be asked to exit the
screensaver.

This can be achieved by issuing the following command:

gsettings set org.gnome.desktop.lockdown disable-lock-screen false

I recommend adding it in /etc/gdm3/PostLogin/Default when
an administrative password is set. It should have no effect
when gnome-screensaver is not installed.

#10 Updated by anonym over 3 years ago

  • Target version set to Tails_2.0
  • Type of work changed from Code to Discuss

With shutdown-helper GNOME Shell extension (#8302) it is trivial to add the Lock Screen button when we're based on Debian Jessie. However, do we actually want to support screen locking?

Screen locking it's not a very strong defense for keeping the current session's data from someone that takes physical control of the computer running Tails. Still, it's better than nothing in a situation where you want to leave the session running among trusted people, but that you still do not want to have access to the session. Unfortunately users may get a false sense of security from this feature.

Next up, if we want to support it we probably want to avoid showing the Lock Screen button if no administrator password is set, since it may be confusing. That is quite doable by adding a dconf option (the original extension that shutdoen-helper is based on has code like that, so it can be borrowed from there) of wether to show it or not. And now we have hinted at another issue: by supporting screen locking we overload the administrator password Tails Greeter option with this functionality, which is non-obvious. The name may have to be changed, or the complete design of all this re-thought.

#11 Updated by micahflee over 3 years ago

anonym wrote:

With shutdown-helper GNOME Shell extension (#8302) it is trivial to add the Lock Screen button when we're based on Debian Jessie. However, do we actually want to support screen locking?

Screen locking it's not a very strong defense for keeping the current session's data from someone that takes physical control of the computer running Tails. Still, it's better than nothing in a situation where you want to leave the session running among trusted people, but that you still do not want to have access to the session. Unfortunately users may get a false sense of security from this feature.

I think supporting this feature is very important. I've worked in multiple office situations where employees use Tails, and sometimes they need to step out of their office to use the bathroom, or join a brief meeting, etc., but don't want to shut down the computer because they're in the middle of work and will be back in a minute. It makes me very concerned that there's nothing protecting the active session when this happens. I don't like even walking out of a room to refill my coffee with my screen unlocked. I think a lock screen feature would go a long way to making Tails more secure.

It might make sense if there were some warning or message on the lock screen that informed users that locking the screen is not a strong defense against physical attackers, and that you should shut down the computer instead if you'll be gone for very long.

#12 Updated by BitingBird over 3 years ago

The warning seems like a good compromise to me.

#13 Updated by intrigeri over 3 years ago

It might make sense if there were some warning or message on the lock screen that informed users that [...]

Good idea, but not sure how well it would work in practice: in my experience, at least GNOME Shell blanks the screen (DPMS or something) immediately after it's been locked, so users would actually not see this warning before they come back and want to unlock the session. Of course, then they've seen it once, and can remember these shortcomings next time. Is that good enough?

#14 Updated by u over 3 years ago

  • Related to Feature #8383: Research technical possibilities to implement a password prompt for screen locking added

#15 Updated by u over 3 years ago

  • Related to Feature #8384: Investigate how other live/privacy distros do screen locking added

#16 Updated by u over 3 years ago

  • Related to Feature #8385: Investigate if other distros are interested in our screen locking solution added

#17 Updated by u over 3 years ago

During the contributor meeting a proposal was made:

instead of using the admin password (that is, really, user password + sudo), a second password would make sense. In order to set this password, we would prompt the user on the first usage of the screen lock feature to set a screen lock password.

This password could also be made persistent, for persistence users. See related tasks.

#18 Updated by intrigeri over 3 years ago

  • Target version deleted (Tails_2.0)

#19 Updated by intrigeri over 3 years ago

  • Related to deleted (Feature #8385: Investigate if other distros are interested in our screen locking solution)

#20 Updated by intrigeri over 3 years ago

  • Related to deleted (Feature #8384: Investigate how other live/privacy distros do screen locking)

#21 Updated by intrigeri over 3 years ago

  • Related to deleted (Feature #8383: Research technical possibilities to implement a password prompt for screen locking)

#22 Updated by sajolida over 3 years ago

  • Type of work changed from Discuss to Research

#23 Updated by sajolida over 3 years ago

  • Description updated (diff)
  • Blueprint set to https://tails.boum.org/blueprint/screen_locker

#24 Updated by sajolida about 3 years ago

  • Description updated (diff)

#25 Updated by intrigeri about 3 years ago

  • Related to Feature #9569: Research available protections against rogue USB devices added

#26 Updated by sajolida almost 3 years ago

  • Description updated (diff)
  • Assignee set to sajolida
  • Target version set to 2017

#27 Updated by elypter almost 3 years ago

besides trying to make circumvention as difficult as possible it should also indicate when the system has been unlocked and locked again.
this could be done by:
-showing the time the screen has been locked and how long
-an incerementing number that increases wen the screen is being locked
-a unique randomly generated visually distinctive and easy to remember image. like random-art.org or terragen. if there exist high quality alternatives it could look quite elegant as side effect.

#28 Updated by sajolida almost 3 years ago

I'm not sure to understand your proposal. What concrete problem or attack are you trying to prevent with this?

#29 Updated by elypter almost 3 years ago

*the victim goes away from his desktop
*the attacker goes to his pc and circumvents the lockscreen and then does his stuff
*then the attacker locks the screen again
*the victim comes back and doesnt notice anything indicating the attack

if every lockscreen looks different though the victim would be able to see that the system has been locked again and knows that he has been spied upon and his system could be compromized.

#30 Updated by elypter almost 3 years ago

there could also be an indicator that shows if someone has tried to unlock the system by checking for keyboard, pointing device, camera, microphone, usb or accelerometer activity.

#31 Updated by sajolida almost 3 years ago

If the attacker manages to circumvent the screenlocking, in most scenarios the security of Tails is already defeated. Noticing it might not change much. And the attacker might as well be able to circumvent the techniques that you are proposing. So I'm really not convinced it's worth it both for us in terms of technical work and for the user in terms of added complexity of the interface.

I'd rather work on making the user understand that leaving their computer unattended is very risky and should be with very much care.

Also, if I were an attacker with unlimited technical power, I would instead insert malicious devices in the computer ports (USB, firewire, etc.) with exploits to access memory. And that would be detected by none of your tricks.

#32 Updated by elypter almost 3 years ago

sajolida wrote:

If the attacker manages to circumvent the screenlocking, in most scenarios the security of Tails is already defeated. And the attacker might as well be able to circumvent the techniques that you are proposing.

most of the time the threat model is not an attacker with unlimited technical power who wrote a tool specificly designed to circumvent the lock screen of a niche os. often its someone who looks over the shoulder of someone while hes typing the password or someone who tries to guess the password or someone who knows the password because the user trusts him.

Noticing it might not change much.

it might change a lot. he would know if he is in serious trouble or not. his life could depend on it.

I'd rather work on making the user understand that leaving their computer unattended is very risky and should be with very much care.

education and protection do not exclude each other.

Also, if I were an attacker with unlimited technical power, I would instead insert malicious devices in the computer ports (USB, firewire, etc.) with exploits to access memory. And that would be detected by none of your tricks.

plugging something in will trigger a notice on the lock screen. you could also disable plug and play during the lock.

#33 Updated by intrigeri over 2 years ago

Testing reports on #9408#note-1 might be of some interest here.

#34 Updated by segfault over 2 years ago

I took the grml-lock.sh script mentioned in the blueprint and modified it to work with Tails 2.0. It asks for a password if there is none set and then it locks the screen. It is ugly and I have to parse the passwd output in order to find out if it was successful because I can't use chpasswd, which is the tool to change passwords in batch mode but requires root.
There is currently a bug which causes the screensaver to not lock the screen if the password was only just set. I reported this in the GNOME bugtracker: https://bugzilla.gnome.org/show_bug.cgi?id=761969
The current workaround is to wait for 2 seconds after setting the password.

During the contributor meeting a proposal was made:

instead of using the admin password (that is, really, user password + sudo), a second password would make sense. In order to set this password, we would prompt the user on the first usage of the screen lock feature to set a screen lock password.

This password could also be made persistent, for persistence users. See related tasks.

I currently don't set a second password and I don't make it persistent, because I see no benefit in either. I read the meeting's notes and still don't see the point. Why would the user want to set a second password instead of just using the one he set?
And what would be the benefit of making the password persistent instead of just asking the first time the screen is locked after boot (if no password was set on the greeter)? I only see a drawback: The user could forget the password he set the last time he used Tails and would not be able to unlock the screen.

Testing reports on #9408#note-1 might be of some interest here.

I tried to log in on tty and it requires the password.

#35 Updated by sajolida over 2 years ago

I tested your script and it works as intended. Good job!

I did some most tests security wise (as I didn't want us to set up an administration password this way), and no it's not possible to sudo after using your script; note that otherwise we would have had a big problem since years :)

Now I wonder about the integration on the desktop. I think we should aim at triggering your script in the same ways as the screen locker would be triggered in a normal GNOME: at least through Super+L and from an icon in the system menu. Debian has a padlock icon in the system menu, right? Maybe we did something to remove it.

Regarding the UI, I wonder whether a single dialog with two password fields would be better than two dialogs with a single password field. The combine two passwords is maybe more common, see this screenshot from GNOME Disks.

Maybe also we don't need the first dialog with no user interaction and fit this in a single dialog with the combined password fields.

Then I'd have some rephrasing to do on the user visible strings, but maybe before someone should do a more in depth technical review... or you should work on the integration.

Assigning to anonym to maybe have a very quick first look to validate the approach. Then maybe segfault should refine the integration and UI.

#36 Updated by segfault over 2 years ago

  • Target version changed from 2017 to Tails_2.4

Oh, I forgot to add myself to the watchers of this ticket. I only saw your comment on this just now, sajolida.

Thanks for testing it. I will set the target version to something sooner than 2017, so that anonym hopefully will have a quick look at this soon.

#37 Updated by segfault about 2 years ago

Now I wonder about the integration on the desktop. I think we should aim at triggering your script in the same ways as the screen locker would be triggered in a normal GNOME: at least through Super+L and from an icon in the system menu. Debian has a padlock icon in the system menu, right? Maybe we did something to remove it.

I found what we did to remove the padlock button, it's the disable-lock-screen = true line in this file:

config/chroot_local-includes/etc/dconf/db/local.d/00_Tails_defaults

You can disable this setting in your Tails session via this command:

gsettings set org.gnome.desktop.lockdown disable-lock-screen false

Triggering the script through Super+L is trivial, since we only have to change the keyboard shortcut. But I did not yet find a way to change the behaviour of the padlock button in the status menu (that's how the GNOME folks refer to this menu btw.)

Still looking forward to some short feedback if this is going the right direction. Then I would spend some more time on the remaining integration issues.

#38 Updated by segfault about 2 years ago

  • Description updated (diff)

#39 Updated by segfault about 2 years ago

  • Type of work changed from Research to Code

I rewrote the script in Python, created a small GUI in glade to realize the password setting in a single dialog and created a GNOME extension to add a screen lock button to the status menu which invokes the Tails Screen Locker.

I've put everything in this repository:
https://gitlab.com/segfault_/tails_screen_locker

Please test it :)

#40 Updated by segfault about 2 years ago

I append a screenshot of the dialog.

#41 Updated by anonym about 2 years ago

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

So far I've discovered these blockers:

  1. The GUI is not translatable.
  2. In the GNOME Session menu, let's place the new "Lock Screen" button between the Settings and Restart buttons , so the Restart/Shutdown buttons remain the rightmost ones and more easily accessible when in a hurry (i.e. for the emergency shutdown use case).
  3. Instead of introducing yet another GNOME Shell plugin, let's add the Lock Screen button to our existing Emergency Shutdown Helper plugin. Have a look at config/chroot_local-includes/usr/share/gnome-shell/extensions/shutdown-helper@tails.boum.org/extension.js. Some pattern matching and copy-pasting should do it. :)
  4. Check passwd's man page: "passwd will reject any password which is not suitably complex" => a too short password causes your password setter to silently fail. You need to handle this case, and I think the correct way is not to try to replicate the password complexity check in your tiny application, but to check stderr of passwd on error and see if the specific error string is there. For this reason you'll want to execute passwd with an English (or "C") locale too.

#42 Updated by anonym about 2 years ago

  • Status changed from Confirmed to In Progress

I should also add that it looks nice! :) Btw, I really appreciate that you provided scripts to help setting it up in a live session in absence of a Tails Git branch that I can build.

So, segfault, do you think you'll have time to fix these remaining issues in time for Tails 2.4 (especially since you have the Tails server GSoC now, and also (IIRC) said you'd have less time for Tails in general for the next couple weeks)? To be clear, for this to make it into Tails 2.4 we'd need to merge something with no serious issues (i.e. at least all of the above issues resolved, barring perhaps the plugin thing) before 2016-05-25.

#43 Updated by segfault about 2 years ago

So far I've discovered these blockers:

1. The GUI is not translatable.

I will look into this later.

2. In the GNOME Session menu, let's place the new "Lock Screen" button between the Settings and Restart buttons , so the Restart/Shutdown buttons remain the rightmost ones and more easily accessible when in a hurry (i.e. for the emergency shutdown use case).

Right, I thought the same and already did that :) The lock button only appears right if the GNOME Shell plugin was just installed. Just reload the GNOME Shell (Alt F2 -> 'r') and the lock button will be left from the restart and shutdown buttons.

3. Instead of introducing yet another GNOME Shell plugin, let's add the Lock Screen button to our existing Emergency Shutdown Helper plugin. Have a look at /extension.js. Some pattern matching and copy-pasting should do it. :)

I already looked at the shutdown-helper extension and copied code from it to the screen lock extension. I'm not sure why merging the plugins would be better than having them separated. The benefit of separate plugins is that we can more easily remove single functionality, if we ever need to do this.

4. Check passwd's man page: "passwd will reject any password which is not suitably complex" => a too short password causes your password setter to silently fail. You need to handle this case, and I think the correct way is not to try to replicate the password complexity check in your tiny application, but to check stderr of passwd on error and see if the specific error string is there. For this reason you'll want to execute passwd with an English (or "C") locale too.

Yes, I noticed this too. But I decided to not handle this case but instead allow users to use insecure passwords for their screen lock. After all, the screen lock is not very secure itself, so why should we force users to think up secure passwords and possibly tell them multiple times that their password is not good enough before they can lock their screen?
This check can be disabled by removing the "obscure" option to pam_unix.so in /etc/pam.d/common-password. I propose we do this. Sorry I forgot to mention this before.

#44 Updated by segfault about 2 years ago

I should also add that it looks nice! :) Btw, I really appreciate that you provided scripts to help setting it up in a live session in absence of a Tails Git branch that I can build.

:)

So, segfault, do you think you'll have time to fix these remaining issues in time for Tails 2.4 (especially since you have the Tails server GSoC now, and also (IIRC) said you'd have less time for Tails in general for the next couple weeks)? To be clear, for this to make it into Tails 2.4 we'd need to merge something with no serious issues (i.e. at least all of the above issues resolved, barring perhaps the plugin thing) before 2016-05-25.

If you think removing the secure password check is fine, I will prepare a commit. What do you think would be the right way to do this? A new script in chroot_local-hooks with sed -i 's/pam_unix.so obscure/pam_unix.so/' /etc/pam.d/common-password?

I'm not sure yet what has to be done to make it translatable. I will take a look at other Tails applications to figure this out. If this turns out to be the only remaining blocker, I will take the time to do this in time for 2.4.

#45 Updated by sajolida about 2 years ago

Also make sure that the doc is updated. I don't mind doing this after the branch is merged for code.

#46 Updated by segfault about 2 years ago

I noticed that the screen lock button reappears right from the shutdown and restart buttons after clicking it. I fixed this with the newest commit in https://gitlab.com/segfault_/tails_screen_locker.

#47 Updated by anonym about 2 years ago

  • Target version changed from Tails_2.4 to Tails_2.5

#48 Updated by intrigeri almost 2 years ago

  • Target version changed from Tails_2.5 to Tails_2.6

#49 Updated by Dr_Whax almost 2 years ago

  • Description updated (diff)
  • Target version changed from Tails_2.6 to 2017

#50 Updated by segfault almost 2 years ago

  • Assignee changed from segfault to anonym
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch set to segfault:feature/5684-screen-locker
The branch https://gitlab.com/segfault_/tails.git feature/5684-screen-locker is ready for QA. It should work like this:
  • Display a lock button in the GNOME status menu, between the settings button and the restart button
    On click it should:
  • Ask for a password, if no password was set before (there should be no password security check anymore)
  • Just lock the screen, if a password was set before

#51 Updated by anonym almost 2 years ago

  • Assignee changed from anonym to segfault
  • QA Check changed from Ready for QA to Info Needed

Looks great, segfault!

If we get some user docs ready in time for 2.6, I'd (probably, see below) merge this. I would even consider merging this in time for 2.6~rc1 without docs if someone promises to deliver the docs before the final 2.6 release. I'm reassigning this ticket to you in hope that you manage something.

There is one potential UX issue: no locking will occur when GNOME blanks the screen after some extended period of no user activity. Try it by settings a lock screen password, and then run this as amnesia:

gsettings set org.gnome.desktop.session idle-delay 5

and wait for five seconds. I'm not sure whether this is a problem, and if it is, if it is a blocker. Any thoughts on this? I think I'd consider it a blocker to have this documented, if we keep this behavior.

#52 Updated by segfault almost 2 years ago

  • Assignee changed from segfault to anonym
  • QA Check changed from Info Needed to Ready for QA

I pushed a commit which makes the screen lock when the screensaver is activated (if a password was already set previously).

#53 Updated by sajolida almost 2 years ago

  • Target version changed from 2017 to Tails_2.6

Marking this for 2.6 until the status of this is clarified, see #5878#note-16.

#54 Updated by intrigeri almost 2 years ago

  • Target version changed from Tails_2.6 to Tails_2.9.1

(Acting as the backup RM, which I am for 2 more days. I'm sorry this was not announced anywhere.)

2.6 has been frozen since September 1, so it's too late to get new features in. Sorry! Thus moving to the next major release.

#55 Updated by intrigeri almost 2 years ago

anonym wrote:

If we get some user docs ready in time for 2.6, I'd (probably, see below) merge this. I would even consider merging this in time for 2.6~rc1 without docs if someone promises to deliver the docs before the final 2.6 release. I'm reassigning this ticket to you in hope that you manage something.

(FTR, this did not happen, hence the postponing.)

#56 Updated by anonym over 1 year ago

Your branch doesn't build, segfault. I've fixed the issues on the feature/5684-screen-locker in Tails main Git repo, which I recommend you fetch and look at.

I'm building an image now to test your recent changes.

#57 Updated by anonym over 1 year ago

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

Now I get two Lock Screen buttons in the GNOME "session" menu (or whatever it is called). I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?

#58 Updated by intrigeri over 1 year ago

I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?

+1

#59 Updated by intrigeri over 1 year ago

FWIW the topic branch currently fails to build on Jenkins. Do we need to merge an update from segfault's branch?

#60 Updated by segfault over 1 year ago

FWIW the topic branch currently fails to build on Jenkins. Do we need to merge an update from segfault's branch?

I will look into this soon

#61 Updated by segfault over 1 year ago

The branch builds fine after merging devel.

I didn't test the image yet though, so I can't say anything about the two Lock Screen buttons anonym reported.

#62 Updated by segfault over 1 year ago

I can't reproduce whatever caused anonym seeing two lock screen buttons.

#63 Updated by anonym over 1 year ago

segfault wrote:

I can't reproduce whatever caused anonym seeing two lock screen buttons.

It's still an issue, see the attached image which shows three lock screen buttons. At least all of them work. :)

#64 Updated by anonym over 1 year ago

anonym wrote:

segfault wrote:

I can't reproduce whatever caused anonym seeing two lock screen buttons.

It's still an issue, see the attached image which shows three lock screen buttons. At least all of them work. :)

During the same session I eventually ended up with four of them. I didn't do anything I can connect to this happening. Looking at your code and comparing it to the shutdown applet, I see some parts "missing", e.g. the stuff about this._menuOpenStateChangedId, but I fail to see why it should be related.

#65 Updated by anonym over 1 year ago

I've pushed two commit to our repo's version of the branch:

d52d946 Enter a name into the Icedove account configuration.
19fb574 Make GNOME's default Lock Screen keyboard shortcut use our locker.

Please review'n'merge, segfault! :)

#66 Updated by anonym over 1 year ago

All code looks good, but please move tails_screen_locker from:

/usr/lib/python3.4/dist-packages/

to
/usr/lib/python3/dist-packages/

The former is not used, and let's try to not (pretend to) depend on specific python versions. An other idea is to move it into Tails' pythonlib Git submodule in case you see any advantage in it.

So what remain before I can merge this branch is what I just said above + the "multiple screen locker icons" issue + the user docs (#10970).

#67 Updated by segfault over 1 year ago

I've pushed two commit to our repo's version of the branch:

I can't find your commits. Did you forget to push? The last commits are from September: https://git-tails.immerda.ch/tails/log/?h=feature/5684-screen-locker

#68 Updated by segfault over 1 year ago

  • Assignee changed from segfault to anonym
  • QA Check changed from Dev Needed to Info Needed

#69 Updated by anonym over 1 year ago

  • Assignee changed from anonym to segfault
  • QA Check changed from Info Needed to Dev Needed

(Force-)Pushed now. I also pushed two more commits:

850afd7 Use the LIVE_USER variable instead of 'amnesia'.
f70dae8 Make live-build hook into live-config hook.

These are in fact old commits (before this force-push) that I asked you to cherry-pick/merge into your branch, which you did not. Please try not to lose such feedback in the future -- it's part of the review process!

#70 Updated by segfault over 1 year ago

I also pushed two more commits:

850afd7 Use the LIVE_USER variable instead of 'amnesia'.
f70dae8 Make live-build hook into live-config hook.

These are in fact old commits (before this force-push) that I asked you to cherry-pick/merge into your branch, which you did not. Please try not to lose such feedback in the future -- it's part of the review process!

I did merge these commits into my branch, but in my branch they have the commit ids 0ceea16b and 5cae9c30:
https://gitlab.com/segfault_/tails/commit/0ceea16bbc945d4bc3223a130da82f51d96e2405
https://gitlab.com/segfault_/tails/commit/5cae9c30855a6178616b21f31ee17ffc4edf464e

I don't know why they have different id's than yours.

#71 Updated by anonym over 1 year ago

  • Target version changed from Tails_2.9.1 to Tails 2.10

#72 Updated by intrigeri over 1 year ago

  • Related to Bug #12098: Spurious screensaver activation while synchronizing the system clock added

#73 Updated by intrigeri over 1 year ago

If #12098 is fixed before the screen locker is completed, then we might want to re-enable the idle timeout whenever the time is not being synchronized.

#74 Updated by intrigeri over 1 year ago

  • Related to Feature #5799: Deactivate screensaver until time is set added

#75 Updated by anonym over 1 year ago

  • Target version changed from Tails 2.10 to Tails_2.11

#76 Updated by intrigeri over 1 year ago

  • Target version changed from Tails_2.11 to Tails_2.12

#77 Updated by intrigeri about 1 year ago

  • Target version changed from Tails_2.12 to 2017

segfault, next time you'll work on this, please:

  1. document what's left to be done (e.g. in case someone wants to help), preferably as subtasks; lack of clarity wrt. the status of this endeavour seems to be a problem on the end-user doc side, and I'd like to avoid us being in a catch-22 where the person who is responsible for the last thing to do is waiting for someone else (undefined) to do something else (undefined) first;
  2. set a target version you find realistic (or none at all, it's fine too); meanwhile I'm setting it back to the 2017 one (as decided at the summit), which conveys our goals better than postponing to next release multiple times in a row.

Thanks in advance!

Also, with my 3.0 RM hat on: if you want to see this in Tails 3.0, then something well tested, that takes into account all comments and important use cases, needs to be ready by May 15. But this can very well wait until the latest 2017 major release (3.2, 2017-10-03) or be postponed to next year if you prefer.

#78 Updated by segfault about 1 year ago

  • Target version changed from 2017 to Tails_3.2

intrigeri wrote:

segfault, next time you'll work on this, please:

  1. document what's left to be done (e.g. in case someone wants to help), preferably as subtasks; lack of clarity wrt. the status of this endeavour seems to be a problem on the end-user doc side, and I'd like to avoid us being in a catch-22 where the person who is responsible for the last thing to do is waiting for someone else (undefined) to do something else (undefined) first;

I think the only remaining blocker is that sometimes multiple lock screen buttons appear in the GNOME status menu. I just created the subtask #12449 for this.

  1. set a target version you find realistic (or none at all, it's fine too); meanwhile I'm setting it back to the 2017 one (as decided at the summit), which conveys our goals better than postponing to next release multiple times in a row.

[...]
Also, with my 3.0 RM hat on: if you want to see this in Tails 3.0, then something well tested, that takes into account all comments and important use cases, needs to be ready by May 15. But this can very well wait until the latest 2017 major release (3.2, 2017-10-03) or be postponed to next year if you prefer.

I think 3.2 is realistic. I have little free time until summer. I don't think I will be able to spend time on this in time for 3.0. The little time I can spend on Tails currently I prefer to spend on getting a Tails Server beta release done.

#79 Updated by intrigeri about 1 year ago

I think 3.2 is realistic. I have little free time until summer. I don't think I will be able to spend time on this in time for 3.0. The little time I can spend on Tails currently I prefer to spend on getting a Tails Server beta release done.

Sounds very reasonable. Thanks for the update!

#80 Updated by segfault 10 months ago

  • Related to Feature #14556: Show a suspend to RAM button in the status menu added

#81 Updated by segfault 9 months ago

  • Target version changed from Tails_3.2 to Tails_3.5

#82 Updated by segfault 7 months ago

  • Assignee changed from segfault to anonym
  • QA Check changed from Dev Needed to Info Needed

I rebased the branch on current stable, resolved the merge conflicts, and replaced the invocation of gnome-screenlocker-command with a dbus call (because gnome-screenlocker-command is not installed in current Tails).

The screen locker now works again in my VM. I am still not able to reproduce the multiple buttons issue anonym experienced. Could you check if you can still reproduce this, anonym?

Re: #57:

Now I get two Lock Screen buttons in the GNOME "session" menu (or whatever it is called). I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?

I am still not convinced that it would be better to merge the screen locker extension with the shutdown-helper extension. They have different purposes, so I think it is better design to keep them separate, so that they can be maintained and debugged separately, and removed separately if one of them becomes obsolete.
I don't see how an additional extension adds more complexity than adding the same functionality to an existing extension.

Except for discussing whether we want to merge the extensions, the only things I see that are blocking this are the translations of the passphrase prompt (I had to temporarily add the tail_screen_locker python package to POTFILES.skip), and the documentation (I don't like doing documentation, but if this turns out to be the last blocker in time for 3.4, I will definitely do it).

#83 Updated by intrigeri 7 months ago

I suspect this should be postponed to the next major release.

#84 Updated by segfault 7 months ago

  • Target version changed from Tails_3.5 to Tails_3.6

I suspect this should be postponed to the next major release.

Ah, right, I forgot that 3.4 is not a major release anymore.

#85 Updated by segfault 7 months ago

  • Target version changed from Tails_3.6 to Tails_3.7

#86 Updated by anonym 7 months ago

segfault wrote:

I rebased the branch on current stable, resolved the merge conflicts, and replaced the invocation of gnome-screenlocker-command with a dbus call (because gnome-screenlocker-command is not installed in current Tails).

The screen locker now works again in my VM. I am still not able to reproduce the multiple buttons issue anonym experienced. Could you check if you can still reproduce this, anonym?

I've only given it a short try so far without reproducing it. To be continued!

Some things I've noticed:

  • If I start gnome-shell-extension-prefs and disable the "Screen Locker", the button is not removed. The button is removed as it should when the "Shutdown Helper" is disabled, so something is wrong in your rewrite. Could this be related to the "two buttons" bug?
  • When you have locked the screen and are at the lock screen, clicking its GNOME menu still shows the screen lock button, which is a bit confusing and ugly (but not so serious, but still).
  • Multiple instances of the password setting prompt can be open at the same time (no big deal).

Re: #57:

Now I get two Lock Screen buttons in the GNOME "session" menu (or whatever it is called). I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?

I am still not convinced that it would be better to merge the screen locker extension with the shutdown-helper extension. They have different purposes, so I think it is better design to keep them separate, so that they can be maintained and debugged separately, and removed separately if one of them becomes obsolete.

I agree that "different purposes" => "different program" is most often the ideal organization, but, as always, blindly following it is not a good idea. The reality here is more complex: there's an inherent arbitrariness where you draw the line for "different purpose". I mean, I could argue that the purpose of the unified plugin I propose is to adapt GNOME's session menu into what Tails needs, i.e. all changes, which is one purpose. Or we could go the opposite way and also split "Shutdown Helper" into "Power off helper" and "Reboot helper". So, in reality it is a good idea to define the purpose with things like code duplication (which splitting increases, unifying decreases), complexity from interaction (see below), etc, in mind.

I don't see how an additional extension adds more complexity than adding the same functionality to an existing extension.

We now have two different entities mutating the same state. That is a much more complex interaction than the same entity doing all the mutating with full control. I mean, it seems there already is (or was) a hard-to-reproduce bug about this, which, from experience, indicates a race or other parallelism (i.e. "program interaction") issue.

#87 Updated by segfault 7 months ago

anonym wrote:

segfault wrote:

I rebased the branch on current stable, resolved the merge conflicts, and replaced the invocation of gnome-screenlocker-command with a dbus call (because gnome-screenlocker-command is not installed in current Tails).

The screen locker now works again in my VM. I am still not able to reproduce the multiple buttons issue anonym experienced. Could you check if you can still reproduce this, anonym?

I've only given it a short try so far without reproducing it. To be continued!

Some things I've noticed:

  • If I start gnome-shell-extension-prefs and disable the "Screen Locker", the button is not removed. The button is removed as it should when the "Shutdown Helper" is disabled, so something is wrong in your rewrite. Could this be related to the "two buttons" bug?

This is fixed in my branch now.

  • When you have locked the screen and are at the lock screen, clicking its GNOME menu still shows the screen lock button, which is a bit confusing and ugly (but not so serious, but still).

This is also fixed.

  • Multiple instances of the password setting prompt can be open at the same time (no big deal).

Not sure about this one. Do you think it is worth to create a pid/lock file for this?

Re: #57:

Now I get two Lock Screen buttons in the GNOME "session" menu (or whatever it is called). I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?

I am still not convinced that it would be better to merge the screen locker extension with the shutdown-helper extension. They have different purposes, so I think it is better design to keep them separate, so that they can be maintained and debugged separately, and removed separately if one of them becomes obsolete.

I agree that "different purposes" => "different program" is most often the ideal organization, but, as always, blindly following it is not a good idea. The reality here is more complex: there's an inherent arbitrariness where you draw the line for "different purpose". I mean, I could argue that the purpose of the unified plugin I propose is to adapt GNOME's session menu into what Tails needs, i.e. all changes, which is one purpose. Or we could go the opposite way and also split "Shutdown Helper" into "Power off helper" and "Reboot helper". So, in reality it is a good idea to define the purpose with things like code duplication (which splitting increases, unifying decreases), complexity from interaction (see below), etc, in mind.

I don't see how an additional extension adds more complexity than adding the same functionality to an existing extension.

We now have two different entities mutating the same state. That is a much more complex interaction than the same entity doing all the mutating with full control. I mean, it seems there already is (or was) a hard-to-reproduce bug about this, which, from experience, indicates a race or other parallelism (i.e. "program interaction") issue.

Ok, I understand the argument about mutating the same state. So now I did unify the two extensions, and named it "Status Menu Helper".

#88 Updated by anonym 6 months ago

  • Assignee changed from anonym to segfault
  • Target version changed from Tails_3.7 to Tails_3.6
  • QA Check changed from Info Needed to Dev Needed

I've tested a build successfully!

I don't like that we have moved the lock screen button to the end of the row, where users have been trained for years to find the emergency shutdown button. Please let the Restart and Shutdown buttons remain in the two last positions!

If you fix the above problem, I'd like to merge this. I actually don't think we even have to block on #10970 (user docs) cause this feature is so self-explanatory, and I cannot come up with any negative UX issues that really requires this feature to be documented. Nevertheless, I've got the ball rolling on those tickets, so we should get the docs any way.

I also have some nitpicky stuff:

From ad1572c863d20549d6f409b6c300b882111711a3:

+ "description": "Adds separate buttons for 'Restart', 'Shutdown', and 'Lock Screen, and 'Lock Screen' to the GNOME user menu, and all actions are immediate without further user verification.",

  • There is a "'Lock Screen, and " too many. :)
  • The "all actions are immediate" part is not true for 'Lock screen'.

I suggest: "Adds separate buttons for the 'Restart', 'Shutdown', and 'Lock Screen' actions to the GNOME user menu."

+ "name": "Status-Menu-Helper",

These hyphens must have been added by mistake when you did the (probably automatic) renaming.

GNOME refers to this menu as the "user menu" (soure) so I think we should stick with "user menu" (which we actually already have in the meta data (description) of the extension). So it should be "User Menu Helper". The "Helper" in "Shutdown Helper" made sense when the extension was about helping users to quickly shutdown, but in "User/Status Menu Helper" the "Helper" part feels out of place. I don't think there is a good name based on functionality any more, so let's just rename it to what it is: Tails User Menu

What do you think?

segfault wrote:

anonym wrote:

  • If I start gnome-shell-extension-prefs and disable the "Screen Locker", the button is not removed. The button is removed as it should when the "Shutdown Helper" is disabled, so something is wrong in your rewrite. Could this be related to the "two buttons" bug?

This is fixed in my branch now.

  • When you have locked the screen and are at the lock screen, clicking its GNOME menu still shows the screen lock button, which is a bit confusing and ugly (but not so serious, but still).

This is also fixed.

Thanks!

  • Multiple instances of the password setting prompt can be open at the same time (no big deal).

Not sure about this one. Do you think it is worth to create a pid/lock file for this?

Forget about it. I mostly said it in case it was convenient in Python (for instance, in Ruby (I think it was) there's a package that you simply import to enforce one instance per user).

Target version changed from Tails_3.6 to Tails_3.7

I'm reverting this; Tails 3.6 (2018-03-13) is the next major release.

#89 Updated by segfault 6 months ago

anonym wrote:

I've tested a build successfully!

I don't like that we have moved the lock screen button to the end of the row, where users have been trained for years to find the emergency shutdown button. Please let the Restart and Shutdown buttons remain in the two last positions!

Makes sense, fixed in 2d03b1314e380bc4d874108a7499a07755d6b91e.

If you fix the above problem, I'd like to merge this. I actually don't think we even have to block on #10970 (user docs) cause this feature is so self-explanatory, and I cannot come up with any negative UX issues that really requires this feature to be documented. Nevertheless, I've got the ball rolling on those tickets, so we should get the docs any way.

I also have some nitpicky stuff:

From ad1572c863d20549d6f409b6c300b882111711a3:

+ "description": "Adds separate buttons for 'Restart', 'Shutdown', and 'Lock Screen, and 'Lock Screen' to the GNOME user menu, and all actions are immediate without further user verification.",

  • There is a "'Lock Screen, and " too many. :)
  • The "all actions are immediate" part is not true for 'Lock screen'.

I suggest: "Adds separate buttons for the 'Restart', 'Shutdown', and 'Lock Screen' actions to the GNOME user menu."

Oops. Fixed in 8d2208617df829448ccfd2706428dd43b2bc514e.

+ "name": "Status-Menu-Helper",

These hyphens must have been added by mistake when you did the (probably automatic) renaming.

GNOME refers to this menu as the "user menu" (soure) so I think we should stick with "user menu" (which we actually already have in the meta data (description) of the extension). So it should be "User Menu Helper". The "Helper" in "Shutdown Helper" made sense when the extension was about helping users to quickly shutdown, but in "User/Status Menu Helper" the "Helper" part feels out of place. I don't think there is a good name based on functionality any more, so let's just rename it to what it is: Tails User Menu

What do you think?

I agree that the "Helper" part should be removed. But I think it was only called user menu until 3.10, when they integrated the status icons. Since then it is called "status area" in the docs [1][2], and (more often) "status menu" in commit messages and tickets. So I would argue that we should name it "Tails Status Menu" or maybe "Tails Status Area" instead.

[1] https://help.gnome.org/misc/release-notes/3.10/
[2] https://wiki.gnome.org/Design/OS/SystemStatus

  • Multiple instances of the password setting prompt can be open at the same time (no big deal).

Not sure about this one. Do you think it is worth to create a pid/lock file for this?

Forget about it. I mostly said it in case it was convenient in Python (for instance, in Ruby (I think it was) there's a package that you simply import to enforce one instance per user).

Ok. There does not seem to be something like that in Python - OTOH, using a pid file would also not be hard.

#90 Updated by segfault 6 months ago

  • Assignee changed from segfault to anonym
  • QA Check changed from Dev Needed to Info Needed

#91 Updated by segfault 6 months ago

  • QA Check changed from Info Needed to Ready for QA

I just pushed some commits which bring multiple improvements, including the "single instance" feature.

#92 Updated by intrigeri 5 months ago

  • Assignee changed from anonym to bertagaz

#93 Updated by bertagaz 5 months ago

  • Assignee changed from bertagaz to segfault
  • QA Check changed from Ready for QA to Dev Needed

I'm having troubles understanding the status of your branch on gitlab. I can't seem to merge it from/into stable without having to resolve weird conflicts. Would you mind preparing this branch so that it merges correctly? Also if it's targeting 3.6, this branch should be based on devel. Could be good to rebase it on this one.

#94 Updated by anonym 5 months ago

bertagaz: FWIW, I have reviewed the branch until 8a4f0f046b5432b8bbb3a149c9545a8c418ecaf2 (i.e. the state of feature/5684-screen-locker in Tails main Git repo).

bertagaz wrote:

I'm having troubles understanding the status of your branch on gitlab. I can't seem to merge it from/into stable without having to resolve weird conflicts. Would you mind preparing this branch so that it merges correctly? Also if it's targeting 3.6, this branch should be based on devel. Could be good to rebase it on this one.

FYI, segfault has merged devel so this is now fixed. OTOH, since he didn't reassign the branch back to you, he might not be done. *flies away*

#95 Updated by segfault 5 months ago

anonym wrote:

FYI, segfault has merged devel so this is now fixed. OTOH, since he didn't reassign the branch back to you, he might not be done. *flies away*

Yes, I merged devel, but I was not finished. My merge somehow reintroduced features/step_definitions/icedove.rb, which I now removed from the commit and force pushed. I think it is ready for review now, but I didn't test it myself after the merge. I'm currently building it in order to test it later.

@bertagaz: Thanks for working on this. I will reassign this to you after I successfully built and tested the merged branch.

#96 Updated by segfault 5 months ago

  • Assignee changed from segfault to bertagaz
  • QA Check changed from Dev Needed to Ready for QA

segfault wrote:

@bertagaz: Thanks for working on this. I will reassign this to you after I successfully built and tested the merged branch.

Sorry, my build setup is broken and I can't build any images currently, so I couldn't test this myself after merging.

#97 Updated by intrigeri 5 months ago

Sorry, my build setup broken and I can't build any images currently

FWIW I can only build with the noram option currently.

#98 Updated by anonym 4 months ago

bertagaz, this ticket has mostly been my responsibility, and it only landed on your plate because of my priorities. I feel like it should be on my plate still, which also probably will be more efficient use of our collective time given my previous familiarity with the branch. If you want to give it back to me, please do so ASAP (before 2018-02-22) so I can plan accordingly!

#99 Updated by bertagaz 4 months ago

  • Assignee changed from bertagaz to segfault
  • QA Check changed from Ready for QA to Info Needed

anonym wrote:

bertagaz, this ticket has mostly been my responsibility, and it only landed on your plate because of my priorities. I feel like it should be on my plate still, which also probably will be more efficient use of our collective time given my previous familiarity with the branch. If you want to give it back to me, please do so ASAP (before 2018-02-22) so I can plan accordingly!

It should be ok hopefully. Thanks to your help I made good progress these last days, thanks!

Segfault, I force pushed your rebased branch in Tails main repo. I had to fix two issues: the build of this branch was failing due to refresh-translations complaining (should be fixed with 3ec58a90424ff2799f0c50066f90f5dcfb31b202), and for some reasons the rebase you made contained b67a1c3af963255f22e1d447b642551df9b57d18, which is not part of devel, so I've reverted it with a8d8f1a8bf6b8221998540443b818b2db375cf6a

Jenkins should start to build and test it soon.

Meanwhile, would you mind having a look at the diff of this branch with devel on immerda's Tails git repo and tell me if the patch looks like what you wanted? If so, please reassign to me.

#100 Updated by segfault 4 months ago

bertagaz wrote:

Segfault, I force pushed your rebased branch in Tails main repo. I had to fix two issues: the build of this branch was failing due to refresh-translations complaining (should be fixed with 3ec58a90424ff2799f0c50066f90f5dcfb31b202)

That seems correct, thanks!

and for some reasons the rebase you made contained b67a1c3af963255f22e1d447b642551df9b57d18, which is not part of devel, so I've reverted it with a8d8f1a8bf6b8221998540443b818b2db375cf6a

That commit was actually pushed by anonym to this branch, and should be there. See #11256.

Jenkins should start to build and test it soon.

Meanwhile, would you mind having a look at the diff of this branch with devel on immerda's Tails git repo and tell me if the patch looks like what you wanted? If so, please reassign to me.

The diff contains some unrelated changes, for example the aufs submodule is removed and the kernel version in config/amnesia is changed. I merged devel myself and cherry-picked your fix for the refresh-translations (3ec58a90424ff2799f0c50066f90f5dcfb31b202). Now the diff looks fine.

After building and testing the image, I noticed that I forgot to install a dependency (python3-pydbus), which I did now in commit 47c465117124aaa39369459b118ac08aeaa1499d.

I will build and test again tomorrow.

#101 Updated by bertagaz 4 months ago

segfault wrote:

bertagaz wrote:

and for some reasons the rebase you made contained b67a1c3af963255f22e1d447b642551df9b57d18, which is not part of devel, so I've reverted it with a8d8f1a8bf6b8221998540443b818b2db375cf6a

That commit was actually pushed by anonym to this branch, and should be there. See #11256.

Oops, right, missed that in the comments of this ticket.

Meanwhile, would you mind having a look at the diff of this branch with devel on immerda's Tails git repo and tell me if the patch looks like what you wanted? If so, please reassign to me.

The diff contains some unrelated changes, for example the aufs submodule is removed and the kernel version in config/amnesia is changed. I merged devel myself and cherry-picked your fix for the refresh-translations (3ec58a90424ff2799f0c50066f90f5dcfb31b202). Now the diff looks fine.

Ok, I'll take your HEAD as the newest one. I was hacking around myself and pushed a commit to fix the dependencies. See below.

After building and testing the image, I noticed that I forgot to install a dependency (python3-pydbus), which I did now in commit 47c465117124aaa39369459b118ac08aeaa1499d.

To get the screen locker to work, I remember also having to install the python3-pampy package.

That's what I've pushed in ec6b26261b017b1146f541ce82a3b51ed59d6e9c and I'm building now locally. As said above I'll push your HEAD in the main repo as soon as Jenkins will have build my ISO with python3-pampy.

I will build and test again tomorrow.

With both this packages installed, my tests of the screen locker are positive. Automatic and triggered locking works nicely. Indeed that means I've only tested the use case where the sudo password is set though, as I had to install this packages. :)

I'll test tomorrow what happens without the sudo password with all deps.

#102 Updated by bertagaz 4 months ago

bertagaz wrote:

I'll test tomorrow what happens without the sudo password with all deps.

So I did some more manual tests with an ISO containing both missing deps I've added (python3-pydbus, python3-pampy).

When the administration password is set in the greeter it still works nicely. :)

But when it's not, the screenlocker password dialog is ineffective. Clicking either "Cancel" or "Ok" does not seem to do anything. I'm not sure how to debug that.

#103 Updated by segfault 4 months ago

bertagaz wrote:

To get the screen locker to work, I remember also having to install the python3-pampy package.

Right, pampy was also missing.

That's what I've pushed in ec6b26261b017b1146f541ce82a3b51ed59d6e9c and I'm building now locally. As said above I'll push your HEAD in the main repo as soon as Jenkins will have build my ISO with python3-pampy.

I can't find ec6b26261b017b1146f541ce82a3b51ed59d6e9c on the immerda.ch Tails repo.

But when it's not, the screenlocker password dialog is ineffective. Clicking either "Cancel" or "Ok" does not seem to do anything. I'm not sure how to debug that.

Oh, ok, I will test it right now.

#104 Updated by bertagaz 4 months ago

segfault wrote:

bertagaz wrote:

To get the screen locker to work, I remember also having to install the python3-pampy package.

Right, pampy was also missing.

Ack, pushed 76f56ca6809170cf30db90ae13a465a4d020b4fe then.

That's what I've pushed in ec6b26261b017b1146f541ce82a3b51ed59d6e9c and I'm building now locally. As said above I'll push your HEAD in the main repo as soon as Jenkins will have build my ISO with python3-pampy.

I can't find ec6b26261b017b1146f541ce82a3b51ed59d6e9c on the immerda.ch Tails repo.

Expected, as said I've forced pushed your branch later on and removed this commit from there.

#105 Updated by segfault 4 months ago

bertagaz wrote:

But when it's not, the screenlocker password dialog is ineffective. Clicking either "Cancel" or "Ok" does not seem to do anything. I'm not sure how to debug that.

Should be fixed by 77a038e9efe482ca049f5c38437896efd0d2445c. Rebuilding now to test once more. Sorry about the bugs, I didn't have a working build setup when I implemented this.

#106 Updated by bertagaz 4 months ago

segfault wrote:

Should be fixed by 77a038e9efe482ca049f5c38437896efd0d2445c. Rebuilding now to test once more. Sorry about the bugs, I didn't have a working build setup when I implemented this.

Now the dialog box works and the screen gets locked. yay! I've pushed your fix in the main repo branch.

However, when run manually I get that when tails-screen-locker exits:

amnesia@amnesia:~$ LANG=en_US DEBUG=1 /usr/local/bin/tails-screen-locker 
ERROR:root:g-io-error-quark: Timeout was reached (24)
Traceback (most recent call last):
  File "/usr/local/bin/tails-screen-locker", line 34, in lock_screen
    SessionBus().get("org.gnome.ScreenSaver").Lock(timeout=5)
  File "/usr/lib/python3/dist-packages/pydbus/proxy_method.py", line 75, in __call__
    0, timeout_to_glib(timeout), None).unpack()
GLib.GError: g-io-error-quark: Timeout was reached (24)

The screen locking works fine though.

Now that it basically works, I need to get through the whole comments one more time, and finish code reviewing.

#107 Updated by segfault 4 months ago

However, when run manually I get that when tails-screen-locker exits:

amnesia@amnesia:~$ LANG=en_US DEBUG=1 /usr/local/bin/tails-screen-locker 
ERROR:root:g-io-error-quark: Timeout was reached (24)
Traceback (most recent call last):
  File "/usr/local/bin/tails-screen-locker", line 34, in lock_screen
    SessionBus().get("org.gnome.ScreenSaver").Lock(timeout=5)
  File "/usr/lib/python3/dist-packages/pydbus/proxy_method.py", line 75, in __call__
    0, timeout_to_glib(timeout), None).unpack()
GLib.GError: g-io-error-quark: Timeout was reached (24)

Yeah, the reason why I added this timeout in the first place is that sometimes this dbus call doesn't return, even though the locking was already done. So without the timeout, the process would just keep on running in the background. I think this way it is fine.

#108 Updated by bertagaz 4 months ago

  • Assignee changed from segfault to bertagaz
  • QA Check changed from Info Needed to Ready for QA

segfault wrote:

Yeah, the reason why I added this timeout in the first place is that sometimes this dbus call doesn't return, even though the locking was already done. So without the timeout, the process would just keep on running in the background. I think this way it is fine.

Ok, fair enough.

Regarding the comments, seems everything's fine and there's no leftovers. Only some code review left!

#109 Updated by bertagaz 4 months ago

  • Assignee changed from bertagaz to segfault
  • QA Check changed from Ready for QA to Info Needed

bertagaz wrote:

segfault wrote:

Yeah, the reason why I added this timeout in the first place is that sometimes this dbus call doesn't return, even though the locking was already done. So without the timeout, the process would just keep on running in the background. I think this way it is fine.

Ok, fair enough.

Regarding the comments, seems everything's fine and there's no leftovers. Only some code review left!

Done for code reviewing. I'm excited, we're close releasing this finally!

I have one remak though: there are two calls to print() here:

77         if p.returncode != 0:
78             print("passwd stdout: %s" % out)
79             print("passwd stderr: %s" % err)

But the script has logging functionnality. Why not rather redirect that prints to the logs?

#110 Updated by segfault 4 months ago

bertagaz wrote:

bertagaz wrote:

segfault wrote:

Yeah, the reason why I added this timeout in the first place is that sometimes this dbus call doesn't return, even though the locking was already done. So without the timeout, the process would just keep on running in the background. I think this way it is fine.

Ok, fair enough.

Regarding the comments, seems everything's fine and there's no leftovers. Only some code review left!

Done for code reviewing. I'm excited, we're close releasing this finally!

Yay \o/

I have one remak though: there are two calls to print() here:

[...]

But the script has logging functionnality. Why not rather redirect that prints to the logs?

I don't think that would make a difference, because I didn't add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr. But I think it would actually be helpful to have errors logged to the journal, so in addition to your proposal, I added a journal handler in commit 72f96f042e684c55df7509670862092bb5146897. I also catch the timeout exception and log all uncaught exceptions.

#111 Updated by intrigeri 4 months ago

I don't think that would make a difference, because I didn't add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr.

I'm curious why stderr does not land into the Journal already.

#112 Updated by segfault 4 months ago

  • Assignee changed from segfault to bertagaz
  • QA Check changed from Info Needed to Ready for QA

intrigeri wrote:

I don't think that would make a difference, because I didn't add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr.

I'm curious why stderr does not land into the Journal already.

Because journald doesn't log stderr of all processes, it does that only for services.

#113 Updated by bertagaz 4 months ago

  • QA Check changed from Ready for QA to Pass

segfault wrote:

intrigeri wrote:

I don't think that would make a difference, because I didn't add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr.

I'm curious why stderr does not land into the Journal already.

Because journald doesn't log stderr of all processes, it does that only for services.

I've tested without the last patch catching up uncaught exceptions, and tails-screen-locker exceptions (#5684#note-107 or introduced fake ones) do ends up in the journal already. So it seems in this case we don't need this patch. Let's drop it from the branch HEAD and merge this branch for 3.6. Screen locker powa \o/.

#114 Updated by intrigeri 4 months ago

Because journald doesn't log stderr of all processes, it does that only for services.

I think it depends on how processes are wired (FD passed when starting them). E.g. last time I checked, stdout+stderr of everything started within the GNOME session ended up in the Journal.

#115 Updated by segfault 4 months ago

bertagaz wrote:

segfault wrote:

intrigeri wrote:

I don't think that would make a difference, because I didn't add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr.

I'm curious why stderr does not land into the Journal already.

Because journald doesn't log stderr of all processes, it does that only for services.

I've tested without the last patch catching up uncaught exceptions, and tails-screen-locker exceptions (#5684#note-107 or introduced fake ones) do ends up in the journal already.

That's strange, because my tests using *76c2c3936f.iso have a different result. When I add fake exceptions to tails-screen-locker, they are only printed to stderr, but don't appear in the journal. The only error in the journal related to the screen locking is gdm-password complaining that it can't find pam_gnome_keyring.so.

#116 Updated by segfault 4 months ago

intrigeri wrote:

Because journald doesn't log stderr of all processes, it does that only for services.

I think it depends on how processes are wired (FD passed when starting them). E.g. last time I checked, stdout+stderr of everything started within the GNOME session ended up in the Journal.

So if I open two gnome-terminals and in the one I do sudo journalctl -f and in the other one I do echo foo 1>&2, I should see the message printed by journalctl, right? This neither works on my Debian Sid nor Tails 3.5.

#117 Updated by segfault 4 months ago

segfault wrote:

intrigeri wrote:

Because journald doesn't log stderr of all processes, it does that only for services.

I think it depends on how processes are wired (FD passed when starting them). E.g. last time I checked, stdout+stderr of everything started within the GNOME session ended up in the Journal.

So if I open two gnome-terminals and in the one I do sudo journalctl -f and in the other one I do echo foo 1>&2, I should see the message printed by journalctl, right? This neither works on my Debian Sid nor Tails 3.5.

Ah, it does work if the script is started by gnome-session!

#118 Updated by segfault 4 months ago

segfault wrote:

That's strange, because my tests using *76c2c3936f.iso have a different result. When I add fake exceptions to tails-screen-locker, they are only printed to stderr, but don't appear in the journal. The only error in the journal related to the screen locking is gdm-password complaining that it can't find pam_gnome_keyring.so.

So actually it does log to the journal if I execute the script via the lock button or the keyboard shortcut, and not start it in a terminal.

#119 Updated by bertagaz 4 months ago

  • Status changed from In Progress to Fix committed

segfault wrote:

segfault wrote:

That's strange, because my tests using *76c2c3936f.iso have a different result. When I add fake exceptions to tails-screen-locker, they are only printed to stderr, but don't appear in the journal. The only error in the journal related to the screen locking is gdm-password complaining that it can't find pam_gnome_keyring.so.

So actually it does log to the journal if I execute the script via the lock button or the keyboard shortcut, and not start it in a terminal.

Yes it does go into the journal when started by Gnome, but not from the terminal. Seems completely reasonable like that, so I've done as intended, and merged without the last commit.

#120 Updated by bertagaz 4 months ago

  • Assignee deleted (bertagaz)

Damn, forgot to remove me... again.

#121 Updated by bertagaz 4 months ago

  • % Done changed from 89 to 100

#122 Updated by intrigeri 4 months ago

  • Status changed from Fix committed to In Progress

(One subtask is still open so this can't be closed as-is.)

#123 Updated by segfault 4 months ago

  • Assignee set to bertagaz
  • QA Check changed from Pass to Ready for QA

There is another issue: The .po files don't contain the screen locker's strings. I fixed this in 4a17e312ce00983d48bcb0d0ff0ca27f121dc44a and 71ca8b982afcf066cec6a70b1c1906c661fb7512. The refresh-translations doesn't support Glade files currently, so I decided to remove the screen-locker.ui file and add the widgets in code in the tails-screen-locker script.

#124 Updated by intrigeri 4 months ago

  • Assignee changed from bertagaz to segfault
  • QA Check changed from Ready for QA to Info Needed

There is another issue: The .po files don't contain the screen locker's strings. I fixed this in 4a17e312ce00983d48bcb0d0ff0ca27f121dc44a and 71ca8b982afcf066cec6a70b1c1906c661fb7512. The refresh-translations doesn't support Glade files currently, so I decided to remove the screen-locker.ui file and add the widgets in code in the tails-screen-locker script.

Three questions/comments:

  • Adding Glade support to refresh-translations seems quite easy and IMO less risky than a code change at this stage of the release cycle. Besides, in general I prefer Glade files over hard-coding the GUI in code, because UI designers can much more easily edit Glade files.
  • Have you verified that translations do get applied despite the fact the text domain is "Tails" (gettext.install("Tails")) and not "tails"? If not, let's discuss this on XMPP today as planned yesterday :)
  • Do we need 72f96f042e684c55df7509670862092bb5146897 in the end? I thought we had decided against it but I didn't re-read the history of this ticket.

#125 Updated by segfault 4 months ago

intrigeri wrote:

There is another issue: The .po files don't contain the screen locker's strings. I fixed this in 4a17e312ce00983d48bcb0d0ff0ca27f121dc44a and 71ca8b982afcf066cec6a70b1c1906c661fb7512. The refresh-translations doesn't support Glade files currently, so I decided to remove the screen-locker.ui file and add the widgets in code in the tails-screen-locker script.

Three questions/comments:

  • Adding Glade support to refresh-translations seems quite easy and IMO less risky than a code change at this stage of the release cycle.

I agree that it seems quite easy to add Glade support to refresh-translations. But I already wrote and tested the code change, I think now it will be less work to use it.

Besides, in general I prefer Glade files over hard-coding the GUI in code, because UI designers can much more easily edit Glade files.

At least I learned that I can achieve what I want a lot faster using PyGObject and devhelp than Glade.

  • Have you verified that translations do get applied despite the fact the text domain is "Tails" (gettext.install("Tails")) and not "tails"? If not, let's discuss this on XMPP today as planned yesterday :)

It does work, but we can still change it for consistency.

Right, I pushed a0fd727ed22455092028ba8a041f9aaebe2eae0b to revert it.

#126 Updated by intrigeri 4 months ago

  • Assignee changed from segfault to bertagaz
  • QA Check changed from Info Needed to Ready for QA

OK! bertagaz, please make sure you refresh-translations and test with a locale that has at least one of tails-screen-locker's strings translated.

#127 Updated by bertagaz 4 months ago

  • Assignee changed from bertagaz to segfault
  • QA Check changed from Ready for QA to Dev Needed

intrigeri wrote:

OK! bertagaz, please make sure you refresh-translations and test with a locale that has at least one of tails-screen-locker's strings translated.

Ok, I've done that, (refresh_translations + translate the tails-screen-locker strings in one language, commit and build), but the password window does not appear to be translated. :/

I've tried with gettext.install("tails") too, but the ui still does not get translated. Not sure what's the problem here.

Also it seems this line is not necessary anymore:

UI_FILE = "/usr/share/tails/screenlocker.ui" 

#128 Updated by segfault 4 months ago

  • Assignee changed from segfault to bertagaz
  • QA Check changed from Dev Needed to Ready for QA

bertagaz wrote:

intrigeri wrote:

OK! bertagaz, please make sure you refresh-translations and test with a locale that has at least one of tails-screen-locker's strings translated.

Ok, I've done that, (refresh_translations + translate the tails-screen-locker strings in one language, commit and build), but the password window does not appear to be translated. :/

I've tried with gettext.install("tails") too, but the ui still does not get translated. Not sure what's the problem here.

Sorry, I misunderstood how gettext works. It should work now with commit d69a47bbd4231758ef5c68ff22793d828b85720d.

Also it seems this line is not necessary anymore:

[...]

Right, fixed in cf70a19fbf25057730587a4dc5e542af5df33f4d.

#129 Updated by bertagaz 4 months ago

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

We fixed that, so it's merged in devel. Good catch about the missing translation!

#130 Updated by sajolida 4 months ago

  • Assignee set to segfault
  • QA Check changed from Pass to Info Needed

I finally got to see the screen locker in action: I love it!

The current dialog is:

Set a screen lock password
==========================

New password:     [          ]
Confirm password: [          ]

[Cancel] [Lock]

Am I still on time to propose to rephrase it a bit?

Proposed version:

Configure a password to unlock the screen
=========================================

Password:        [          ]
Verify password: [          ]

[Cancel] [Lock Screen]

Comments:

  • "screen lock password": It's kind of a noun blob. Making it more explicit can only help our international audience and translators.
  • ... and then the password is actually the password to unlock the screen, not to lock it.
  • "New password" → "Password": There's no old password so "new" is not needed.
  • "Confirm password" → "Verify password": GNOME Disks says "Confirm" but "Settings → Users" says "Verify". But I don't care much :)
  • "Lock" → "Lock Screen": being more explicit here cannot hurt.

#131 Updated by bertagaz 4 months ago

  • Status changed from In Progress to Fix committed

#132 Updated by sajolida 4 months ago

Actually, to be consistent with the wording we have to set up an administration password in Tails Greeter we should write:

Set up a password to unlock the screen
======================================

Password:   [          ]
Confirm:    [          ]

[Cancel] [Lock Screen]

#133 Updated by bertagaz 4 months ago

  • Status changed from Fix committed to In Progress
  • Assignee deleted (segfault)
  • QA Check changed from Info Needed to Pass

sajolida wrote:

Actually, to be consistent with the wording we have to set up an administration password in Tails Greeter we should write:

[...]

We're supposed to be frozen, but given that for some unrelated reasons I'm a bit late, I'll push your new wording.

#134 Updated by bertagaz 4 months ago

bertagaz wrote:

We're supposed to be frozen, but given that for some unrelated reasons I'm a bit late, I'll push your new wording.

Updated with abcce04c551650bac9d46d89040be01e12bca78d

#135 Updated by intrigeri 4 months ago

We're supposed to be frozen

Arguably these could be seen as bugfixes that can be applied post-RC :)

#136 Updated by bertagaz 4 months ago

intrigeri wrote:

We're supposed to be frozen

Arguably these could be seen as bugfixes that can be applied post-RC :)

Sure, but there was still time to push that as I didn't really start the release process at that moment. So that's ok :)

#137 Updated by pablonatalino 4 months ago

bertagaz wrote:

intrigeri wrote:

We're supposed to be frozen

This happened sometimes with me, the button light on / off works and a small sound from the cooler seems like the computer is working, but the keyboard, screen, usb and hdmi ports do not work anymore. Anyway tails-screen-locker did me very much ;) :) ;)

I wrote for whisperback but in Portuguese, since the whisperback instrusions were in Portuguese.

tails-screen-locker = ;)

#138 Updated by bertagaz 3 months ago

  • Target version changed from Tails_3.6 to Tails_3.7

#139 Updated by sajolida 3 months ago

bertagaz: Why can't we close this ticket?

#140 Updated by bertagaz 3 months ago

  • Status changed from In Progress to Resolved

Ooops you're right, it can be closed I don't know why I missed that.

#141 Updated by pablonatalino 3 months ago

The issue of closing the laptop lid also happened to me on windows, so in my case it should be a specific problem of my device

Also available in: Atom PDF