Abort the module execution when running w/o enough privileges #583

dgdavid · 2019-11-25T17:37:28Z

Problem

When yast-bootloader module is executed without enough privileges/permissions, it crash because an Errno::EACCES error when trying access to the grub.cfg file.

Click to show/hide an screenshot

Solution

Abort the execution nicely, giving human readable information to the user.

💡 Proposed message:

The module is running without enough privileges to perform all possible actions.

Cannot continue. Please, try again as root.

Tests

Tested manually in a running system.

When catch an Errno::EACCES trying to read the grub.cfg file.

coveralls · 2019-11-25T17:43:42Z

Coverage decreased (-0.05%) to 83.788% when pulling cf778fa on check_privileges into 4b0f30a on master.

dgdavid

This a working in progress pull request yet. However, I would like to have earlier opinions or suggestions. See my self pre-review / request for comments.

dgdavid · 2019-11-25T17:53:13Z

src/modules/Bootloader.rb

@@ -166,8 +166,14 @@ def Read

        ::Bootloader::BootloaderFactory.current = ::Bootloader::BootloaderFactory.proposed
        ::Bootloader::BootloaderFactory.current.propose
+      rescue ::Bootloader::InsufficientPrivileges => e


A self pre-review / Request for comments

I added this new exception just to follow the pattern used here (I mean, rescuing its own exception). But, to be honest, I'm not sure if

I like it

It is a good idea

Another way to do it without a custom exception is to rescue the Errno::ACCES here directly.

... rescue Errno::ACCES error_msg = _( "The module is running without enough privileges to perform all possible actions.\n\n" \ "Cannot continue. Please, try again as root." ) Yast2::Popup.show(error_msg, headline: :error) return false end ...

Shorter than the previous option but, again, I'm not sure if

I like it

It is a good idea

What do you think? Do you have any other suggestions?

Thanks in advance for your input!

I agree there is no need to invent a new type of exception that is only used to directly replace Errno::ACCESS.

The current custom exceptions defined in yast2-bootloader exist because they actually describe a pretty specific circumstance that is not covered by the standard ruby. But that's not the case with that new (and unnecessary) InsufficientPrivileges class.

Based on a code review comment: https://github.com/yast/yast-bootloader/pull/583/files#r350677467 Basically, catch the Errno:EACCES exception direclty in Bootloader.Read

ancorgs · 2019-11-26T12:53:15Z

Thinking about this more widely, I wonder whether we should just use Yast::Confirm.MustBeRoot at the beginning of the wizard/client instead of waiting for the permission problems to appear later.

I'd say that's what most traditional YaST modules do.

dgdavid · 2019-11-26T13:40:08Z

@ancorgs

I didn't know about Yast::Confirm.MustBeRoot 🙄 However, after some small tests, I don't think it could be a "real" solution for this issue. It only warning to the user that "if you continue, things might not work". But allows to continue.

Click to show/hide

And what I'd like to do is abort the execution. To me, it makes no sense to

YaST: "hey, if you continue something wrong could happen"

User: "it doesn't matter, let's Continue"

YaST: "ok, here you have a crash"

Click to show/hide a video

or even worst

YaST: "hey, if you continue something wrong could happen"

User: "it doesn't matter, let's Continue"

YaST: "you don't know what are you doing my dear user. Let's Abort"

Am I missing something? Sorry for the noise :(

ancorgs · 2019-11-26T13:46:50Z

Yes, you are right. The approach you implemented makes more sense from a user POV. But, please add a comment in that rescue block mentioning that not using MustBeRoot was a conscious decision.

Suggested during the review. See #583 (comment)

ancorgs · 2019-11-26T16:37:44Z

package/yast2-bootloader.changes

+Tue Nov 26 12:22:50 UTC 2019 - David Diaz <dgonzalez@suse.com>
+
+- Abort the execution when the module run without enough
+  permissions (related to bsc#1137688).


It's actually hard to see the relationship with bsc#1137688, but I trust you ;-)

Yes, it's hard also to me. It's what I said in a comment in the Trello card 🤷‍♂️

ancorgs · 2019-11-26T16:40:15Z

src/modules/Bootloader.rb

+        # wizard/client is not enough since it allows continue.
+
+        Yast2::Popup.show(
+          # TRANSLATORS: warn to the user that is running the module without enough permissions


Is really that useful to have a message to the translators to tell them what is already said in the string itself?

Something like TRANSLATORS: pop-up message, beware the line breaks would actually be useful, but the current one...

Answer: actually no.

Thanks for the suggestion :)

Suggested during the review. See #583 (comment)

ancorgs

LGTM

yast-bot · 2019-11-27T08:43:29Z

✔️ Public Jenkins job #40 successfully finished
✔️ Created OBS submit request #751215

yast-bot · 2019-11-27T08:44:12Z

✔️ Internal Jenkins job #14 successfully finished
✔️ Created IBS submit request #206200

dgdavid added 3 commits November 25, 2019 17:41

Add an exception for the lack of enough privileges

6b1efeb

Raise a Bootloader::InsufficientPrivileges exception

2606c38

When catch an Errno::EACCES trying to read the grub.cfg file.

Abort the module execution when not enough privileges

3701667

dgdavid force-pushed the check_privileges branch from 984ae49 to 3701667 Compare November 25, 2019 17:41

dgdavid commented Nov 25, 2019

View reviewed changes

dgdavid mentioned this pull request Nov 25, 2019

Abort when bootloader cannot be read yast/yast-tune#40

Merged

Do not use a custom exception for insufficient priveleges

1f45550

Based on a code review comment: https://github.com/yast/yast-bootloader/pull/583/files#r350677467 Basically, catch the Errno:EACCES exception direclty in Bootloader.Read

dgdavid force-pushed the check_privileges branch from a2c90c4 to 1f45550 Compare November 26, 2019 11:47

Update version and changelog

22af92e

dgdavid changed the title ~~[WIP] Abort the module execution when running w/o enough privileges~~ Abort the module execution when running w/o enough privileges Nov 26, 2019

dgdavid marked this pull request as ready for review November 26, 2019 12:34

dgdavid added a commit that referenced this pull request Nov 26, 2019

Add explanatory comment

931d6b1

Suggested during the review. See #583 (comment)

ancorgs reviewed Nov 26, 2019

View reviewed changes

Add explanatory comment

cf778fa

Suggested during the review. See #583 (comment)

dgdavid force-pushed the check_privileges branch from 931d6b1 to cf778fa Compare November 26, 2019 16:42

ancorgs approved these changes Nov 26, 2019

View reviewed changes

dgdavid merged commit 8630995 into master Nov 27, 2019

dgdavid deleted the check_privileges branch November 27, 2019 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort the module execution when running w/o enough privileges #583

Abort the module execution when running w/o enough privileges #583

dgdavid commented Nov 25, 2019 •

edited

coveralls commented Nov 25, 2019 •

edited

dgdavid left a comment

dgdavid Nov 25, 2019 •

edited

ancorgs Nov 26, 2019

ancorgs commented Nov 26, 2019

dgdavid commented Nov 26, 2019 •

edited

ancorgs commented Nov 26, 2019

ancorgs Nov 26, 2019

dgdavid Nov 26, 2019

ancorgs Nov 26, 2019

dgdavid Nov 26, 2019 •

edited

ancorgs left a comment

yast-bot commented Nov 27, 2019

yast-bot commented Nov 27, 2019

Abort the module execution when running w/o enough privileges #583

Abort the module execution when running w/o enough privileges #583

Conversation

dgdavid commented Nov 25, 2019 • edited

Problem

Solution

Tests

coveralls commented Nov 25, 2019 • edited

dgdavid left a comment

Choose a reason for hiding this comment

dgdavid Nov 25, 2019 • edited

Choose a reason for hiding this comment

A self pre-review / Request for comments

ancorgs Nov 26, 2019

Choose a reason for hiding this comment

ancorgs commented Nov 26, 2019

dgdavid commented Nov 26, 2019 • edited

ancorgs commented Nov 26, 2019

ancorgs Nov 26, 2019

Choose a reason for hiding this comment

dgdavid Nov 26, 2019

Choose a reason for hiding this comment

ancorgs Nov 26, 2019

Choose a reason for hiding this comment

dgdavid Nov 26, 2019 • edited

Choose a reason for hiding this comment

ancorgs left a comment

Choose a reason for hiding this comment

yast-bot commented Nov 27, 2019

yast-bot commented Nov 27, 2019

dgdavid commented Nov 25, 2019 •

edited

coveralls commented Nov 25, 2019 •

edited

dgdavid Nov 25, 2019 •

edited

dgdavid commented Nov 26, 2019 •

edited

dgdavid Nov 26, 2019 •

edited