URL: https://github.com/freeipa/freeipa/pull/1582
Author: flo-renaud
 Title: #1582: User must not be able to delete his last active otp token
Action: opened

PR body:
"""
Fix and unit test for the issue. When an OTP token is the last active token, 
the user should not be allowed to delete its token if 'otp' is the only allowed 
authentication method.

Fixes:
https://pagure.io/freeipa/issue/7012

"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/1582/head:pr1582
git checkout pr1582
From aeddfdb345b9e86c481cc4ed1e4e2772457279e9 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud <f...@redhat.com>
Date: Wed, 14 Feb 2018 13:56:08 +0100
Subject: [PATCH 1/2] User must not be able to delete his last active otp token

The 389-ds plugin for OTP last token is performing data initialization
in its ipa_otp_lasttoken_init method, which is wrong according to
the Plug-in Guide:
> For example, the init function should not attempt to perform an
> internal search or other internal operation, because the all of
> the subsystems are not up and running during the init phase.

This init method fills a structure containing the configuration of
allowed authentication types. As the method is called too early, the
method does not find any suffix and leaves the structure empty.
Subsequent calls find an empty structure and take the default values
(for authentication methods, the default is 1 = password).

Because of that, the code consider that the global configuration defines
password authentication method, and in this case it is allowed to delete
a user's last otp token.

The fix implements a SLAPI_PLUGIN_START_FN method that will be called
when 389-ds is ready to initialize the plugin data, ensuring that the
structure is properly initialized.

Fixes:
https://pagure.io/freeipa/issue/7012
---
 .../ipa-otp-lasttoken/ipa_otp_lasttoken.c          | 32 ++++++++++++++++------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c b/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c
index a085a3a328..b7a2ba7f01 100644
--- a/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c
+++ b/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c
@@ -50,6 +50,7 @@
 #define OTP_CONTAINER "cn=otp,%s"
 
 static struct otp_config *otp_config;
+void *ipa_otp_lasttoken_plugin_id;
 
 static bool entry_is_token(Slapi_Entry *entry)
 {
@@ -255,6 +256,17 @@ static int postop_init(Slapi_PBlock *pb)
     return ret;
 }
 
+/* Init data structs */
+static int ipa_otp_lasttoken_start(Slapi_PBlock *pb)
+{
+    /* NOTE: We never call otp_config_fini() from a destructor. This is because
+     *       it may race with threaded requests at shutdown. This leak should
+     *       only occur when the DS is exiting, so it isn't a big deal.
+     */
+    otp_config = otp_config_init(ipa_otp_lasttoken_plugin_id);
+    return LDAP_SUCCESS;
+}
+
 int ipa_otp_lasttoken_init(Slapi_PBlock *pb)
 {
     static const Slapi_PluginDesc preop_desc = {
@@ -264,20 +276,24 @@ int ipa_otp_lasttoken_init(Slapi_PBlock *pb)
         "Protect the user's last active token"
     };
 
-    Slapi_ComponentId *plugin_id = NULL;
     int ret = 0;
 
-    ret |= slapi_pblock_get(pb, SLAPI_PLUGIN_IDENTITY, &plugin_id);
+    ret |= slapi_pblock_get(pb, SLAPI_PLUGIN_IDENTITY,
+                            &ipa_otp_lasttoken_plugin_id);
     ret |= slapi_pblock_set(pb, SLAPI_PLUGIN_VERSION, SLAPI_PLUGIN_VERSION_01);
     ret |= slapi_pblock_set(pb, SLAPI_PLUGIN_DESCRIPTION, (void *) &preop_desc);
     ret |= slapi_register_plugin("betxnpreoperation", 1, __func__, preop_init,
-                                 PLUGIN_NAME " betxnpreoperation", NULL, plugin_id);
+                                 PLUGIN_NAME " betxnpreoperation", NULL,
+                                 ipa_otp_lasttoken_plugin_id);
     ret |= slapi_register_plugin("postoperation", 1, __func__, postop_init,
-                                 PLUGIN_NAME " postoperation", NULL, plugin_id);
-    ret |= slapi_register_plugin("internalpostoperation", 1, __func__, intpostop_init,
-                                 PLUGIN_NAME " internalpostoperation", NULL, plugin_id);
+                                 PLUGIN_NAME " postoperation", NULL,
+                                 ipa_otp_lasttoken_plugin_id);
+    ret |= slapi_register_plugin("internalpostoperation", 1, __func__,
+                                 intpostop_init,
+                                 PLUGIN_NAME " internalpostoperation", NULL,
+                                 ipa_otp_lasttoken_plugin_id);
+    ret |= slapi_pblock_set(pb, SLAPI_PLUGIN_START_FN,
+                            (void *)ipa_otp_lasttoken_start);
 
-    /* NOTE: leak otp_config on process exit. */
-    otp_config = otp_config_init(plugin_id);
     return ret;
 }

From 67df89b4bb36c0c7279af9e89972cbad6c4ca016 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud <f...@redhat.com>
Date: Wed, 14 Feb 2018 14:06:48 +0100
Subject: [PATCH 2/2] 389-ds OTP lasttoken plugin: Add unit test

Add a xmlrpc test checking that a user cannot delete his last
OTP token.

Related to
https://pagure.io/freeipa/issue/7012
---
 ipatests/test_xmlrpc/test_otptoken_plugin.py | 173 +++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)
 create mode 100644 ipatests/test_xmlrpc/test_otptoken_plugin.py

diff --git a/ipatests/test_xmlrpc/test_otptoken_plugin.py b/ipatests/test_xmlrpc/test_otptoken_plugin.py
new file mode 100644
index 0000000000..2a152464bf
--- /dev/null
+++ b/ipatests/test_xmlrpc/test_otptoken_plugin.py
@@ -0,0 +1,173 @@
+#
+# Copyright (C) 2018  FreeIPA Contributors see COPYING for license
+#
+
+
+"""
+Test the otptoken plugin.
+"""
+
+from __future__ import print_function
+
+import pytest
+
+from ipalib import api, errors
+from ipatests.util import change_principal, unlock_principal_password
+from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test
+from ipatests.test_xmlrpc.tracker.user_plugin import UserTracker
+
+
+user_password = u'userSecretPassword123'
+
+
+@pytest.fixture
+def user(request):
+    tracker = UserTracker(name=u'user_for_otp_test',
+                          givenname=u'Test', sn=u'User for OTP')
+    return tracker.make_fixture(request)
+
+
+def id_function(arg):
+    """
+    Return a label for the test parameters.
+
+    The params can be:
+    - the global config (list containing ipauserauthtypes)
+      in this case we need to extract the 'disabled' auth type to evaluate
+      whether user setting override is allowed
+      Example: [u'disabled', u'otp'] will return a label noOverride-otp
+               [u'otp', u'password']                    otp+password
+    - the user config (list containing ipauserauthtypes)
+    - the expected outcome (boolean True if delete should be allowed)
+    """
+
+    if isinstance(arg, list):
+        # The arg is a list, need to extract the override flag
+        labels = list()
+        if u'disabled' in arg:
+            labels.append('noOverride')
+
+        label = 'default'
+        if arg:
+            without_override = [item for item in arg if item != u'disabled']
+            if without_override:
+                label = '+'.join(without_override)
+        labels.append(label)
+
+        return "-".join(labels)
+
+    if isinstance(arg, bool):
+        return "allowed" if arg else "forbidden"
+
+    return 'default'
+
+
+class TestDeleteLastOtpToken(XMLRPC_test):
+
+    @pytest.mark.parametrize(
+        "globalCfg,userCfg,allowDelLast", [
+            # When Global config is not set and prevents user override,
+            # it is possible to delete last token
+            ([u'disabled'],  None, True),
+            ([u'disabled'], [u'otp'], True),
+            ([u'disabled'], [u'password'], True),
+            ([u'disabled'], [u'password', u'otp'], True),
+            # When Global config is not set and allows user override,
+            # the userCfg applies
+            # Deletion is forbidden only when usercfg = otp only
+            (None,  None, True),
+            (None, [u'otp'], False),
+            (None, [u'password'], True),
+            (None, [u'password', u'otp'], True),
+            # When Global config is set to otp and prevents user override,
+            # it is forbidden to delete last token
+            ([u'disabled', u'otp'], None, False),
+            ([u'disabled', u'otp'], [u'otp'], False),
+            ([u'disabled', u'otp'], [u'password'], False),
+            ([u'disabled', u'otp'], [u'password', u'otp'], False),
+            # When Global config is set to otp and allows user override,
+            # the userCfg applies
+            # Deletion is forbidden when usercfg = otp only or usercfg not set
+            ([u'otp'], None, False),
+            ([u'otp'], [u'otp'], False),
+            ([u'otp'], [u'password'], True),
+            ([u'otp'], [u'password', u'otp'], True),
+            # When Global config is set to password and prevents user override,
+            # it is possible to delete last token
+            ([u'disabled', u'password'], None, True),
+            ([u'disabled', u'password'], [u'otp'], True),
+            ([u'disabled', u'password'], [u'password'], True),
+            ([u'disabled', u'password'], [u'password', u'otp'], True),
+            # When Global config is set to password and allows user override,
+            # the userCfg applies
+            # Deletion is forbidden when usercfg = otp only
+            ([u'password'], None, True),
+            ([u'password'], [u'otp'], False),
+            ([u'password'], [u'password'], True),
+            ([u'password'], [u'password', u'otp'], True),
+            # When Global config is set to password+otp and prevents user
+            # override, it is possible to delete last token
+            ([u'disabled', u'password', u'otp'], None, True),
+            ([u'disabled', u'password', u'otp'], [u'otp'], True),
+            ([u'disabled', u'password', u'otp'], [u'password'], True),
+            ([u'disabled', u'password', u'otp'], [u'password', u'otp'], True),
+            # When Global config is set to password+otp and allows user
+            # override, the userCfg applies
+            # Deletion is forbidden when usercfg = otp only
+            ([u'password', u'otp'], None, True),
+            ([u'password', u'otp'], [u'otp'], False),
+            ([u'password', u'otp'], [u'password'], True),
+            ([u'password', u'otp'], [u'password', u'otp'], True),
+        ],
+        ids=id_function)
+    def test_delete(self, globalCfg, userCfg, allowDelLast, user):
+        """
+        Test the deletion of the last otp token
+
+        The user auth type can be defined at a global level, or
+        per-user if the override is not disabled.
+        Depending on the resulting setting, the deletion of last token
+        is allowed or forbidden.
+        """
+        # Save current global config
+        result = api.Command.config_show()
+        current_globalCfg = result.get('ipauserauthtype', None)
+
+        try:
+            # Set the global config for the test
+            api.Command.config_mod(ipauserauthtype=globalCfg)
+        except errors.EmptyModlist:
+            pass
+
+        try:
+            user.ensure_exists()
+            api.Command.user_mod(user.name, userpassword=user_password)
+            unlock_principal_password(user.name,
+                                      user_password, user_password)
+            # Set the user config for the test
+            api.Command.user_mod(user.name, ipauserauthtype=userCfg)
+
+            # Connect as user, create and delete the token
+            with change_principal(user.name, user_password):
+                api.Command.otptoken_add(u'lastotp', description=u'last otp',
+                                         ipatokenowner=user.name)
+                if allowDelLast:
+                    # We are expecting the del command to succeed
+                    api.Command.otptoken_del(u'lastotp')
+                else:
+                    # We are expecting the del command to fail
+                    with pytest.raises(errors.DatabaseError):
+                        api.Command.otptoken_del(u'lastotp')
+
+        finally:
+            # Make sure the token is removed
+            try:
+                api.Command.otptoken_del(u'lastotp',)
+            except errors.NotFound:
+                pass
+
+            # Restore the previous ipauserauthtype
+            try:
+                api.Command.config_mod(ipauserauthtype=current_globalCfg)
+            except errors.EmptyModlist:
+                pass
_______________________________________________
FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org
To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org

Reply via email to