Sysaccounts

[abbra]

Summary by Sourcery

This pull request introduces the concept of system accounts, which are LDAP accounts for applications that do not support Kerberos authentication. It also includes API commands to manage these accounts.

New Features:

• Introduces system accounts, which are LDAP accounts for applications that do not support Kerberos authentication.
• Adds API commands to manage system accounts, including add, delete, modify, find, show, enable, and disable.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request introduces the concept of system accounts, which are LDAP accounts for applications to query the LDAP database. It also adds API commands to manage these accounts, including add, delete, find, show, enable, and disable.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Introduces system accounts, which are LDAP accounts for applications to query the LDAP database.	Added a new sysaccount object class to represent system accounts in the LDAP directory. Implemented methods for creating, deleting, modifying, finding, showing, enabling, and disabling system accounts. Defined the location of system accounts within the LDAP directory (cn=sysaccounts,cn=etc). Added input validation for system account IDs, including pattern matching and maximum length constraints. Implemented permission management for system accounts, including defining default privileges and access control rules.	ipaserver/plugins/sysaccounts.py
Adds API commands to manage system accounts, including add, delete, find, show, enable, and disable.	Implemented sysaccount_add command to create new system accounts. Implemented sysaccount_del command to delete system accounts, with a check to prevent deletion of IPA-required system accounts. Implemented sysaccount_find command to search for system accounts based on various criteria. Implemented sysaccount_show command to display detailed information about a specific system account. Implemented sysaccount_disable command to disable system accounts by deactivating the entry and removing the principal key. Implemented sysaccount_enable command to enable system accounts by activating the entry. Added documentation for each command, including arguments, options, and output formats.	ipaserver/plugins/sysaccounts.py doc/api/sysaccount_add.md doc/api/sysaccount_del.md doc/api/sysaccount_find.md doc/api/sysaccount_show.md doc/api/sysaccount_disable.md doc/api/sysaccount_enable.md
Introduces design documentation for LDAP system accounts.	Added an overview of system accounts and their purpose. Described use cases for system accounts, such as legacy application authentication and external account password rotation. Explained how to use system accounts for LDAP authentication. Outlined the design considerations for system accounts in FreeIPA.	doc/designs/sysaccounts.md

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!
• Generate a plan of action for an issue: Comment @sourcery-ai plan on
  an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @abbra - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

• The ACI configuration allows modification of the userpassword attribute, which could be a security risk if not properly controlled. (link)

Overall Comments:

• Consider adding tests to verify the behavior of the new sysaccount commands.
• Ensure that the new sysaccount commands are properly integrated into the IPA CLI.

Here's what I looked at during the review

• 🟡 General issues: 3 issues found
• 🔴 Security: 1 blocking issue
• 🟢 Testing: all looks good
• 🟡 Complexity: 1 issue found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (typo): Typo in the permission_filter_objectclasses value.

The value 'simplesecuretyobject' appears to be a typo. It should likely be 'simplesecurityobject' to match the object_class defined and the intended LDAP schema.

Suggested change

	permission_filter_objectclasses = ['simplesecuretyobject']
	permission_filter_objectclasses = ['simplesecurityobject']

[sourcery-ai]

suggestion (bug_risk): Potential risk for KeyError in populate_krbcanonicalname.

If 'krbprincipalname' might be absent in entry_attrs, using entry_attrs.get('krbprincipalname', '') could help avoid an unexpected KeyError when calling setdefault.

Suggested change

	def populate_krbcanonicalname(self, entry_attrs, options):
	if options.get('raw', False):
	return
	entry_attrs.setdefault(
	'krbcanonicalname', entry_attrs['krbprincipalname'])
	def populate_krbcanonicalname(self, entry_attrs, options):
	if options.get('raw', False):
	return
	entry_attrs.setdefault(
	'krbcanonicalname', entry_attrs.get('krbprincipalname', ''))

[sourcery-ai]

issue (typo): Typo: "simplesecuretyobject" should be "simplesecurityobject".

Suggested implementation:

aci: (targetfilter = "(objectclass=simplesecurityobject)")(version 3.0;acl "permission:System: Add System Accounts";allow (add) groupdn = "ldap:///cn=System: Add System Accounts,cn=permissions,cn=pbac,dc=ipa,dc=example";)

aci: (targetattr = "userpassword")(targetfilter = "(objectclass=simplesecurityobject)")(version 3.0;acl "permission:System: Modify System Accounts";allow (write) groupdn = "ldap:///cn=System: Modify System Accounts,cn=permissions,cn=pbac,dc=ipa,dc=example";)

aci: (targetattr = "createtimestamp || entryusn || memberof || modifytimestamp || objectclass || uid")(targetfilter = "(objectclass=simplesecurityobject)")(version 3.0;acl "permission:System: Read System Accounts";allow (compare,read,search) userdn = "ldap:///all";)

aci: (targetfilter = "(objectclass=simplesecurityobject)")(version 3.0;acl "permission:System: Remove System Accounts";allow (delete) groupdn = "ldap:///cn=System: Remove System Accounts,cn=permissions,cn=pbac,dc=ipa,dc=example";)

[abbra]

@sourcery-ai review

[sourcery-ai]

Hey @abbra - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

• The ACI contains the userpassword attribute, which is a secret. (link)

Overall Comments:

• Consider adding a test case that attempts to delete one of the required system accounts to ensure the appropriate error is raised.
• The sysaccount object class list seems incomplete - are 'ipaobject' and 'top' missing?

Here's what I looked at during the review

• 🟡 General issues: 1 issue found
• 🔴 Security: 1 blocking issue
• 🟢 Testing: all looks good
• 🟡 Complexity: 1 issue found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (bug_risk): Safeguard access to 'has_keytab'.

To avoid potential KeyErrors, consider using entry_attrs.get('has_keytab', False) in case the attribute is missing in some entries.

Suggested change

	if entry_attrs['has_keytab']:
	if entry_attrs.get('has_keytab', False):

[sourcery-ai]

issue (complexity): Consider refactoring get_primary_key_from_dn and extracting the inline sort_key function in sysaccount_find to reduce nesting and improve clarity, without altering functionality or introducing new logic.

Consider simplifying some nested blocks to reduce cognitive overhead. For example, refactor `get_primary_key_from_dn` by extracting a helper function:

# Before
def get_primary_key_from_dn(self, dn):
    assert isinstance(dn, DN)
    try:
        entry_attrs = self.backend.get_entry(dn, [self.primary_key.name])
        try:
            return entry_attrs[self.primary_key.name][0]
        except (KeyError, IndexError):
            return ''
    except errors.NotFound:
        pass

    try:
        return dn['krbprincipalname']
    except KeyError:
        return str(dn)

# After
def _get_entry_primary_key(self, dn):
    try:
        entry_attrs = self.backend.get_entry(dn, [self.primary_key.name])
        return entry_attrs.get(self.primary_key.name, [None])[0]
    except errors.NotFound:
        return None

def get_primary_key_from_dn(self, dn):
    assert isinstance(dn, DN)
    primary = self._get_entry_primary_key(dn)
    if primary:
        return primary
    return dn.get('krbprincipalname', str(dn))

Similarly, for the inline sort_key function in sysaccount_find, extract it to improve clarity:

# Before inside post_callback:
def sort_key(x):
    if 'krbcanonicalname' in x:
        return x['krbcanonicalname'][0]
    else:
        return x['krbprincipalname'][0]
entries.sort(key=sort_key)

# After extracting:
def _extract_sort_key(entry):
    # Prefer krbcanonicalname; fallback to krbprincipalname; default to empty string.
    attr = entry.get('krbcanonicalname') or entry.get('krbprincipalname') or ['']
    return attr[0]

# then in post_callback:
entries.sort(key=_extract_sort_key)

These changes keep functionality intact while reducing nesting and duplication.

[sourcery-ai]

🚨 issue (security): The ACI contains the userpassword attribute, which is a secret.

[sourcery-ai]

issue (code-quality): Don't assign to builtin variable \_\_doc\_\_ (avoid-builtin-shadow)

Explanation

Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:

list = [1, 2, 3]

However, this is considered poor practice.

• It will confuse other developers.
• It will confuse syntax highlighters and linters.
• It means you can no longer use that builtin for its original purpose.

How can you solve this?

Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.

[abbra]

@sourcery-ai review

[sourcery-ai]

Hey @abbra - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

• The ACI contains the userpassword attribute, which is a secret. (link)

Overall Comments:

• Consider adding a comment to explain why the jitter window is being adjusted for anonymous PKINIT requests.
• The new sysaccount plugin introduces a lot of code - can you extract some common functionality into a shared module?

Here's what I looked at during the review

• 🟡 General issues: 1 issue found
• 🔴 Security: 1 blocking issue
• 🟢 Testing: all looks good
• 🟡 Complexity: 1 issue found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): Revisit the disable logic when keytab is absent.

In sysaccount_disable, deactivating the entry is performed regardless, but done_work is only set if a keytab removal happens. As a result, an account without a keytab may be deactivated yet still trigger an AlreadyInactive error. Please verify if this behavior is intended or if the logic should also treat a deactivated entry without keytab removal as successfully disabled.

[sourcery-ai]

issue (complexity): Consider extracting helper methods to simplify nested error handling and improve code maintainability by flattening the control flow and separating concerns, for example, in get_primary_key_from_dn and sysaccount_find.post_callback methods, which will improve readability and maintainability without altering the functionality.

Consider extracting helper methods to simplify the nested error‐handling in methods like get_primary_key_from_dn. For example, you could extract the attribute extraction logic into its own helper function and then use early returns. This keeps responsibilities separate and flattens the control flow. One approach:

def _get_attr_value(self, entry_attrs, attr):
    """Return first value of attribute or None if missing."""
    try:
        return entry_attrs[attr][0]
    except (KeyError, IndexError):
        return None

def get_primary_key_from_dn(self, dn):
    """Refactored: tries fetching primary key from LDAP entry then falls back."""
    try:
        entry_attrs = self.backend.get_entry(dn, [self.primary_key.name])
        primary = self._get_attr_value(entry_attrs, self.primary_key.name)
        if primary is not None:
            return primary
    except errors.NotFound:
        pass
    # Fallback to krbprincipalname or DN string representation
    return dn.get('krbprincipalname', str(dn))

Similarly, if other methods (like sysaccount_find.post_callback) mix sorting, error handling, and formatting logic, consider extracting the sort key logic:

def _sort_key(self, entry):
    return entry.get('krbcanonicalname', entry.get('krbprincipalname'))[0]

def post_callback(self, ldap, entries, truncated, *args, **options):
    entries.sort(key=self._sort_key)
    if options.get('pkey_only', False):
        return truncated
    for entry_attrs in entries:
        # Process each entry using a dedicated helper if needed.
        self._finalize_entry(ldap, entry_attrs)
    return truncated

This separates concerns and flattens nested structures, improving maintainability while keeping functionality intact.

[sourcery-ai]

🚨 issue (security): The ACI contains the userpassword attribute, which is a secret.

[sourcery-ai]

issue (code-quality): Don't assign to builtin variable \_\_doc\_\_ (avoid-builtin-shadow)

Explanation

Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:

list = [1, 2, 3]

However, this is considered poor practice.

• It will confuse other developers.
• It will confuse syntax highlighters and linters.
• It means you can no longer use that builtin for its original purpose.

How can you solve this?

Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.

[abbra]

@sourcery-ai review

[sourcery-ai]

Hey @abbra - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

• The ACI contains the attribute userpassword, which is often used to store passwords. Please ensure that this is intentional and that appropriate security measures are in place. (link)

Overall Comments:

• The sysaccount object should have a description to explain its purpose.
• Consider adding a test suite to verify the functionality of the new sysaccount API.

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🔴 Security: 1 blocking issue
• 🟢 Testing: all looks good
• 🟡 Complexity: 1 issue found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (complexity): Consider refactoring duplicated code into helper functions to improve readability and maintainability, such as extracting the try/except block in get_primary_key_from_dn and the sorting logic in sysaccount_find into separate functions .

Consider extracting common patterns into helper functions/modules to reduce duplication and nesting. For example, you can refactor the nested try/except logic from get_primary_key_from_dn into a dedicated function. You can also extract the inline sorting function from the post_callback method into a standalone helper.

Example for refactoring get_primary_key_from_dn:

def resolve_primary_key(backend, primary_key, dn):
    try:
        entry_attrs = backend.get_entry(dn, [primary_key.name])
        return entry_attrs.get(primary_key.name, [''])[0]
    except (errors.NotFound, KeyError, IndexError):
        pass
    return dn.get('krbprincipalname', str(dn))

Then update your method as:

def get_primary_key_from_dn(self, dn):
    assert isinstance(dn, DN)
    return resolve_primary_key(self.backend, self.primary_key, dn)

Example for refactoring the inline sort key in sysaccount_find:

def sort_by_canonical_or_principal(entry):
    if 'krbcanonicalname' in entry:
        return entry['krbcanonicalname'][0]
    return entry['krbprincipalname'][0]

# In the post_callback:
entries.sort(key=sort_by_canonical_or_principal)

By isolating repeated logic, you improve readability and maintainability while keeping the existing functionality unchanged.

[sourcery-ai]

🚨 issue (security): The ACI contains the attribute userpassword, which is often used to store passwords. Please ensure that this is intentional and that appropriate security measures are in place.

[sourcery-ai]

issue (code-quality): Don't assign to builtin variable \_\_doc\_\_ (avoid-builtin-shadow)

Explanation

Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:

list = [1, 2, 3]

However, this is considered poor practice.

• It will confuse other developers.
• It will confuse syntax highlighters and linters.
• It means you can no longer use that builtin for its original purpose.

How can you solve this?

Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.