#50641 Update default aci to allows users to change their own password
Closed 3 years ago by spichugi. Opened 4 years ago by firstyear.

@@ -0,0 +1,133 @@ 

+ # --- BEGIN COPYRIGHT BLOCK ---

+ # Copyright (C) 2019 William Brown <william@blackhats.net.au>

+ # All rights reserved.

+ #

+ # License: GPL (version 3 or any later version).

+ # See LICENSE for details.

+ # --- END COPYRIGHT BLOCK ---

+ 

+ 

+ import pytest

+ from lib389.idm.user import nsUserAccounts, UserAccounts

+ from lib389.topologies import topology_st as topology

+ from lib389.paths import Paths

+ from lib389.utils import ds_is_older

+ from lib389._constants import *

+ 

+ default_paths = Paths()

+ 

+ pytestmark = pytest.mark.tier1

+ 

+ USER_PASSWORD = "some test password"

+ NEW_USER_PASSWORD = "some new password"

+ 

+ @pytest.mark.skipif(default_paths.perl_enabled or ds_is_older('1.4.2.0'), reason="Default aci's in older versions do not support this functionality")

+ def test_acl_default_allow_self_write_nsuser(topology):

+     """

+     Testing nsusers can self write and self read. This it a sanity test

+     so that our default entries have their aci's checked.

+ 

+     :id: 4f0fb01a-36a6-430c-a2ee-ebeb036bd951

+ 

+     :setup: Standalone instance

+ 

+     :steps:

+         1. Testing comparison of two different users.

+ 

+     :expectedresults:

+         1. Should fail to compare

+     """

+     topology.standalone.enable_tls()

+     nsusers = nsUserAccounts(topology.standalone, DEFAULT_SUFFIX)

+     # Create a user as dm.

+     user = nsusers.create(properties={

+         'uid': 'test_nsuser',

+         'cn': 'test_nsuser',

+         'displayName': 'testNsuser',

+         'legalName': 'testNsuser',

+         'uidNumber': '1001',

+         'gidNumber': '1001',

+         'homeDirectory': '/home/testnsuser',

+         'userPassword': USER_PASSWORD,

+     })

+     # Create a new con and bind as the user.

+     user_conn = user.bind(USER_PASSWORD)

+ 

+     user_nsusers = nsUserAccounts(user_conn, DEFAULT_SUFFIX)

+     self_ent = user_nsusers.get(dn=user.dn)

+ 

+     # Can we self read x,y,z

+     check = self_ent.get_attrs_vals_utf8([

+         'uid',

+         'cn',

+         'displayName',

+         'legalName',

+         'uidNumber',

+         'gidNumber',

+         'homeDirectory',

+     ])

+     for k in check.values():

+         # Could we read the values?

+         assert(isinstance(k, list))

+         assert(len(k) > 0)

+     # Can we self change a,b,c

+     self_ent.ensure_attr_state({

+         'legalName': ['testNsuser_update'],

+         'displayName': ['testNsuser_update'],

+         'nsSshPublicKey': ['testkey'],

+     })

+     # self change pw

+     self_ent.change_password(USER_PASSWORD, NEW_USER_PASSWORD)

+ 

+ 

+ @pytest.mark.skipif(default_paths.perl_enabled or ds_is_older('1.4.2.0'), reason="Default aci's in older versions do not support this functionality")

+ def test_acl_default_allow_self_write_user(topology):

+     """

+     Testing users can self write and self read. This it a sanity test

+     so that our default entries have their aci's checked.

+ 

+     :id: 4c52321b-f473-4c32-a1d5-489b138cd199

+ 

+     :setup: Standalone instance

+ 

+     :steps:

+         1. Testing comparison of two different users.

+ 

+     :expectedresults:

+         1. Should fail to compare

+     """

+     topology.standalone.enable_tls()

+     users = UserAccounts(topology.standalone, DEFAULT_SUFFIX)

+     # Create a user as dm.

+     user = users.create(properties={

+         'uid': 'test_user',

+         'cn': 'test_user',

+         'sn': 'User',

+         'uidNumber': '1002',

+         'gidNumber': '1002',

+         'homeDirectory': '/home/testuser',

+         'userPassword': USER_PASSWORD,

+     })

+     # Create a new con and bind as the user.

+     user_conn = user.bind(USER_PASSWORD)

+ 

+     user_users = UserAccounts(user_conn, DEFAULT_SUFFIX)

+     self_ent = user_users.get(dn=user.dn)

+     # Can we self read x,y,z

+     check = self_ent.get_attrs_vals_utf8([

+         'uid',

+         'cn',

+         'sn',

+         'uidNumber',

+         'gidNumber',

+         'homeDirectory',

+     ])

+     for (a, k) in check.items():

+         print(a)

+         # Could we read the values?

+         assert(isinstance(k, list))

+         assert(len(k) > 0)

+     # Self change pw

+     self_ent.change_password(USER_PASSWORD, NEW_USER_PASSWORD)

+ 

+ 

file modified
+4 -2
@@ -421,12 +421,14 @@ 

          :type version: str

          """

  

-         self._log.debug('Creating sample entries at version %s....' % version)

-         # Grab the correct sample entry config

+         self._log.debug('Requested sample entries at version %s....' % version)

+         # Grab the correct sample entry config - remember this is a function ptr.

          centries = get_sample_entries(version)

          # apply it.

          basedn = self.get_attr_val('nsslapd-suffix')

          cent = centries(self._instance, basedn)

+         # Now it's built, we can get the version for logging.

+         self._log.debug('Creating sample entries at version %s' % cent.version)

          cent.apply()

  

      def _validate(self, rdn, properties, basedn):

@@ -10,15 +10,20 @@ 

  from lib389._constants import INSTALL_LATEST_CONFIG

  from .config_001003006 import c001003006, c001003006_sample_entries

  from .config_001004000 import c001004000, c001004000_sample_entries

+ from .config_001004002 import c001004002, c001004002_sample_entries

  

  

  def get_config(version):

      # We do this to avoid test breaking on older version that may

      # not expect the new default layout.

-     if (version == INSTALL_LATEST_CONFIG and ds_is_newer('1.4.0')):

+     if (version == INSTALL_LATEST_CONFIG and ds_is_newer('1.4.2')):

+         return c001004002

+     elif (version == INSTALL_LATEST_CONFIG and ds_is_newer('1.4.0')):

          return c001004000

      elif (version == INSTALL_LATEST_CONFIG):

          return c001003006

+     elif (version == '001004002' and ds_is_newer('1.4.2')):

+         return c001004002

      elif (version == '001004000' and ds_is_newer('1.4.0')):

          return c001004000

      elif (version == '001003006'):
@@ -28,7 +33,9 @@ 

  

  def get_sample_entries(version):

      if (version == INSTALL_LATEST_CONFIG):

-         return c001004000_sample_entries

+         return c001004002_sample_entries

+     elif (version == '001004002'):

+         return c001004002_sample_entries

      elif (version == '001004000'):

          return c001004000_sample_entries

      elif (version == '001003006'):

@@ -18,6 +18,7 @@ 

      def __init__(self, instance, basedn):

          super(c001003006_sample_entries, self).__init__(instance, basedn)

          self.description = """Apply sample entries matching the 1.3.6 sample data and access controls."""

+         self.version = "c001003006"

  

      # All the checks are done, apply them.

      def _apply(self):

@@ -7,13 +7,7 @@ 

  # --- END COPYRIGHT BLOCK ---

  

  from .config import baseconfig, configoperation

- from .sample import (

-     sampleentries,

-     create_base_domain,

-     create_base_org,

-     create_base_orgunit,

-     create_base_cn,

- )

+ from .sample import sampleentries

  from lib389.idm.organizationalunit import OrganizationalUnits

  from lib389.idm.group import Groups

  from lib389.idm.posixgroup import PosixGroups
@@ -26,26 +20,11 @@ 

      def __init__(self, instance, basedn):

          super(c001004000_sample_entries, self).__init__(instance, basedn)

          self.description = """Apply sample entries matching the 1.4.0 sample data and access controls"""

+         self.version = "c001004000"

  

      # All checks done, apply!

      def _apply(self):

-         suffix_rdn_attr = self._basedn.split('=')[0].lower()

-         if suffix_rdn_attr == 'dc':

-             suffix_obj = create_base_domain(self._instance, self._basedn)

-             aci_vals = ['dc', 'domain']

-         elif suffix_rdn_attr == 'o':

-             suffix_obj = create_base_org(self._instance, self._basedn)

-             aci_vals = ['o', 'organization']

-         elif suffix_rdn_attr == 'ou':

-             suffix_obj = create_base_orgunit(self._instance, self._basedn)

-             aci_vals = ['ou', 'organizationalunit']

-         elif suffix_rdn_attr == 'cn':

-             suffix_obj = create_base_cn(self._instance, self._basedn)

-             aci_vals = ['cn', 'nscontainer']

-         else:

-             # Unsupported rdn

-             raise ValueError("Suffix RDN is not supported for creating sample entries.  Only 'dc', 'o', 'ou', and 'cn' are supported.")

- 

+         suffix_obj = self._configure_base()

          # Create the base object

          suffix_obj.add('aci', [

              # Allow reading the base domain object

@@ -0,0 +1,140 @@ 

+ # --- BEGIN COPYRIGHT BLOCK ---

+ # Copyright (C) 2017 Red Hat, Inc.

+ # All rights reserved.

+ #

+ # License: GPL (version 3 or any later version).

+ # See LICENSE for details.

+ # --- END COPYRIGHT BLOCK ---

+ 

+ from .config import baseconfig, configoperation

+ from .sample import sampleentries

+ 

+ from lib389.idm.organizationalunit import OrganizationalUnits

+ from lib389.idm.group import Groups

+ from lib389.idm.posixgroup import PosixGroups

+ from lib389.idm.user import nsUserAccounts

+ from lib389.idm.services import ServiceAccounts

+ 

+ from lib389.idm.nscontainer import nsHiddenContainers

+ 

+ class c001004002_sample_entries(sampleentries):

+     def __init__(self, instance, basedn):

+         super(c001004002_sample_entries, self).__init__(instance, basedn)

+         self.description = """Apply sample entries matching the 1.4.2 sample data and access controls"""

+         self.version = "c001004002"

+ 

+     # All checks done, apply!

+     def _apply(self):

+         suffix_obj = self._configure_base()

+         # Create the base domain object

+         suffix_obj.add('aci', [

+             # Allow reading the base domain object

+             '(targetattr="dc || description || objectClass")(targetfilter="(objectClass=domain)")(version 3.0; acl "Enable anyone domain read"; allow (read, search, compare)(userdn="ldap:///anyone");)',

+             # Allow reading the ou

+             '(targetattr="ou || objectClass")(targetfilter="(objectClass=organizationalUnit)")(version 3.0; acl "Enable anyone ou read"; allow (read, search, compare)(userdn="ldap:///anyone");)'

+         ])

+ 

+         # Create the 389 service container

+         # This could also move to be part of core later ....

+         hidden_containers = nsHiddenContainers(self._instance, self._basedn)

+         ns389container = hidden_containers.create(properties={

+             'cn': '389_ds_system'

+             })

+ 

+         # Create our ous.

+         ous = OrganizationalUnits(self._instance, self._basedn)

+         ous.create(properties = {

+             'ou': 'groups',

+             'aci': [

+                 # Allow anon partial read

+                 '(targetattr="cn || member || gidNumber || nsUniqueId || description || objectClass")(targetfilter="(objectClass=groupOfNames)")(version 3.0; acl "Enable anyone group read"; allow (read, search, compare)(userdn="ldap:///anyone");)',

+                 # Allow group_modify to modify but not create groups

+                 '(targetattr="member")(targetfilter="(objectClass=groupOfNames)")(version 3.0; acl "Enable group_modify to alter members"; allow (write)(groupdn="ldap:///cn=group_modify,ou=permissions,{BASEDN}");)'.format(BASEDN=self._basedn),

+                 # Allow group_admin to fully manage groups (posix or not).

+                 '(targetattr="cn || member || gidNumber || description || objectClass")(targetfilter="(objectClass=groupOfNames)")(version 3.0; acl "Enable group_admin to manage groups"; allow (write, add, delete)(groupdn="ldap:///cn=group_admin,ou=permissions,{BASEDN}");)'.format(BASEDN=self._basedn),

+             ]

+         })

+ 

+         ous.create(properties = {

+             'ou': 'people',

+             'aci': [

+                 # allow anon partial read.

+                 '(targetattr="objectClass || description || nsUniqueId || uid || displayName || loginShell || uidNumber || gidNumber || gecos || homeDirectory || cn || memberOf || mail || nsSshPublicKey || nsAccountLock || userCertificate")(targetfilter="(objectClass=posixaccount)")(version 3.0; acl "Enable anyone user read"; allow (read, search, compare)(userdn="ldap:///anyone");)',

+                 # allow self partial mod

+                 '(targetattr="displayName || legalName || userPassword || nsSshPublicKey")(version 3.0; acl "Enable self partial modify"; allow (write)(userdn="ldap:///self");)',

+                 # Allow self full read

+                 '(targetattr="legalName || telephoneNumber || mobile || sn")(targetfilter="(|(objectClass=nsPerson)(objectClass=inetOrgPerson))")(version 3.0; acl "Enable self legalname read"; allow (read, search, compare)(userdn="ldap:///self");)',

+                 # Allow reading legal name

+                 '(targetattr="legalName || telephoneNumber")(targetfilter="(objectClass=nsPerson)")(version 3.0; acl "Enable user legalname read"; allow (read, search, compare)(groupdn="ldap:///cn=user_private_read,ou=permissions,{BASEDN}");)'.format(BASEDN=self._basedn),

+                 # These below need READ so they can read userPassword and legalName

+                 # Allow user admin create mod

+                 '(targetattr="uid || description || displayName || loginShell || uidNumber || gidNumber || gecos || homeDirectory || cn || memberOf || mail || legalName || telephoneNumber || mobile")(targetfilter="(&(objectClass=nsPerson)(objectClass=nsAccount))")(version 3.0; acl "Enable user admin create"; allow (write, add, delete, read)(groupdn="ldap:///cn=user_admin,ou=permissions,{BASEDN}");)'.format(BASEDN=self._basedn),

+                 # Allow user mod permission to mod only

+                 '(targetattr="uid || description || displayName || loginShell || uidNumber || gidNumber || gecos || homeDirectory || cn || memberOf || mail || legalName || telephoneNumber || mobile")(targetfilter="(&(objectClass=nsPerson)(objectClass=nsAccount))")(version 3.0; acl "Enable user modify to change users"; allow (write, read)(groupdn="ldap:///cn=user_modify,ou=permissions,{BASEDN}");)'.format(BASEDN=self._basedn),

+                 # Allow user_pw_admin to nsaccountlock and password

+                 '(targetattr="userPassword || nsAccountLock || userCertificate || nsSshPublicKey")(targetfilter="(objectClass=nsAccount)")(version 3.0; acl "Enable user password reset"; allow (write, read)(groupdn="ldap:///cn=user_passwd_reset,ou=permissions,{BASEDN}");)'.format(BASEDN=self._basedn),

+             ]

+         })

+ 

+         ous.create(properties = {

+             'ou': 'permissions',

+         })

+ 

+         ous.create(properties = {

+             'ou': 'services',

+             'aci': [

+                 # Minimal service read

+                 '(targetattr="objectClass || description || nsUniqueId || cn || memberOf || nsAccountLock ")(targetfilter="(objectClass=netscapeServer)")(version 3.0; acl "Enable anyone service account read"; allow (read, search, compare)(userdn="ldap:///anyone");)',

+             ]

+         })

+ 

+         # Create the demo user

+         users = nsUserAccounts(self._instance, self._basedn)

+         users.create(properties={

+             'uid': 'demo_user',

+             'cn': 'Demo User',

+             'displayName': 'Demo User',

+             'legalName': 'Demo User Name',

+             'uidNumber': '99998',

+             'gidNumber': '99998',

+             'homeDirectory': '/var/empty',

+             'loginShell': '/bin/false',

+             'nsAccountlock': 'true'

+         })

+ 

+         # Create the demo group

+         groups = PosixGroups(self._instance, self._basedn)

+         groups.create(properties={

+             'cn' : 'demo_group',

+             'gidNumber': '99999'

+         })

+ 

+         # Create the permission groups required for the acis

+         permissions = Groups(self._instance, self._basedn, rdn='ou=permissions')

+         permissions.create(properties={

+             'cn': 'group_admin',

+         })

+         permissions.create(properties={

+             'cn': 'group_modify',

+         })

+         permissions.create(properties={

+             'cn': 'user_admin',

+         })

+         permissions.create(properties={

+             'cn': 'user_modify',

+         })

+         permissions.create(properties={

+             'cn': 'user_passwd_reset',

+         })

+         permissions.create(properties={

+             'cn': 'user_private_read',

+         })

+ 

+ 

+ class c001004002(baseconfig):

+     def __init__(self, instance):

+         super(c001004002, self).__init__(instance)

+         self._operations = [

+             # For now this is an empty place holder - likely this

+             # will become part of core server.

+         ]

@@ -15,19 +15,6 @@ 

  from lib389.utils import ensure_str

  

  

- class sampleentries(object):

-     def __init__(self, instance, basedn):

-         self._instance = instance

-         self._basedn = ensure_str(basedn)

-         self.description = None

- 

-     def apply(self):

-         self._apply()

- 

-     def _apply(self):

-         raise Exception('Not implemented')

- 

- 

  def create_base_domain(instance, basedn):

      """Create the base domain object"""

  
@@ -95,3 +82,40 @@ 

      })

  

      return cn

+ 

+ 

+ class sampleentries(object):

+     def __init__(self, instance, basedn):

+         self._instance = instance

+         self._basedn = ensure_str(basedn)

+         self.description = None

+         self.version = None

+ 

+     def apply(self):

+         self._apply()

+ 

+     def _configure_base(self):

+         suffix_rdn_attr = self._basedn.split('=')[0].lower()

+         suffix_obj = None

+         if suffix_rdn_attr == 'dc':

+             suffix_obj = create_base_domain(self._instance, self._basedn)

+             aci_vals = ['dc', 'domain']

+         elif suffix_rdn_attr == 'o':

+             suffix_obj = create_base_org(self._instance, self._basedn)

+             aci_vals = ['o', 'organization']

+         elif suffix_rdn_attr == 'ou':

+             suffix_obj = create_base_orgunit(self._instance, self._basedn)

+             aci_vals = ['ou', 'organizationalunit']

+         elif suffix_rdn_attr == 'cn':

+             suffix_obj = create_base_cn(self._instance, self._basedn)

+             aci_vals = ['cn', 'nscontainer']

+         else:

+             # Unsupported rdn

+             raise ValueError("Suffix RDN is not supported for creating sample entries.  Only 'dc', 'o', 'ou', and 'cn' are supported.")

+ 

+         return suffix_obj

+ 

+     def _apply(self):

+         raise Exception('Not implemented')

+ 

+ 

Bug Description: The default acis were too restrictive - we do want
people to be able to self change passwords by default!

Fix Description: Fix the default aci's and add tests to prove they behave
as we actually expect.

rebased onto 5e5562740508d13e7ddefffa49950b45adc7a071

4 years ago

As I mentioned before, please don't conflate tickets/issues and PRs. Issues in pagure are used for traceability and cross-linking to bugzilla. PRs can't be used for this.

Please create an issue for this change and link back to the issue in the commit message using "Fixes/Relates" keywords (see http://www.port389.org/docs/389ds/contributing.html#getting-the-patch-ready and https://pagure.io/389-ds-base/pull-request/50444#comment-89063). This way PR and issue will be automatically cross-linked.

I appreciate fixing this, but it really should be a separate PR linking (using Relates keyword) to #50627.

This test fails on a build from this PR with:

E         ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'write' privilege to the 'legalName' attribute of entry 'uid=test_nsu
ser,ou=people,dc=example,dc=com'.\n"}

Is this expected?

There are sometimes issues with python paths and not installing correctly if the file exists ... so ensure you have the PYTHONPATH set to the git check of lib389 and it works.

I appreciate fixing this, but it really should be a separate PR linking (using Relates keyword) to #50627.

Yep. I can split this out. It was causing tests to crash for me.

rebased onto 31dcd2a959aa6024fcdafd90936621c89e8d7eab

4 years ago

As I mentioned before, please don't conflate tickets/issues and PRs. Issues in pagure are used for traceability and cross-linking to bugzilla. PRs can't be used for this.
Please create an issue for this change and link back to the issue in the commit message using "Fixes/Relates" keywords (see http://www.port389.org/docs/389ds/contributing.html#getting-the-patch-ready and https://pagure.io/389-ds-base/pull-request/50444#comment-89063). This way PR and issue will be automatically cross-linked.

I feel like we're going to need to reconsider this in the future to make it easier to push small fixes etc without a high admin overhead.

But for now: https://pagure.io/389-ds-base/issue/50644

I feel like we're going to need to reconsider this in the future to make it easier to push small fixes etc without a high admin overhead.

I feel we need to discuss this first before making changes in process. Let's move this to the mailing list?

And changing default aci is not a small fix IMHO. Things like this need to have an audit trail.

I'll put something on the ml now then :)

Okay, no one has responded ... or at least objected. So I'll merge this then!

Actually, no one has said "ack" so I'll wait. @mreynolds and @vashirov ??

rebased onto 79ace62d88581e4caa26263cd30873f2ea99e562

4 years ago

@firstyear, so @vashirov was asking that you create a ticket so we track this change. Viktor also noted that a test is failing with this PR. So we need to get those things fixed first before acking...

Uhhhh @mreynolds I did make the ticket. https://pagure.io/389-ds-base/issue/50644
The test failing also can't be reproduced by me, I think that's an environment thing ....

Test fails with the perl installer, because new acis are not created.
So either
1. ldap/ldif/template.ldif should be updated.
or
2. test should be xfailed when enable_perl = yes in defaults.inf.

I'd prefer 1. for the consistency. WDYT?

Well, template.ldif isn't updated for the 1.4.0 changes either? So updating it now seems incorrect because it's already inconsistent.

I think the xfail option may be better here because it seems like the least movement/disruption? Most tests add their aci/data as needed anyway, and ignore the default entries.... There is certainly an argument to "test" instances having a different example database setup for testing consistency vs what we ship to users for their out of box experience.

rebased onto 445bca1ff40f031b5c4998433d43720be5e40174

4 years ago

@vashirov marked with skipif flags now for perl.

It passes the basic test suite. Now something you need to do now is how I changed how the sample entries are created. Previously the sample entries assumed you used dc=DOMAIN styling. But it failed for base dn's that use "o", "ou" or "cn", etc.

Check out config_001004000.py and you'll see what I did. Should be a minor change to apply the same thing to config_001004002.py.

Thanks!

@mreynolds I'll have a look, is this a change that's in git master and I need to rebase to fix it? Where are the changes is what I'm asking. Thanks!

rebased onto 5a26d54

4 years ago

You'll need to review again @mreynolds as I did a minor refactor to standardise the suffix_obj into sampleentries, and then have both the configs call that in their work. But it shoulddo what you want :)

Pull-Request has been merged by firstyear

4 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3696

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago