On 13.11.2014 08:53, Martin Kosek wrote:
On 11/13/2014 08:51 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 08:48 +0100, Martin Kosek wrote:
On 11/12/2014 11:37 PM, Nathaniel McCallum wrote:
On Mon, 2014-11-10 at 08:28 +0100, Martin Kosek wrote:
On 11/07/2014 04:44 PM, Petr Vobornik wrote:
On 7.11.2014 08:58, Martin Kosek wrote:
On 11/04/2014 05:17 PM, Nathaniel McCallum wrote:
On Wed, 2014-10-29 at 09:34 -0400, Nathaniel McCallum wrote:
On Wed, 2014-10-29 at 12:21 +0100, Petr Viktorin wrote:
On 10/29/2014 10:37 AM, Martin Kosek wrote:
On 10/28/2014 09:59 PM, Nathaniel McCallum wrote:
On Thu, 2014-10-23 at 18:07 -0400, Nathaniel McCallum wrote:
This patch gives the administrator variables to control the size of
the authentication and synchronization windows for OTP tokens.

https://fedorahosted.org/freeipa/ticket/4511

NOTE: There is one known issue with this patch which I don't know
how to
solve. This patch changes the schema in
install/share/60ipaconfig.ldif.
On an upgrade, all of the new attributeTypes appear correctly.
However,
the modifications to the pre-existing objectClass do not show up
on the
server. What am I doing wrong?

After modifying ipaGuiConfig manually, everything in this patch
works
just fine.

This new version takes into account the new (proper) OIDs and
attribute
names.

Thanks Nathaniel!

The above known issue still remains.

Petr3, any idea what could have gone wrong? ObjectClass MAY list
extension
should work just fine, AFAIK.

You added a blank line to the LDIF file. This is an entry separator, so
the objectClasses after the blank line don't belong to cn=schema, so
they aren't considered in the update.
Without the blank line it works fine.

Thanks for the catch!

Here is a version without the blank line.

I forgot to remove the old steps defines. This patch performs this
cleanup.

I am now wondering, is the global config object really the nest place to
add these OTP specific settings?

I would prefer not to overload the object and instead:
- create new ipaOTPConfig objectclass
- add it to cn=otp,$SUFFIX
- create otpconfig-mod and otpconfig-show commands to follow an example
of dnsconfig-* and trustconfig-* commands

IMO, this would allow more flexibility for the OTP settings and would
also scale better for the future updates.

+1

I will comment the patch as if ^^ would not exist because it will still be
needed in the new plugin.

Because of ^^ I did not test, just read.

1. Got:
install/ui/src/freeipa/serverconfig.js(135): lint warning: extra comma is not
recommended in array initializers

Please run:
   jsl -nofilelisting -nosummary -nologo -conf jsl.conf
in install/ui directory

The goal is no have no warnings and errors.

2. new attrs should be added to 'System: Read Global Configuration' managed
permission

+1. Though if we go with OTP config, it should be called

System: Read OTP Configuration

Martin

Attached is a new set of patches that replaces this single patch. This
now fixes multiple issues.

I now create two new entries:
  * cn=TOTP,cn=Token Config,cn=etc,$SUFFIX
  * cn=HOTP,cn=Token Config,cn=etc,$SUFFIX

There are two corresponding CLI commands:
  * totpconfig-(show|mod)
  * hotpconfig-(show|mod)

There is no UI support for this yet (pointers welcome).

This is designed so that eventually tokens can grow a per-token
override, but I have not yet implemented this feature (it should be easy
in the future).

Additionally, I had to do some shared refactoring to address issues in
ipa-otp-lasttoken, which is why all of these are now merged into a
single patch set.

Nathaniel

I'm little confused with a state of reviews. Thierry were some of the patches ACKed in different threads or are they under review (I'm not reviewing DS plugin parts)?




Ah, I meant adding the token config to cn=otp,SUFFIX directly, but if we want
to make TOTP/HOTP token config as separate entries (to enable future per-token
overrides), your approach should make sense. Rather adding Rob to CC for sanity.

That would work too. I'm open to that.

I am just not sure we should create them as separate plugins, I think the new
commands should be rather added to otp plugin directly so that they show in
"ipa help otptoken" instead of adding 2 new topics just for OTP config.

I can play with that.

Do you plan to change it? I like the idea of a single point of help for OTP but I'm also unsure about the length of the commands. Current solution is also more consistent with a rest of the framework. Would it be something like:

  otptoken-totpconfig-(show|mod)
  otptoken-hotpconfig-(show|mod)

Maybe it would be better to introduce more help topics for otp. This concept is used for HBAC already:

  $ ipa help hbac
    hbacsvcgroup  HBAC Service Groups
    hbacsvc       HBAC Services
    hbacrule      Host-based access control

  $ ipa help hbacrule
  Host-based access control
  ... a lot of text

So we could introduce otp umbrella topic:

  $ ipa help otp
    opttoken     OTP tokens'
    totpconfig   TOTP configuration options
    hotpconfig   HOTP configuration options



Nathaniel

No worries ATM, you can wait for proper review. I was just looking at the new
API to make sure we are on the same page - we seem to mostly are.

Martin


Commenting just patch 0004:

1. Requires rebase because of API change.

2. git diff HEAD~4 -U0 | pep8 --diff
I would ignore E124 and fix E302 (5x)

I did not test actual functionality yet.
--
Petr Vobornik

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

Reply via email to