389-ds-base

Clone
Source Code
GIT

#50262 Ticket 49715 - extend account functionality
Closed 3 years ago by spichugi. Opened 5 years ago by firstyear.
firstyear/389-ds-base 49715-account-extend-functionality  into  master

file modified
+2 
@@ -56,6 +56,8 @@ 
 

  

  def _gen_filter(attrtypes, values, extra=None):
 

      filt = ''
 

+     if attrtypes is None:
 

+         raise ValueError("Attempting to filter on type that doesn't support filtering!")
 

      for attr, value in zip(attrtypes, values):
 

          if attr is not None and value is not None:
 

              filt += '(%s=%s)' % (attr, ldap_filter.escape_filter_chars(value))

file modified
+27 -12 
@@ -297,22 +297,37 @@ 
 

          raise ValueError("Unknown action '%s'. Expected add, delete, replace" % change)
 

  

  

+ def _generic_modify_inner(log, o, changes):
 

+     # Now parse the series of arguments.
 

+     # Turn them into mod lists. See apply_mods.
 

+     mods = [_generic_modify_change_to_mod(x) for x in changes]
 

+     log.debug("Requested mods: %s" % mods)
 

+     # Now push them to dsldapobject to modify
 

+     o.apply_mods(mods)
 

+     print('Successfully modified %s' % o.dn)
 

+ 

+ 

  def _generic_modify(inst, basedn, log, manager_class, selector, args=None):
 

+     if not args or not args.changes:
 

+         raise ValueError("Missing modify actions to perform.")
 

      # Here, we should have already selected the type etc. mc should be a
 

-     # type of DSLdapObject (singular)
 

+     # type of DSLdapObjects (plural)
 

      mc = manager_class(inst, basedn)
 

-     # Get the object
 

-     if args and args.changes:
 

-         o = mc.get(selector)
 

-         # Now parse the series of arguments.
 

-         # Turn them into mod lists. See apply_mods.
 

-         mods = [_generic_modify_change_to_mod(x) for x in args.changes]
 

-         log.debug("Requested mods: %s" % mods)
 

-         # Now push them to dsldapobject to modify
 

-         o.apply_mods(mods)
 

-         print('Successfully modified %s' % o.dn)
 

-     else:
 

+     # Get the object singular by selector
 

+     o = mc.get(selector)
 

+     _generic_modify_inner(log, o, args.changes)
 

+ 

+ 

+ def _generic_modify_dn(inst, basedn, log, manager_class, dn, args=None):
 

+     if not args or not args.changes:
 

          raise ValueError("Missing modify actions to perform.")
 

+     # Here, we should have already selected the type etc. mc should be a
 

+     # type of DSLdapObjects (plural)
 

+     mc = manager_class(inst, basedn)
 

+     # Get the object singular by dn
 

+     o = mc.get(dn=dn)
 

+     _generic_modify_inner(log, o, args.changes)
 

+ 

  

  

  class LogCapture(logging.Handler):

file modified
+36 -2 
@@ -11,15 +11,35 @@ 
 

  

  from lib389.idm.account import Account, Accounts
 

  from lib389.cli_base import (
 

+     _generic_get,
 

+     _generic_get_dn,
 

      _generic_list,
 

+     _generic_delete,
 

+     _generic_modify_dn,
 

      _get_arg,
 

+     _warn,
 

      )
 

  

  MANY = Accounts
 

+ SINGULAR = Account
 

  

  def list(inst, basedn, log, args):
 

      _generic_list(inst, basedn, log.getChild('_generic_list'), MANY, args)
 

  

+ def get_dn(inst, basedn, log, args):
 

+     dn = _get_arg( args.dn, msg="Enter dn to retrieve")
 

+     _generic_get_dn(inst, basedn, log.getChild('_generic_get_dn'), MANY, dn, args)
 

+ 

+ def delete(inst, basedn, log, args, warn=True):
 

+     dn = _get_arg( args.dn, msg="Enter dn to delete")
 

+     if warn:
 

+         _warn(dn, msg="Deleting %s %s" % (SINGULAR.__name__, dn))
 

+     _generic_delete(inst, basedn, log.getChild('_generic_delete'), SINGULAR, dn, args)
 

+ 

+ def modify(inst, basedn, log, args, warn=True):
 

+     dn = _get_arg( args.dn, msg="Enter dn to modify")
 

+     _generic_modify_dn(inst, basedn, log.getChild('_generic_modify'), MANY, dn, args)
 

+ 

  def status(inst, basedn, log, args):
 

      dn = _get_arg( args.dn, msg="Enter dn to check")
 

      accounts = Accounts(inst, basedn)
 
@@ -64,13 +84,27 @@ 
 

  

  

  def create_parser(subparsers):
 

-     account_parser = subparsers.add_parser('account', help='Manage generic accounts IE account locking and unlocking.')
 

+     account_parser = subparsers.add_parser('account', help='''Manage generic accounts, with tasks
 

+ like modify, locking and unlocking. To create an account, see "user" subcommand instead.''')
 

  

      subcommands = account_parser.add_subparsers(help='action')
 

  

-     list_parser = subcommands.add_parser('list', help='list')
 

+     list_parser = subcommands.add_parser('list', help='list accounts that could login to the directory')
 

      list_parser.set_defaults(func=list)
 

  

+     get_dn_parser = subcommands.add_parser('get-by-dn', help='get-by-dn <dn>')
 

+     get_dn_parser.set_defaults(func=get_dn)
 

+     get_dn_parser.add_argument('dn', nargs='?', help='The dn to get and display')
 

+ 

+     modify_dn_parser = subcommands.add_parser('modify-by-dn', help='modify-by-dn <dn> <add|delete|replace>:<attribute>:<value> ...')
 

+     modify_dn_parser.set_defaults(func=modify)
 

+     modify_dn_parser.add_argument('dn', nargs=1, help='The dn to get and display')
 

+     modify_dn_parser.add_argument('changes', nargs='+', help="A list of changes to apply in format: <add|delete|replace>:<attribute>:<value>")
 

+ 

+     delete_parser = subcommands.add_parser('delete', help='deletes the account')
 

+     delete_parser.set_defaults(func=delete)
 

+     delete_parser.add_argument('dn', nargs='?', help='The dn of the account to delete')
 

+ 

      lock_parser = subcommands.add_parser('lock', help='lock')
 

      lock_parser.set_defaults(func=lock)
 

      lock_parser.add_argument('dn', nargs='?', help='The dn to lock')

file modified
+2 -2 
@@ -169,7 +169,7 @@ 
 

              'posixGroup',
 

              'mailRecipient',
 

          ]
 

-         # MUST BE NONE.
 

+         # MUST BE NONE. For more, see _gen_filter in _mapped_object.py.
 

          self._filterattrs = None
 

          self._childobject = Account
 

          self._basedn = basedn
 
@@ -181,7 +181,7 @@ 
 

              _gen_filter(_term_gen('objectclass'), self._objectclasses)
 

          )
 

  

-     

+ 

  class Anonymous(DSLdapObject):
 

      """A single instance of Anonymous bind

Bug Description: It was noted by mreynolds that account doesn't
do as much as user does. This brings account to partial-feature
parity with user, able to modify, show and delete accounts.

Fix Description: Add the ability to show, modify and delete generic
account types.

Note that account can never, and will never gain the ability to
create accounts, because "accounts" are such an opinionated and
complex topic. For creating accounts, user will remain the
preferred command. Account exists to "manage existing" account
types, that an external system may create or feed to the 389
instance.

https://pagure.io/389-ds-base/issue/49715

Author: William Brown william@blackhats.net.au

Review by: ???

get_dn and modify_dn seems weird to me. It sounds like the command will modify entry DN or it will get the entry DN.
As an option, can we have it as get-by-dn (underscores also looks weird I think) or we can have just get/show. I think it is even better because it is short and the user will see the help with it requres DN info.

@spichugi We have a lot of commands that already use this pattern though, but I think you may be right taht it could be confusing. So we have to choose consistency (because lots already implement the action_dn scheme) or to break consistency in this case. Worth some thought ....

@spichugi We have a lot of commands that already use this pattern though, but I think you may be right taht it could be confusing. So we have to choose consistency (because lots already implement the action_dn scheme) or to break consistency in this case. Worth some thought ....

I found only one... backend get-dn.
The rest of the existing commands follow get pattern. And my upcoming Plugin CLI also uses get pattern.
So, for the consistency, I'd say we better stick to get and modify (DN will be specified as an argument)... What do you think?

Let me have a check of the code and a think about it. There are some reasons why we want a dn over a filter component in these cases.

I think that having the arugemnt as say "get --dn blah" or "get <name>", is potentially confusing as one adds the --dn param, one adds the selector, and it may affect positional arguments.

I'd say better to have a seperate command like get-dn, or if that's isn't clear, D Ithink you said "get-by-dn" which could be better. I don't think we should mix-and-match in our arguments to cli tools (that's how we generated so much tech debt in the project already in commands and apis ....)

I think that having the arugemnt as say "get --dn blah" or "get <name>", is potentially confusing as one adds the --dn param, one adds the selector, and it may affect positional arguments.
I'd say better to have a seperate command like get-dn, or if that's isn't clear, D Ithink you said "get-by-dn" which could be better. I don't think we should mix-and-match in our arguments to cli tools (that's how we generated so much tech debt in the project already in commands and apis ....)

I agree with you that the optional --dn parameter is not a good choice.
I think get-by-dn is good when we have get-by-name/get-by-cn - so the user can easily destinguish the commands.
When we have just one thing -get DN in the help, it looks better to me than having get-by-dn DN which looks redundant to me.

What do you think?

I think the confusion could be that other commands use "get <selector>" rather than "get <dn>", so this violates consistency of the interface. That's why I think get should always be a filter specificaiton for "get william" for example. That's why I'd rather it be seperate.

I think the confusion could be that other commands use "get <selector>" rather than "get <dn>", so this violates consistency of the interface. That's why I think get should always be a filter specificaiton for "get william" for example. That's why I'd rather it be seperate.

Ok, I think I see your point.

So just to sum up:

get-by-dn and modify-by-dn is used for DN specific operations.
get SELECTOR used for selector specific operations.

Is that what you mean? (and I agree to it)

Yes, that is what I mean. I'll adjust this patch accordingly. Thanks!

rebased onto 5e383292d7d20532de19748feca8a6e52b6001ff
5 years ago

Looks good to me.
One more thing though - could you please make it get-by-dn and modify-by-dn instead of modify_by_dn and get_by_dn?
It is a common approach we use in other CLI commands.

rebased onto 644fff1f27d960fa4621833d72fd631210a8e8e7
5 years ago

Updated :)

Ack! Thanks!

rebased onto 208111a
5 years ago

Pull-Request has been merged by firstyear
5 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/3321

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
Metadata
None
No Tags
Changes Summary 4
+2 -0
file changed
src/lib389/lib389/_mapped_object.py
+27 -12
file changed
src/lib389/lib389/cli_base/__init__.py
+36 -2
file changed
src/lib389/lib389/cli_idm/account.py
+2 -2
file changed
src/lib389/lib389/idm/account.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.