Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 18.11.2014 18:27, Petr Vobornik wrote: On 18.11.2014 17:27, Nathaniel McCallum wrote: This patch still needs to land in 4.1.2, so is it okay as it is? I don't think the label is necessary but it doesn't hurt either, at least it's clear, so ACK. Pushed to: master: 3c900ba7a8d98a72ff4e040b688fe3213c722a64 ipa-4-1: 1cd2ca11c5b22dc3bd555e63dba69e9475817872 -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Tue, 18 Nov 2014 12:27:28 +0100 Martin Kosek mko...@redhat.com wrote: On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. And needs to be changed, it always was supposed to be temporary, and it is time to change it. now that we have various distributions and not just Fedora with FreeIPA we cannot make the ipa command useless unless you happen to have the same exact version that you have on the server, normally clients are always a few versions ahead of servers which move more slowly. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. No, this would be the wrong way to go. The solution is the same Linux adopted for ABI compatibility in libraries. We version the single API command, all we need to do is add a version field in the json structure (or even just in the command name). Any time people want to change a command a new command is created instead and the old one stays around for older clients. This is how all successful and backwards compatible RPC APIs are built, and we need to follow suite asap. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Tue, 2014-11-18 at 07:45 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 12:27:28 +0100 Martin Kosek mko...@redhat.com wrote: On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. And needs to be changed, it always was supposed to be temporary, and it is time to change it. now that we have various distributions and not just Fedora with FreeIPA we cannot make the ipa command useless unless you happen to have the same exact version that you have on the server, normally clients are always a few versions ahead of servers which move more slowly. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. No, this would be the wrong way to go. The solution is the same Linux adopted for ABI compatibility in libraries. We version the single API command, all we need to do is add a version field in the json structure (or even just in the command name). Any time people want to change a command a new command is created instead and the old one stays around for older clients. This is how all successful and backwards compatible RPC APIs are built, and we need to follow suite asap. +1 However, this is probably 4.2 material, no? If so, let's file a bug and schedule it. This patch still needs to land in 4.1.2, so is it okay as it is? Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 18.11.2014 17:27, Nathaniel McCallum wrote: On Tue, 2014-11-18 at 07:45 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 12:27:28 +0100 Martin Kosek mko...@redhat.com wrote: On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. And needs to be changed, it always was supposed to be temporary, and it is time to change it. now that we have various distributions and not just Fedora with FreeIPA we cannot make the ipa command useless unless you happen to have the same exact version that you have on the server, normally clients are always a few versions ahead of servers which move more slowly. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. No, this would be the wrong way to go. The solution is the same Linux adopted for ABI compatibility in libraries. We version the single API command, all we need to do is add a version field in the json structure (or even just in the command name). Any time people want to change a command a new command is created instead and the old one stays around for older clients. This is how all successful and backwards compatible RPC APIs are built, and we need to follow suite asap. +1 However, this is probably 4.2 material, no? If so, let's file a bug and schedule it. Yes, https://fedorahosted.org/freeipa/ticket/4739 This patch still needs to land in 4.1.2, so is it okay as it is? I don't think the label is necessary but it doesn't hurt either, at least it's clear, so ACK. Nathaniel -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 13.11.2014 19:12, Petr Viktorin wrote:
On 11/13/2014 06:02 PM, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 16:57 +0100, Petr Viktorin wrote:
On 11/13/2014 04:40 PM, Petr Vobornik wrote:
On 13.11.2014 16:19, Nathaniel McCallum wrote:
Like you, I like #2 the best. Attached is an implementation.
I like --no-qrcode as well.
Should we also keep qrcode as 'no_option' to maintain API
compatibility
(but not CLI)?
I don't think it is necessary. It only makes sense to specify --qrcode
in an interactive session.
Makes sense.
ACK
Not pushing yet to give time for NACK if anybody doesn't agree with the
API change.
Hold on, what is happening here?
Aren't all clients since 4.0 sending the qrcode option to the server?
We absolutely can not break backwards compatibility with released
versions.
We also should not break the CLI. Just make it a no-op option, and say
it's deprecated in the doc.
As I understand the current behavior, the qrcode option is *not* sent to
the server by default in any scenario.
Nope, defaults are filled in by the client. (And also on the server if
they're still missing; it's part of the common validation.)
IMHO this is quite unfortunate behavior which may also fail horribly if
there is a newer client and an older server - backwards compatibility
is on API level, not CLI level. Defaults should be filled by server, not
a client. We should seriously reconsider the design of our CLI. But
that's for different, future discussion.
That's said and given the circumstances, it is easier and cleaner to
return the --qrcode back as no_param now than to deal with potential
future issues.
You can try it out, actually:
$ ipa -vv otptoken-add
ipa: INFO: trying https://vm-175.idm.lab.eng.brq.redhat.com/ipa/json
ipa: INFO: Forwarding 'otptoken_add' to json server
'https://vm-175.idm.lab.eng.brq.redhat.com/ipa/json'
ipa: INFO: Request: {
id: 0,
method: otptoken_add,
params: [
[
null
],
{
all: false,
ipatokenhotpcounter: 0,
ipatokenotpalgorithm: sha1,
ipatokenotpdigits: 6,
ipatokenotpkey:
5\ufffdK\ufffd1\u000e\ufffd7,\ufffd_\ufffd\ufffd.0\ufffdM\ufffd\u0016\ufffd,
ipatokentotpclockoffset: 0,
ipatokentotptimestep: 30,
no_members: false,
qrcode: false,
raw: false,
type: totp,
version: 2.108
}
]
}
ipa: INFO: Response: {
error: null,
id: 0,
principal: ad...@idm.lab.eng.brq.redhat.com,
...
--
Petr Vobornik
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Feel free to file a ticket. But yes, redesigning the API is not exactly a priority. That's said and given the circumstances, it is easier and cleaner to return the --qrcode back as no_param now than to deal with potential future issues. What's the reason to break the CLI by making it no_param? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/14/2014 02:25 PM, Petr Vobornik wrote: On 14.11.2014 14:08, Petr Viktorin wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. And that's bad because, IMHO, this case may be more common that a newer server and an older client, e.g., RHEL 6.5 server and Fedora 21 client. Feel free to file a ticket. But yes, redesigning the API is not exactly a priority. That's said and given the circumstances, it is easier and cleaner to return the --qrcode back as no_param now than to deal with potential future issues. What's the reason to break the CLI by making it no_param? Sorry I meant, no_option, there is no no_param. Because it returns it back to Nathaniel's argument about interactive session and I agree with it. Why would we have both --no-qrcode and --qrcode options available on CLI? It breaks the CLI, and for that there should be a better reason than it looks bad. The CLI is an interface as well. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Fri, 2014-11-14 at 17:20 +0100, Petr Viktorin wrote:
On 11/14/2014 02:25 PM, Petr Vobornik wrote:
On 14.11.2014 14:08, Petr Viktorin wrote:
On 11/14/2014 01:18 PM, Petr Vobornik wrote:
[...]
Nope, defaults are filled in by the client. (And also on the server if
they're still missing; it's part of the common validation.)
IMHO this is quite unfortunate behavior which may also fail horribly if
there is a newer client and an older server - backwards compatibility
is on API level, not CLI level. Defaults should be filled by server, not
a client. We should seriously reconsider the design of our CLI. But
that's for different, future discussion.
You can't use a newer client with an older server, you get a
VersionError in that case.
And that's bad because, IMHO, this case may be more common that a newer
server and an older client, e.g., RHEL 6.5 server and Fedora 21 client.
Feel free to file a ticket. But yes, redesigning the API is not exactly
a priority.
That's said and given the circumstances, it is easier and cleaner to
return the --qrcode back as no_param now than to deal with potential
future issues.
What's the reason to break the CLI by making it no_param?
Sorry I meant, no_option, there is no no_param. Because it returns it
back to Nathaniel's argument about interactive session and I agree with
it. Why would we have both --no-qrcode and --qrcode options available on
CLI?
It breaks the CLI, and for that there should be a better reason than it
looks bad. The CLI is an interface as well.
The attached patch retains --qrcode but specified as no_option. It is
marked as deprecated.
From 51be0dcf3003a992909da935a0d2529b8768900e Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum npmccal...@redhat.com
Date: Thu, 6 Nov 2014 15:30:13 -0500
Subject: [PATCH] Enable QR code display by default in otptoken-add
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
---
API.txt | 3 ++-
VERSION | 4 ++--
ipalib/plugins/otptoken.py | 5 +++--
ipalib/plugins/otptoken_yubikey.py | 1 +
ipaserver/install/ipa_otptoken_import.py | 2 +-
5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/API.txt b/API.txt
index 491d7a76fd1d2d50208d314d1600839ce295..2a63f1e2349f0df69433fa7cb742e269cd42d79f 100644
--- a/API.txt
+++ b/API.txt
@@ -2592,7 +2592,7 @@ output: Entry('result', type 'dict', Gettext('A dictionary representing an LDA
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
command: otptoken_add
-args: 1,22,3
+args: 1,23,3
arg: Str('ipatokenuniqueid', attribute=True, cli_name='id', multivalue=False, primary_key=True, required=False)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
@@ -2611,6 +2611,7 @@ option: Int('ipatokentotpclockoffset', attribute=True, autofill=True, cli_name='
option: Int('ipatokentotptimestep', attribute=True, autofill=True, cli_name='interval', default=30, minvalue=5, multivalue=False, required=False)
option: Str('ipatokenvendor', attribute=True, cli_name='vendor', multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
+option: Flag('no_qrcode', autofill=True, default=False)
option: Flag('qrcode?', autofill=True, default=False)
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('setattr*', cli_name='setattr', exclude='webui')
diff --git a/VERSION b/VERSION
index b0d41e5e1ec59ddefbdcccf588b97bac2ff798ee..bae782a4ec4333f8fdb610465a7b9ea3877c990e 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
# #
IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=108
-# Last change: pvoborni - manage authorization of keytab operations
+IPA_API_VERSION_MINOR=109
+# Last change: npmccallum - display qrcode by default
diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index f48f0502992f1b5fed4f342cace1c404624b..f0850854f98e84e44acdcef311225220ac0129a3 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -268,7 +268,8 @@ class otptoken_add(LDAPCreate):
msg_summary = _('Added OTP token %(value)s')
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode?', label=_('(deprecated)'), flags=('no_option')),
+Flag('no_qrcode', label=_('Do not display QR code'), default=False),
)
has_output_params = LDAPCreate.has_output_params + (
@@
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. If a Fedora 21 client can't talk to a RHEL 6 server we have a huge problem that we need to fix *yesterday*. Feel free to file a ticket. But yes, redesigning the API is not exactly a priority. That's said and given the circumstances, it is easier and cleaner to return the --qrcode back as no_param now than to deal with potential future issues. What's the reason to break the CLI by making it no_param? -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. If a Fedora 21 client can't talk to a RHEL 6 server we have a huge problem that we need to fix *yesterday*. Then we have a huge task on our hands. [0] ticket https://fedorahosted.org/freeipa/ticket/584 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. If a Fedora 21 client can't talk to a RHEL 6 server we have a huge problem that we need to fix *yesterday*. Then we have a huge task on our hands. [0] ticket https://fedorahosted.org/freeipa/ticket/584 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
I am not reviewing, just curious. What is the purpose of --qrcode option with
default=True?
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
How can user turn it off? Both passing no option and passing --qrcode will get
the same result, no? :-)
Shouldn't we instead change --qrcode to --no-qrcode and process somehow fill
qrcode=0 when it's enabled? (To achieve API compatibility with old clients).
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
I am not reviewing, just curious. What is the purpose of --qrcode option with
default=True?
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
How can user turn it off? Both passing no option and passing --qrcode will get
the same result, no? :-)
Shouldn't we instead change --qrcode to --no-qrcode and process somehow fill
qrcode=0 when it's enabled? (To achieve API compatibility with old clients).
I see three options:
1. Don't let the user turn it off from the command line (he can still
turn it off from the Python API).
2. Change it to --no-qrcode (API change)
3. Change the type to Bool (API change)
Like you, I like #2 the best. Attached is an implementation.
From 816d3d5f98b78a8b82d81ef2f7162614b8ead4b2 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum npmccal...@redhat.com
Date: Thu, 6 Nov 2014 15:30:13 -0500
Subject: [PATCH] Enable QR code display by default in otptoken-add
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
---
API.txt | 2 +-
VERSION | 4 ++--
ipalib/plugins/otptoken.py | 4 ++--
ipalib/plugins/otptoken_yubikey.py | 1 +
ipaserver/install/ipa_otptoken_import.py | 2 +-
5 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/API.txt b/API.txt
index 491d7a76fd1d2d50208d314d1600839ce295..357c0b1aeaafcacf8d82d57fbfb3fabc21ec8f93 100644
--- a/API.txt
+++ b/API.txt
@@ -2611,7 +2611,7 @@ option: Int('ipatokentotpclockoffset', attribute=True, autofill=True, cli_name='
option: Int('ipatokentotptimestep', attribute=True, autofill=True, cli_name='interval', default=30, minvalue=5, multivalue=False, required=False)
option: Str('ipatokenvendor', attribute=True, cli_name='vendor', multivalue=False, required=False)
option: Flag('no_members', autofill=True, default=False, exclude='webui')
-option: Flag('qrcode?', autofill=True, default=False)
+option: Flag('no_qrcode', autofill=True, default=False)
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('setattr*', cli_name='setattr', exclude='webui')
option: StrEnum('type', attribute=False, autofill=True, cli_name='type', default=u'totp', multivalue=False, required=False, values=(u'totp', u'hotp', u'TOTP', u'HOTP'))
diff --git a/VERSION b/VERSION
index b0d41e5e1ec59ddefbdcccf588b97bac2ff798ee..bae782a4ec4333f8fdb610465a7b9ea3877c990e 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
# #
IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=108
-# Last change: pvoborni - manage authorization of keytab operations
+IPA_API_VERSION_MINOR=109
+# Last change: npmccallum - display qrcode by default
diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 2b5f1c5fb83341d392e165a3507f5076820f1d3a..dc687e37b398fb413cd565ac9793a013beda8504 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -249,7 +249,7 @@ class otptoken_add(LDAPCreate):
msg_summary = _('Added OTP token %(value)s')
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('no_qrcode', label=_('Do not display QR code'), default=False),
)
has_output_params = LDAPCreate.has_output_params + (
@@ -329,7 +329,7 @@ class otptoken_add(LDAPCreate):
rv = super(otptoken_add, self).output_for_cli(textui, output, *args, **options)
# Print QR code to terminal if specified
-if uri and options.get('qrcode', False):
+if uri and not options.get('no_qrcode', False):
print \n
qr = qrcode.QRCode()
qr.add_data(uri)
diff --git a/ipalib/plugins/otptoken_yubikey.py b/ipalib/plugins/otptoken_yubikey.py
index e70ddb6e42b5ea34d7ebecb252d6bbd73ac64d03..7095887ac7cdf5d4b7d0d30edc6cab046664 100644
--- a/ipalib/plugins/otptoken_yubikey.py
+++ b/ipalib/plugins/otptoken_yubikey.py
@@ -124,6 +124,7 @@ class otptoken_add_yubikey(Command):
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 13.11.2014 16:00, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
I am not reviewing, just curious. What is the purpose of --qrcode option with
default=True?
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
How can user turn it off? Both passing no option and passing --qrcode will get
the same result, no? :-)
Shouldn't we instead change --qrcode to --no-qrcode and process somehow fill
qrcode=0 when it's enabled? (To achieve API compatibility with old clients).
I see three options:
1. Don't let the user turn it off from the command line (he can still
turn it off from the Python API).
2. Change it to --no-qrcode (API change)
3. Change the type to Bool (API change)
Like you, I like #2 the best. Attached is an implementation.
I like --no-qrcode as well.
Should we also keep qrcode as 'no_option' to maintain API compatibility
(but not CLI)?
--
Petr Vobornik
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Thu, 2014-11-13 at 16:13 +0100, Petr Vobornik wrote:
On 13.11.2014 16:00, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
I am not reviewing, just curious. What is the purpose of --qrcode option
with
default=True?
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
How can user turn it off? Both passing no option and passing --qrcode will
get
the same result, no? :-)
Shouldn't we instead change --qrcode to --no-qrcode and process somehow
fill
qrcode=0 when it's enabled? (To achieve API compatibility with old
clients).
I see three options:
1. Don't let the user turn it off from the command line (he can still
turn it off from the Python API).
2. Change it to --no-qrcode (API change)
3. Change the type to Bool (API change)
Like you, I like #2 the best. Attached is an implementation.
I like --no-qrcode as well.
Should we also keep qrcode as 'no_option' to maintain API compatibility
(but not CLI)?
I don't think it is necessary. It only makes sense to specify --qrcode
in an interactive session.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/13/2014 04:40 PM, Petr Vobornik wrote:
On 13.11.2014 16:19, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 16:13 +0100, Petr Vobornik wrote:
On 13.11.2014 16:00, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a
standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
I am not reviewing, just curious. What is the purpose of --qrcode
option with
default=True?
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
How can user turn it off? Both passing no option and passing
--qrcode will get
the same result, no? :-)
Shouldn't we instead change --qrcode to --no-qrcode and process
somehow fill
qrcode=0 when it's enabled? (To achieve API compatibility with old
clients).
I see three options:
1. Don't let the user turn it off from the command line (he can still
turn it off from the Python API).
2. Change it to --no-qrcode (API change)
3. Change the type to Bool (API change)
Like you, I like #2 the best. Attached is an implementation.
I like --no-qrcode as well.
Should we also keep qrcode as 'no_option' to maintain API compatibility
(but not CLI)?
I don't think it is necessary. It only makes sense to specify --qrcode
in an interactive session.
Makes sense.
ACK
Not pushing yet to give time for NACK if anybody doesn't agree with the
API change.
Hold on, what is happening here?
Aren't all clients since 4.0 sending the qrcode option to the server?
We absolutely can not break backwards compatibility with released versions.
We also should not break the CLI. Just make it a no-op option, and say
it's deprecated in the doc.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Thu, 2014-11-13 at 16:57 +0100, Petr Viktorin wrote:
On 11/13/2014 04:40 PM, Petr Vobornik wrote:
On 13.11.2014 16:19, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 16:13 +0100, Petr Vobornik wrote:
On 13.11.2014 16:00, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a
standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
I am not reviewing, just curious. What is the purpose of --qrcode
option with
default=True?
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
How can user turn it off? Both passing no option and passing
--qrcode will get
the same result, no? :-)
Shouldn't we instead change --qrcode to --no-qrcode and process
somehow fill
qrcode=0 when it's enabled? (To achieve API compatibility with old
clients).
I see three options:
1. Don't let the user turn it off from the command line (he can still
turn it off from the Python API).
2. Change it to --no-qrcode (API change)
3. Change the type to Bool (API change)
Like you, I like #2 the best. Attached is an implementation.
I like --no-qrcode as well.
Should we also keep qrcode as 'no_option' to maintain API compatibility
(but not CLI)?
I don't think it is necessary. It only makes sense to specify --qrcode
in an interactive session.
Makes sense.
ACK
Not pushing yet to give time for NACK if anybody doesn't agree with the
API change.
Hold on, what is happening here?
Aren't all clients since 4.0 sending the qrcode option to the server?
We absolutely can not break backwards compatibility with released versions.
We also should not break the CLI. Just make it a no-op option, and say
it's deprecated in the doc.
As I understand the current behavior, the qrcode option is *not* sent to
the server by default in any scenario. The only thing this change would
break would be if someone had scripted on top of this and had manually
specified the qrcode option. I think this is extremely unlikely as
scripts are much more likely to want to disable the qrcode output.
Nathaniel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/13/2014 06:02 PM, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 16:57 +0100, Petr Viktorin wrote:
On 11/13/2014 04:40 PM, Petr Vobornik wrote:
On 13.11.2014 16:19, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 16:13 +0100, Petr Vobornik wrote:
On 13.11.2014 16:00, Nathaniel McCallum wrote:
On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a
standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
I am not reviewing, just curious. What is the purpose of --qrcode
option with
default=True?
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
How can user turn it off? Both passing no option and passing
--qrcode will get
the same result, no? :-)
Shouldn't we instead change --qrcode to --no-qrcode and process
somehow fill
qrcode=0 when it's enabled? (To achieve API compatibility with old
clients).
I see three options:
1. Don't let the user turn it off from the command line (he can still
turn it off from the Python API).
2. Change it to --no-qrcode (API change)
3. Change the type to Bool (API change)
Like you, I like #2 the best. Attached is an implementation.
I like --no-qrcode as well.
Should we also keep qrcode as 'no_option' to maintain API compatibility
(but not CLI)?
I don't think it is necessary. It only makes sense to specify --qrcode
in an interactive session.
Makes sense.
ACK
Not pushing yet to give time for NACK if anybody doesn't agree with the
API change.
Hold on, what is happening here?
Aren't all clients since 4.0 sending the qrcode option to the server?
We absolutely can not break backwards compatibility with released versions.
We also should not break the CLI. Just make it a no-op option, and say
it's deprecated in the doc.
As I understand the current behavior, the qrcode option is *not* sent to
the server by default in any scenario.
Nope, defaults are filled in by the client. (And also on the server if
they're still missing; it's part of the common validation.)
You can try it out, actually:
$ ipa -vv otptoken-add
ipa: INFO: trying https://vm-175.idm.lab.eng.brq.redhat.com/ipa/json
ipa: INFO: Forwarding 'otptoken_add' to json server
'https://vm-175.idm.lab.eng.brq.redhat.com/ipa/json'
ipa: INFO: Request: {
id: 0,
method: otptoken_add,
params: [
[
null
],
{
all: false,
ipatokenhotpcounter: 0,
ipatokenotpalgorithm: sha1,
ipatokenotpdigits: 6,
ipatokenotpkey:
5\ufffdK\ufffd1\u000e\ufffd7,\ufffd_\ufffd\ufffd.0\ufffdM\ufffd\u0016\ufffd,
ipatokentotpclockoffset: 0,
ipatokentotptimestep: 30,
no_members: false,
qrcode: false,
raw: false,
type: totp,
version: 2.108
}
]
}
ipa: INFO: Response: {
error: null,
id: 0,
principal: ad...@idm.lab.eng.brq.redhat.com,
...
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
I removed the period from the doc string to maintain consistency with
the rest of the plugin.
Nathaniel
From 24f8c5abe5c576e7399ce10b6634190bb67760d3 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum npmccal...@redhat.com
Date: Thu, 6 Nov 2014 15:30:13 -0500
Subject: [PATCH] Enable QR code display by default in otptoken-add
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
---
ipalib/plugins/otptoken.py | 4 ++--
ipalib/plugins/otptoken_yubikey.py | 1 +
ipaserver/install/ipa_otptoken_import.py | 2 +-
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 2b5f1c5fb83341d392e165a3507f5076820f1d3a..ead6fb40884157b31305c0d8fe540715ecf6a2a2 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -249,7 +249,7 @@ class otptoken_add(LDAPCreate):
msg_summary = _('Added OTP token %(value)s')
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code'), default=True),
)
has_output_params = LDAPCreate.has_output_params + (
@@ -329,7 +329,7 @@ class otptoken_add(LDAPCreate):
rv = super(otptoken_add, self).output_for_cli(textui, output, *args, **options)
# Print QR code to terminal if specified
-if uri and options.get('qrcode', False):
+if uri and options['qrcode']:
print \n
qr = qrcode.QRCode()
qr.add_data(uri)
diff --git a/ipalib/plugins/otptoken_yubikey.py b/ipalib/plugins/otptoken_yubikey.py
index e70ddb6e42b5ea34d7ebecb252d6bbd73ac64d03..643096a507121f3596d7d3798bff024c08d738fb 100644
--- a/ipalib/plugins/otptoken_yubikey.py
+++ b/ipalib/plugins/otptoken_yubikey.py
@@ -124,6 +124,7 @@ class otptoken_add_yubikey(Command):
ipatokenotpalgorithm=u'sha1',
ipatokenhotpcounter=0,
ipatokenotpkey=key,
+qrcode=False,
**options)
# Suppress values we don't want to return.
diff --git a/ipaserver/install/ipa_otptoken_import.py b/ipaserver/install/ipa_otptoken_import.py
index 31a6902014b8e3b2aafb3ba98a4190dc2059a3e7..70a38867c64674ce5879be6b7b03270856a31c77 100644
--- a/ipaserver/install/ipa_otptoken_import.py
+++ b/ipaserver/install/ipa_otptoken_import.py
@@ -517,7 +517,7 @@ class OTPTokenImport(admintool.AdminTool):
# Parse tokens
for keypkg in self.doc.getKeyPackages():
try:
-api.Command.otptoken_add(keypkg.id, **keypkg.options)
+api.Command.otptoken_add(keypkg.id, qrcode=False, **keypkg.options)
except Exception as e:
self.log.warn(Error adding token: %s, e)
else:
--
2.1.0
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
From 86d9c7f6ec82db1c4b29e97c0529970e888d7bb8 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum npmccal...@redhat.com
Date: Thu, 6 Nov 2014 15:30:13 -0500
Subject: [PATCH] Enable QR code display by default in otptoken-add
This is possible because python-qrcode's output now fits in a standard
terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
disable QR code output as it doesn't make sense in these contexts.
https://fedorahosted.org/freeipa/ticket/4703
---
ipalib/plugins/otptoken.py | 4 ++--
ipalib/plugins/otptoken_yubikey.py | 1 +
ipaserver/install/ipa_otptoken_import.py | 2 +-
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 2b5f1c5fb83341d392e165a3507f5076820f1d3a..f16bce04f5274c3e519856512025e38b3995a4f4 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -249,7 +249,7 @@ class otptoken_add(LDAPCreate):
msg_summary = _('Added OTP token %(value)s')
takes_options = LDAPCreate.takes_options + (
-Flag('qrcode?', label=_('Display QR code')),
+Flag('qrcode', label=_('Display QR code.'), default=True),
)
has_output_params = LDAPCreate.has_output_params + (
@@ -329,7 +329,7 @@ class otptoken_add(LDAPCreate):
rv = super(otptoken_add, self).output_for_cli(textui, output, *args, **options)
# Print QR code to terminal if specified
-if uri and options.get('qrcode', False):
+if uri and options['qrcode']:
print \n
qr = qrcode.QRCode()
qr.add_data(uri)
diff --git a/ipalib/plugins/otptoken_yubikey.py b/ipalib/plugins/otptoken_yubikey.py
index e70ddb6e42b5ea34d7ebecb252d6bbd73ac64d03..643096a507121f3596d7d3798bff024c08d738fb 100644
--- a/ipalib/plugins/otptoken_yubikey.py
+++ b/ipalib/plugins/otptoken_yubikey.py
@@ -124,6 +124,7 @@ class otptoken_add_yubikey(Command):
ipatokenotpalgorithm=u'sha1',
ipatokenhotpcounter=0,
ipatokenotpkey=key,
+qrcode=False,
**options)
# Suppress values we don't want to return.
diff --git a/ipaserver/install/ipa_otptoken_import.py b/ipaserver/install/ipa_otptoken_import.py
index 31a6902014b8e3b2aafb3ba98a4190dc2059a3e7..70a38867c64674ce5879be6b7b03270856a31c77 100644
--- a/ipaserver/install/ipa_otptoken_import.py
+++ b/ipaserver/install/ipa_otptoken_import.py
@@ -517,7 +517,7 @@ class OTPTokenImport(admintool.AdminTool):
# Parse tokens
for keypkg in self.doc.getKeyPackages():
try:
-api.Command.otptoken_add(keypkg.id, **keypkg.options)
+api.Command.otptoken_add(keypkg.id, qrcode=False, **keypkg.options)
except Exception as e:
self.log.warn(Error adding token: %s, e)
else:
--
2.1.0
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
