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

Reply via email to