[PATCH 2/3] Fix issues in password validation. (#1014405)

Vratislav Podzimek vpodzime at redhat.com
Thu Oct 3 08:04:01 UTC 2013


On Wed, 2013-10-02 at 17:22 -0400, David Shea wrote:
> Redefined validatePassword so that it returns validity, strength and a
> reason all at once, removing the need to call pwquality more than once
> for a particular password.
> 
> Convert the password checking in the root password spoke to use
> GUICheck, similar to the checks in the user spoke.
> 
> Fix the handling of messages from libpwquality.
> 
> Fix the handling of the case where a user password is set by kickstart
> and overriden by the user.
> 
> Moved several of the password error strings into constants.
> 
> This commit also includes some changes that glade automatically made to
> the GtkLevelBar properties. The behavior of the LevelBars is still the
> same: some of the removed properties were duplicates, some of them were
> the default values, and some were not using values in a form that
> gladeui expects. The interface-requires change is because GtkLevelBar
> was added in Gtk 3.6.
I believe it would be quite hard to split this big patch into multiple
shorter ones, but try to do that next time. It makes searching for
things in the git history, reverting, review etc. much easier.

> ---
>  pyanaconda/constants.py                 |  12 ++
>  pyanaconda/ui/gui/spokes/password.glade |  16 +--
>  pyanaconda/ui/gui/spokes/password.py    | 227 ++++++++++++++++++--------------
>  pyanaconda/ui/gui/spokes/user.glade     |  17 +--
>  pyanaconda/ui/gui/spokes/user.py        | 109 +++++++--------
>  pyanaconda/ui/tui/spokes/__init__.py    |  38 +++---
>  pyanaconda/users.py                     |  85 +++++++-----
>  7 files changed, 280 insertions(+), 224 deletions(-)
> 
> diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py
> index 0d1956b..13440e2 100644
> --- a/pyanaconda/constants.py
> +++ b/pyanaconda/constants.py
> @@ -130,3 +130,15 @@ FIRSTBOOT_ENVIRON = "firstboot"
>  
>  # Tainted hardware
>  UNSUPPORTED_HW = 1 << 28
> +
> +# Password validation
> +PASSWORD_MIN_LEN = 6
> +PASSWORD_EMPTY_ERROR = _("The password is empty.")
> +PASSWORD_CONFIRM_ERROR_GUI = _("The passwords do not match.")
> +PASSWORD_CONFIRM_ERROR_TUI = _("The passwords you entered were different.  Please try again.")
> +PASSWORD_WEAK = _("The password you have provided is weak. You will have to press Done twice to confirm it.")
> +PASSWORD_WEAK_WITH_ERROR = _("The password you have provided is weak: %s. You will have to press Done twice to confirm it.")
> +PASSWORD_WEAK_CONFIRM = _("You have provided a weak password. Press Done again to use anyway.")
> +PASSWORD_WEAK_CONFIRM_WITH_ERROR = _("You have provided a weak password: %s. Press Done again to use anyway.")
> +
> +PASSWORD_STRENGTH_DESC = [_("Empty"), _("Weak"), _("Fair"), _("Good"), _("Strong")]
> diff --git a/pyanaconda/ui/gui/spokes/password.glade b/pyanaconda/ui/gui/spokes/password.glade
> index 37942eb..b57b7e4 100644
> --- a/pyanaconda/ui/gui/spokes/password.glade
> +++ b/pyanaconda/ui/gui/spokes/password.glade
> @@ -1,6 +1,6 @@
>  <?xml version="1.0" encoding="UTF-8"?>
>  <interface>
> -  <!-- interface-requires gtk+ 3.0 -->
> +  <!-- interface-requires gtk+ 3.6 -->
>    <!-- interface-requires AnacondaWidgets 1.0 -->
>    <object class="AnacondaSpokeWindow" id="passwordWindow">
>      <property name="startup_id">filler</property>
> @@ -87,7 +87,7 @@
>                          <property name="can_focus">True</property>
>                          <property name="visibility">False</property>
>                          <property name="invisible_char">●</property>
> -                        <signal name="changed" handler="_checkPassword" swapped="no"/>
> +                        <signal name="changed" handler="_updatePwQuality" swapped="no"/>
>                        </object>
>                        <packing>
>                          <property name="left_attach">1</property>
> @@ -103,7 +103,6 @@
>                          <property name="visibility">False</property>
>                          <property name="invisible_char">●</property>
>                          <property name="activates_default">True</property>
> -                        <signal name="changed" handler="_checkPassword" swapped="no"/>
>                        </object>
>                        <packing>
>                          <property name="left_attach">1</property>
> @@ -134,15 +133,10 @@
>                            <object class="GtkLevelBar" id="password_bar">
>                              <property name="visible">True</property>
>                              <property name="can_focus">False</property>
> -                            <property name="orientation">vertical</property>
> -                            <property name="spacing">2</property>
> -                            <property name="mode">GTK_LEVEL_BAR_MODE_DISCRETE</property>
> -                            <property name="min-value">0</property>
> -                            <property name="max-value">4</property>
> -                            <property name="orientation">GTK_ORIENTATION_HORIZONTAL</property>
> -                            <property name="value">2</property>
> -                            <property name="halign">fill</property>
>                              <property name="valign">center</property>
> +                            <property name="value">2</property>
> +                            <property name="max_value">4</property>
> +                            <property name="mode">discrete</property>
>                            </object>
>                            <packing>
>                              <property name="expand">True</property>
> diff --git a/pyanaconda/ui/gui/spokes/password.py b/pyanaconda/ui/gui/spokes/password.py
> index b828317..c64df56 100644
> --- a/pyanaconda/ui/gui/spokes/password.py
> +++ b/pyanaconda/ui/gui/spokes/password.py
> @@ -20,13 +20,16 @@
>  #
>  
>  from pyanaconda.i18n import _, N_
> -from pyanaconda.users import cryptPassword, validatePassword, checkPassword
> -from pwquality import PWQError
> +from pyanaconda.users import cryptPassword, validatePassword
>  
>  from pyanaconda.ui.gui.spokes import NormalSpoke
>  from pyanaconda.ui.gui.categories.user_settings import UserSettingsCategory
>  from pyanaconda.ui.common import FirstbootSpokeMixIn
>  
> +from pyanaconda.constants import PASSWORD_EMPTY_ERROR, PASSWORD_CONFIRM_ERROR_GUI,\
> +        PASSWORD_STRENGTH_DESC, PASSWORD_WEAK, PASSWORD_WEAK_WITH_ERROR,\
> +        PASSWORD_WEAK_CONFIRM, PASSWORD_WEAK_CONFIRM_WITH_ERROR
> +
>  __all__ = ["PasswordSpoke"]
>  
> 
> @@ -43,9 +46,6 @@ class PasswordSpoke(FirstbootSpokeMixIn, NormalSpoke):
>  
>      def __init__(self, *args):
>          NormalSpoke.__init__(self, *args)
> -        self._password = None
> -        self._error = False
> -        self._oldweak = None
>          self._kickstarted = False
>  
>      def initialize(self):
> @@ -54,6 +54,35 @@ class PasswordSpoke(FirstbootSpokeMixIn, NormalSpoke):
>          self.pw = self.builder.get_object("pw")
>          self.confirm = self.builder.get_object("confirm")
>  
> +        # Install the password checks:
> +        # - Has a password been specified?
> +        # - If a password has been specified and there is data in the confirm box, do they match?
> +        # - How strong is the password?
> +        # - Is there any data in the confirm box?
> +        self.add_check(self.pw, self._checkPasswordEmpty)
> +
> +        # The password confirmation needs to be checked whenever either of the password
> +        # fields change. Separate checks are created for each field so that edits on either
> +        # will trigger a new check and so that the last edited field will get focus when
> +        # Done is clicked. The checks are saved here so that either check can trigger the
> +        # other check in order to reset the status on both when either field is changed.
> +        # The check_data field is used as a flag to prevent infinite recursion.
> +        self._confirm_check = self.add_check(self.confirm, self._checkPasswordConfirm)
> +        self._password_check = self.add_check(self.pw, self._checkPasswordConfirm)
> +
> +        # Keep a reference for this check, since it has to be manually run for the
> +        # click Done twice check.
> +        self._pwStrengthCheck = self.add_check(self.pw, self._checkPasswordStrength)
> +
> +        self.add_check(self.confirm, self._checkPasswordEmpty)
> +
> +        # Counter for the click Done twice check override
> +        self._waivePasswordClicks = 0
> +
> +        # Password validation data
> +        self._pwq_error = None
> +        self._pwq_valid = True
> +
>          self._kickstarted = self.data.rootpw.seen
>          if self._kickstarted:
>              self.pw.set_placeholder_text(_("The password is set."))
> @@ -69,13 +98,11 @@ class PasswordSpoke(FirstbootSpokeMixIn, NormalSpoke):
>  
>      def refresh(self):
>          self.pw.grab_focus()
> -        self._checkPassword()
> +        self.pw.emit("changed")
>  
>      @property
>      def status(self):
> -        if self._error:
> -            return _("Error setting root password")
> -        elif self.data.rootpw.password:
> +        if self.data.rootpw.password:
>              return _("Root password is set")
>          elif self.data.rootpw.lock:
>              return _("Root account is disabled")
> @@ -88,10 +115,11 @@ class PasswordSpoke(FirstbootSpokeMixIn, NormalSpoke):
>                              if "wheel" in user.groups)
>  
>      def apply(self):
> -        if self._password is None and self._kickstarted:
> +        pw = self.pw.get_text()
> +        if (not pw) and (self._kickstarted):
>              return
>  
> -        self.data.rootpw.password = cryptPassword(self._password)
> +        self.data.rootpw.password = cryptPassword(self.pw.get_text())
>          self.data.rootpw.isCrypted = True
>          self.data.rootpw.lock = False
>  
> @@ -106,108 +134,115 @@ class PasswordSpoke(FirstbootSpokeMixIn, NormalSpoke):
>      def completed(self):
>          return bool(self.data.rootpw.password or self.data.rootpw.lock)
>  
> -    def _checkPassword(self, editable = None, data = None):
> -        """This method updates the password indicators according
> -        to the passwords entered by the user. It is called by
> -        the changed Gtk event handler.
> +    def _checkPasswordEmpty(self, editable, data):
> +        """Check whether a password has been specified at all."""
> +
> +        # If the password was set by kickstart, skip this check
> +        if self._kickstarted:
> +            return True
> +
> +        if not editable.get_text():
> +            if editable == self.pw:
> +                return PASSWORD_EMPTY_ERROR
> +            else:
> +                return PASSWORD_CONFIRM_ERROR_GUI
> +        else:
> +            return None
A suggestion for some future patch -- could those checks return some
constants? I see the same problem we had with the TUI input method here,
those None and True says nothing about their meaning.

> +
> +    def _checkPasswordConfirm(self, editable=None, reset_status=None):
> +        """Check whether the password matches the confirmation data."""
> +
> +        # This check is triggered by changes to either the password field or the
> +        # confirmation field. If this method is being run from a successful check
> +        # to reset the status, just return success
> +        if reset_status:
> +            return None
> +
> +        # Skip the check if no password is required
> +        if self._kickstarted:
> +            result = None
> +        elif self.confirm.get_text() and (self.pw.get_text() != self.confirm.get_text()):
> +            result = PASSWORD_CONFIRM_ERROR_GUI
> +        else:
> +            result = None
> +
> +        # If the check succeeded, reset the status of the other check object
> +        if result is None:
> +            if editable == self.confirm:
> +                self._password_check.update_check_status(check_data=True)
> +            else:
> +                self._confirm_check.update_check_status(check_data=True)
> +
> +        return result
> +
> +    def _updatePwQuality(self, editable=None, data=None):
> +        """Update the password quality information.
> +
> +           This function is called by the ::changed signal handler on the
> +           password field.
>          """
> -        try:
> -            strength = checkPassword(self.pw.get_text())
> -            _pwq_error = None
> -        except PWQError as e:
> -            _pwq_error = e.message
> -            strength = 0
> -
> -        if strength < 50:
> +
> +        pwtext = self.pw.get_text()
> +
> +        # Reset the counter used for the "press Done twice" logic
> +        self._waivePasswordClicks = 0
> +
> +        self._pwq_valid, strength, self._pwq_error = validatePassword(pwtext, "root")
> +
> +        if not pwtext:
> +            val = 0
> +        elif strength < 50:
>              val = 1
> -            text = _("Weak")
> -            self._error = _("The password you have provided is weak")
> -            if _pwq_error:
> -                self._error += ": %s. " % _pwq_error
> -            else:
> -                self._error += ". "
> -            self._error += _("You will have to press Done twice to confirm it.")
>          elif strength < 75:
>              val = 2
> -            text = _("Fair")
> -            self._error = False
>          elif strength < 90:
>              val = 3
> -            text = _("Good")
> -            self._error = False
>          else:
>              val = 4
> -            text = _("Strong")
> -            self._error = False
> -
> -        if not self.pw.get_text():
> -            val = 0
> -            text = _("Empty")
> -            self._error = _("The password is empty.")
> -        elif self.confirm.get_text() and self.pw.get_text() != self.confirm.get_text():
> -            self._error = _("The passwords do not match.")
> +        text = PASSWORD_STRENGTH_DESC[val]
>  
>          self.pw_bar.set_value(val)
>          self.pw_label.set_text(text)
>  
> -        self.clear_info()
> -        if self._error:
> -            self.set_warning(self._error)
> -            self.window.show_all()
> -            return False
> +    def _checkPasswordStrength(self, editable=None, data=None):
> +        """Update the error message based on password strength.
>  
> -        return True
> +           Convert the strength set by _updatePwQuality into an error message.
> +        """
>  
> -    def _validatePassword(self):
> -        # Do various steps to validate the password
> -        # sets self._error to an error string
> -        # Return True if valid, False otherwise
> -        self._error = False
>          pw = self.pw.get_text()
>          confirm = self.confirm.get_text()
>  
> -        if not pw and not confirm:
> -            if self._kickstarted:
> -                return True
> +        # Skip the check if no password is required
> +        if (not pw and not confirm) and self._kickstarted:
> +            return None
> +
> +        # Check for validity errors
> +        if (not self._pwq_valid) and (self._pwq_error):
> +            return self._pwq_error
> +
> +        pwstrenth = self.pw_bar.get_value()
I believe this should be pwstrength.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list