On Monday 24 of June 2013 16:22:01 Petr Viktorin wrote:
> On 06/20/2013 12:56 PM, Tomas Babej wrote:
> > On 06/17/2013 02:34 PM, Ana Krivokapic wrote:
> >> On 06/06/2013 11:10 AM, Tomas Babej wrote:
> >>> Hi,
> >>>
> >>> Adds --use-posix option to ipa trust-add command. It takes two
> >>> allowed values:
> >>> 'yes' : the 'ipa-ad-trust-posix' range type is enforced
> >>> 'no' : the 'ipa-ad-trust' range type is enforced
> >>>
> >>> When --use-posix option is not specified, the range type should be
> >>> determined by ID range discovery.
> >>>
> >>> https://fedorahosted.org/freeipa/ticket/3650
> >>>
> >>> Tomas
> >>>
> >>>
> >>> _______________________________________________
> >>> Freeipa-devel mailing list
> >>> Freeipa-devel@redhat.com
> >>> https://www.redhat.com/mailman/listinfo/freeipa-devel
> >>
> >> The patch works nicely, but I have a few comments:
> >>
> >> 1) You added a new option to the API, but you forgot to bump the
> >> IPA_API_VERSION_MINOR in the VERSION file.
> >>
> >> 2) Typo in commit message: "shold" instead of "should".
> >>
> >> 3) This construct:
> >>
> >> +            if range_type is not None:
> >> +                if range_type != old_range_type:
> >>
> >> can be replaced with a more readable variant which also avoids nested 
ifs:
> >>
> >> +            if range_type and range_type != old_range_type:
> >>
> >
> > All fixed.
> >
> >> 4) Some unit tests to cover the behavior of the newly added option
> >> would be nice.
> >
> > This is not doable at the moment, we have no unit test framework to test
> > the trust-add command.
> >
> >> --
> >> Regards,
> >>
> >> Ana Krivokapic
> >> Associate Software Engineer
> >> FreeIPA team
> >> Red Hat Inc.
> >>
> >>
> >> _______________________________________________
> >> Freeipa-devel mailing list
> >> Freeipa-devel@redhat.com
> >> https://www.redhat.com/mailman/listinfo/freeipa-devel
> >
> > Tomas
> 
> I don't know much about AD trusts, but a command-line/API option that 
> takes a 'yes' or 'no' string raised a tiny warning flag for me.
> 
> It looks like it's possible that there can be other range types in the 
> future than posix and algorithmic? If that's the case, there should be a 
> --range-type option instead. (If not, I'd still go for --range-type but 
> that would just be bikeshedding.)
> 
> In any case I think an explicit 'auto' option would be nice.
> 
> But that's just an outsider's view, maybe --use-posix makes more sense.

I replaced --use-posix with more versatile --range-type. It supports only
trust-related values for now, and can be extended.

Rebased patch attached.

> 
> 
> AFAIK, for CLI changes there should be a a design page; is this covered 
> anywhere?
> 

Design page for the umbrella ticket is here:

http://www.freeipa.org/page/V3/Use_posix_attributes_defined_in_AD

> 
> -- 
> Petr³
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
>From 784021ef93694bb19e8a30c509dbbec8cb06cdf0 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Tue, 25 Jun 2013 14:25:44 +0200
Subject: [PATCH] Add --range-type option that forces range type of the trusted
 domain

Adds --range-type option to ipa trust-add command. It takes two
allowed values: 'ipa-ad-trust-posix' and 'ipa-ad-trust'.

When --range-type option is not specified, the range type should be
determined by ID range discovery.

https://fedorahosted.org/freeipa/ticket/3650
---
 API.txt                   |  3 ++-
 VERSION                   |  2 +-
 ipalib/plugins/idrange.py |  4 ++--
 ipalib/plugins/trust.py   | 40 ++++++++++++++++++++++++++++++++++++++--
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/API.txt b/API.txt
index 067955ef77332374bf242655187ef9f912b0c2c0..44b3dd444964c8dac595177f8601c82d0235eabe 100644
--- a/API.txt
+++ b/API.txt
@@ -3278,12 +3278,13 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA
 output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
 output: Output('value', <type 'unicode'>, None)
 command: trust_add
-args: 1,12,3
+args: 1,13,3
 arg: Str('cn', attribute=True, cli_name='realm', multivalue=False, primary_key=True, required=True)
 option: Str('addattr*', cli_name='addattr', exclude='webui')
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
 option: Int('base_id?', cli_name='base_id')
 option: Int('range_size?', autofill=True, cli_name='range_size', default=200000)
+option: StrEnum('range_type?', cli_name='range_type', values=(u'ipa-ad-trust-posix', u'ipa-ad-trust'))
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
 option: Str('realm_admin?', cli_name='admin')
 option: Password('realm_passwd?', cli_name='password', confirm=False)
diff --git a/VERSION b/VERSION
index c713b18b7ce42db7fb290912ec6028342015f142..8606d724e6c8c785ba9d554ed3effa905573e25f 100644
--- a/VERSION
+++ b/VERSION
@@ -89,4 +89,4 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=60
+IPA_API_VERSION_MINOR=61
diff --git a/ipalib/plugins/idrange.py b/ipalib/plugins/idrange.py
index f258cbb15565f62a56017d0909dfaff17202282a..7f8c1ab7b2000065230c3dd5b0263f763ddf398c 100644
--- a/ipalib/plugins/idrange.py
+++ b/ipalib/plugins/idrange.py
@@ -458,12 +458,12 @@ class idrange_add(LDAPCreate):
             entry_attrs['objectclass'].append('ipatrustedaddomainrange')
 
             # Default to ipa-ad-trust if no type set
-            if 'iparangetype' not in entry_attrs:
+            if not is_set('iparangetype'):
                 entry_attrs['iparangetype'] = u'ipa-ad-trust'
 
             if entry_attrs['iparangetype'] not in (u'ipa-ad-trust',
                                                    u'ipa-ad-trust-posix'):
-                raise errors.ValidationError('ID Range setup',
+                raise errors.ValidationError(name='ID Range setup',
                     error=_('IPA Range type must be one of ipa-ad-trust '
                             'or ipa-ad-trust-posix when SID of the trusted '
                             'domain is specified.'))
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index d2b58399f80d0ebe5fbc14df140438a6fa093895..965ff76bb7968a8d2784e67478eb824dc3f0621b 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -259,6 +259,12 @@ this will cause change to trust relationship credentials on both
 sides.
     ''')
 
+    range_types = {
+        u'ipa-ad-trust': unicode(_('Active Directory domain range')),
+        u'ipa-ad-trust-posix': unicode(_('Active Directory trust range with '
+                                        'POSIX attributes')),
+                  }
+
     takes_options = LDAPCreate.takes_options + (
         _trust_type_option,
         Str('realm_admin?',
@@ -289,6 +295,13 @@ sides.
             default=DEFAULT_RANGE_SIZE,
             autofill=True
         ),
+        StrEnum('range_type?',
+            label=_('Range type'),
+            cli_name='range_type',
+            doc=(_('Type of trusted domain ID range, one of {vals}'
+                 .format(vals=', '.join(range_types.keys())))),
+            values=tuple(range_types.keys()),
+        ),
     )
 
     msg_summary = _('Added Active Directory trust for realm "%(value)s"')
@@ -388,13 +401,27 @@ sides.
     def validate_range(self, *keys, **options):
         # If a range for this trusted domain already exists,
         # '--base-id' or '--range-size' options should not be specified
-        range_name = keys[-1].upper()+'_id_range'
+        range_name = keys[-1].upper() + '_id_range'
+        range_type = options.get('range_type')
 
         try:
-            old_range = api.Command['idrange_show'](range_name)
+            old_range = api.Command['idrange_show'](range_name, raw=True)
         except errors.NotFound:
             old_range = None
 
+        if options.get('type') == u'ad':
+            if range_type and range_type not in (u'ipa-ad-trust',
+                                                 u'ipa-ad-trust-posix'):
+                raise errors.ValidationError(
+                    name=_('id range type'),
+                    error=_(
+                        'Only the ipa-ad-trust and ipa-ad-trust-posix are '
+                        'allowed values for --range-type when adding an AD '
+                        'trust.'
+                    )
+
+)
+
         base_id = options.get('base_id')
         range_size = options.get('range_size') != DEFAULT_RANGE_SIZE
 
@@ -420,6 +447,7 @@ sides.
 
         if old_range:
             old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0]
+            old_range_type = old_range['result']['iparangetype'][0]
 
             if old_dom_sid != dom_sid:
                 raise errors.ValidationError(
@@ -431,6 +459,13 @@ sides.
                     )
                 )
 
+            if range_type and range_type != old_range_type:
+                raise errors.ValidationError(name=_('range type change'),
+                    error=_('ID range for the trusted domain already exists, '
+                            'but it has a different type. Please remove the '
+                            'old range manually, or do not enforce type '
+                            'via --range-type option.'))
+
         return old_range, range_name, dom_sid
 
     def add_range(self, range_name, dom_sid, **options):
@@ -448,6 +483,7 @@ sides.
                                    ipabaseid=base_id,
                                    ipaidrangesize=options['range_size'],
                                    ipabaserid=0,
+                                   iparangetype=options.get('range_type'),
                                    ipanttrusteddomainsid=dom_sid)
 
     def execute_ad(self, full_join, *keys, **options):
-- 
1.8.3.1

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to