Re: [gnus git] branch tzz-auth-source-rewrite created: =0=

Re: [gnus git] branch tzz-auth-source-rewrite created: =0=

[Ted Zlatanov]

On Fri, 04 Feb 2011 17:50:49 +0100 Lars Ingebrigtsen <larsi@gnus.org> wrote: 

LI>         at  c29bed20521bd1925c62ef3a340bafca5b7be931 (commit)
LI> - Log -----------------------------------------------------------------
LI> commit c29bed20521bd1925c62ef3a340bafca5b7be931
LI> Merge: 9b636e0 829f8a0
LI> Author: Ted Zlatanov <tzz@lifelogs.com>
LI> Date:   Fri Feb 4 10:49:56 2011 -0600

LI>     Merge branch 'master' of https://git.gnus.org/gnus into tzz-auth-source-rewrite

This is a branch for my auth-source.el rewrite.  It has the latest
working version.  Still missing:

- Secrets API support (ETA this weekend)
- manual update (ETA eventually)
- backwards compatibility with `auth-source-user-or-password' (ETA when bugs are filed)

Any of those contributions will be gratefully received if anyone is
feeling frisky.

Thanks
Ted

tzz-auth-source-rewrite branch (was: [gnus git] branch tzz-auth-source-rewrite created: =0=)

[Ted Zlatanov]

On Fri, 04 Feb 2011 10:55:09 -0600 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> This is a branch for my auth-source.el rewrite.  It has the latest
TZ> working version.

I added Secrets API support (search only, no create or delete) and
`auth-source-user-or-password' compatibility, plus I rebased the
branch.  I think the Secrets API should use the :max parameter if
possible so we don't get too many results at the top level.  Also it
seems quite slow to get the results one by one so maybe we can optimize
`secrets-search-items'.  Finally, Google Chrome stores passwords in
there but with a different scheme.  I wonder if it's useful to add
specific support for mapping those to the auth-source tokens (host,
protocol, user) or if I should put that special code in url.el only.

The `auth-source-user-or-password' wrapper tries to create an entry
currently, which is not OK.  So it's not ready for use.  When I think
it's OK, I'll update the manual and merge the branch back into the
master branch.

Ted

Re: tzz-auth-source-rewrite branch

[Lars Ingebrigtsen]

Ted Zlatanov <tzz@lifelogs.com> writes:

> The `auth-source-user-or-password' wrapper tries to create an entry
> currently, which is not OK.  So it's not ready for use.  When I think
> it's OK, I'll update the manual and merge the branch back into the
> master branch.

Sounds good.  Let me know when it's ready to use, and we can start
adapting nnimap and/or nntp to use it.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: tzz-auth-source-rewrite branch

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

Ted Zlatanov <tzz@lifelogs.com> writes:

> I added Secrets API support (search only, no create or delete) and
> `auth-source-user-or-password' compatibility, plus I rebased the
> branch.  I think the Secrets API should use the :max parameter if
> possible so we don't get too many results at the top level.  Also it
> seems quite slow to get the results one by one so maybe we can optimize
> `secrets-search-items'.

`secrets-search-items' returns already a list of results. It is slow to
get all attributes and secret strings of the items sequentially;
unfortunately there is no D-Bus method to get them in a bunch (for
several items at once).

I've changed `auth-source-secrets-search' such a way that it does not call
`secrets-get-secret', this call is moved to the returned function. This
should reduce the number of D-Bus calls in `auth-source-secrets-search'.

>  Finally, Google Chrome stores passwords in there but with a different
> scheme.  I wonder if it's useful to add specific support for mapping
> those to the auth-source tokens (host, protocol, user) or if I should
> put that special code in url.el only.

This is a disadvantage of the Secret Service API (IMO): it defines
access methods for the storage, but it does not define default
keys/attributes. Every application is free to use its own
attributes. For reuse of existing, we must either do some assumptions,
or we must inspect which attributes are already used, and apply them.

> The `auth-source-user-or-password' wrapper tries to create an entry
> currently, which is not OK.  So it's not ready for use.  When I think
> it's OK, I'll update the manual and merge the branch back into the
> master branch.

I haven't tested this function (yet).

My patch is enclosed.

> Ted

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 3498 bytes --]

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index d7cc274..f7f4803 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,11 @@
+2011-02-06  Michael Albinus  <michael.albinus@gmx.de>
+
+	* auth-source.el (top): Require 'eieio unconditionally.  Autoload
+	`secrets-get-attributes' instead of `secrets-get-attribute'.
+	(auth-source-secrets-search): Limit search when `max' is greater than
+	number of results.  The function returned calls `secrets-get-secret',
+	in order to improve performance of the search function.
+
 2011-02-06  Julien Danjou  <julien@danjou.info>
 
 	* message.el (message-setup-1): Handle message-generate-headers-first
diff --git a/lisp/auth-source.el b/lisp/auth-source.el
index f7b5591..e959102 100644
--- a/lisp/auth-source.el
+++ b/lisp/auth-source.el
@@ -43,12 +43,12 @@
 (require 'netrc)
 (require 'assoc)
 (eval-when-compile (require 'cl))
-(eval-when-compile (require 'eieio))
+(require 'eieio)
 
 (autoload 'secrets-create-item "secrets")
 (autoload 'secrets-delete-item "secrets")
 (autoload 'secrets-get-alias "secrets")
-(autoload 'secrets-get-attribute "secrets")
+(autoload 'secrets-get-attributes "secrets")
 (autoload 'secrets-get-secret "secrets")
 (autoload 'secrets-list-collections "secrets")
 (autoload 'secrets-search-items "secrets")
@@ -305,8 +305,8 @@ If the value is not a list, symmetric encryption will be used."
     ((and
       (not (null (plist-get entry :source))) ; the source must not be nil
       (listp (plist-get entry :source))      ; and it must be a list
-          (require 'secrets nil t)           ; and we must load the Secrets API
-          secrets-enabled)                   ; and that API must be enabled
+      (require 'secrets nil t)               ; and we must load the Secrets API
+      secrets-enabled)                       ; and that API must be enabled
 
      ;; the source is either the :secrets key in ENTRY or
      ;; if that's missing or nil, it's "session"
@@ -861,6 +861,7 @@ See `auth-source-search' for details on SPEC."
 ;;; (let ((auth-sources '(default))) (auth-source-search :max 1 :create t))
 ;;; (let ((auth-sources '(default))) (auth-source-search :max 1 :delete t))
 ;;; (let ((auth-sources '(default))) (auth-source-search :max 1))
+;;; (let ((auth-sources '(default))) (auth-source-search))
 ;;; (let ((auth-sources '("secrets:login"))) (auth-source-search :max 1))
 
 (defun* auth-source-secrets-search (&rest
@@ -911,15 +912,16 @@ TODO: Example."
                                   (not (string-match label item)))
                       collect item))
          ;; TODO: respect max in `secrets-search-items', not after the fact
-         (items (subseq items 0 max))
+         (items (subseq items 0 (min (length items) max)))
          ;; convert the item name to a full plist
          (items (mapcar (lambda (item)
                           (nconc
                            ;; make an entry for the secret (password) element
                            (list
                             :secret
-                            (lexical-let ((v (secrets-get-secret coll item)))
-                              (lambda () v)))
+                            (lexical-let ((coll coll)
+					  (item item))
+                              (lambda () (secrets-get-secret coll item))))
                            ;; rewrite the entry from ((k1 v1) (k2 v2)) to plist
                            (mapcan (lambda (entry)
                                      (list (car entry) (cdr entry)))

Re: tzz-auth-source-rewrite branch

[Ted Zlatanov]

On Sun, 06 Feb 2011 19:38:45 +0100 Michael Albinus <michael.albinus@gmx.de> wrote: 

MA> Ted Zlatanov <tzz@lifelogs.com> writes:
>> I added Secrets API support (search only, no create or delete) and
>> `auth-source-user-or-password' compatibility, plus I rebased the
>> branch.  I think the Secrets API should use the :max parameter if
>> possible so we don't get too many results at the top level.  Also it
>> seems quite slow to get the results one by one so maybe we can optimize
>> `secrets-search-items'.

MA> `secrets-search-items' returns already a list of results. It is slow to
MA> get all attributes and secret strings of the items sequentially;
MA> unfortunately there is no D-Bus method to get them in a bunch (for
MA> several items at once).

I'll leave it that way for now because it will be cached, but that seems
like a painful limitation long-term.

Am I correct in assuming that substring/regex searches on the attributes
are not possible?

MA> I've changed `auth-source-secrets-search' such a way that it does not call
MA> `secrets-get-secret', this call is moved to the returned function. This
MA> should reduce the number of D-Bus calls in
MA> `auth-source-secrets-search'.

Yes, but now every time the user wants the secret they will get a
surprise call and caching won't work.  So I would prefer to call
`secrets-get-secret' early.  I left that bit out of the patch, I hope
you don't mind.

>> Finally, Google Chrome stores passwords in there but with a different
>> scheme.  I wonder if it's useful to add specific support for mapping
>> those to the auth-source tokens (host, protocol, user) or if I should
>> put that special code in url.el only.

MA> This is a disadvantage of the Secret Service API (IMO): it defines
MA> access methods for the storage, but it does not define default
MA> keys/attributes. Every application is free to use its own
MA> attributes. For reuse of existing, we must either do some assumptions,
MA> or we must inspect which attributes are already used, and apply them.

I think we should support the Google Chrome schema, at least:

username_value => user
origin_url => protocol (using the protocol piece of the URL) and host
(using the host piece)

Those values won't work for creation (meaning we won't be able to create
valid Chrome entries without user assistance), but at least there's a
good chance that the user can reuse their Chrome passwords.  Is that
reasonable?  Do you know of any other software that has its own schema
like Chrome does?

Ted

p.s. Sorry about the multiple merge and commit messages on this branch.

Re: tzz-auth-source-rewrite branch

[Michael Albinus]

Ted Zlatanov <tzz@lifelogs.com> writes:

> MA> `secrets-search-items' returns already a list of results. It is slow to
> MA> get all attributes and secret strings of the items sequentially;
> MA> unfortunately there is no D-Bus method to get them in a bunch (for
> MA> several items at once).
>
> I'll leave it that way for now because it will be cached, but that seems
> like a painful limitation long-term.

Yes. I'll grab whether we could do it better.

> Am I correct in assuming that substring/regex searches on the attributes
> are not possible?

Yes. The Secret Service API claims:

"During a lookup, attribute names and values are matched via
 case-sensitive string equality."

> MA> I've changed `auth-source-secrets-search' such a way that it does not call
> MA> `secrets-get-secret', this call is moved to the returned function. This
> MA> should reduce the number of D-Bus calls in
> MA> `auth-source-secrets-search'.
>
> Yes, but now every time the user wants the secret they will get a
> surprise call and caching won't work.  So I would prefer to call
> `secrets-get-secret' early.  I left that bit out of the patch, I hope
> you don't mind.

No problem. Mid-term, we could improve the returned function to
check/add caches as well.

> MA> This is a disadvantage of the Secret Service API (IMO): it defines
> MA> access methods for the storage, but it does not define default
> MA> keys/attributes. Every application is free to use its own
> MA> attributes. For reuse of existing, we must either do some assumptions,
> MA> or we must inspect which attributes are already used, and apply them.
>
> I think we should support the Google Chrome schema, at least:
>
> username_value => user
> origin_url => protocol (using the protocol piece of the URL) and host
> (using the host piece)
>
> Those values won't work for creation (meaning we won't be able to create
> valid Chrome entries without user assistance), but at least there's a
> good chance that the user can reuse their Chrome passwords.  Is that
> reasonable?  Do you know of any other software that has its own schema
> like Chrome does?

Once I've heard that Firefox will use it as well. I haven't seen it.

I know that it is used by Nautilus, UbuntuOne, Desktop Couch, Deja Dup
backup, some IRC clients, ...

Open your seahorse client, and have a look on it.

> Ted
>
> p.s. Sorry about the multiple merge and commit messages on this branch.

No problem. That's why we have git :-)

Best regards, Michael.

Re: tzz-auth-source-rewrite branch

[Ted Zlatanov]

On Sun, 06 Feb 2011 21:36:32 +0100 Michael Albinus <michael.albinus@gmx.de> wrote: 

MA> Mid-term, we could improve the returned function to check/add caches
MA> as well.

I thought the cache would only be at the top level of
`auth-source-search', keyed on (format "%S" spec) and nothing else.  Any
creation or deletion on a backend would clear that backend's cache (we'd
know which result lists involve a backend because :type and :source in
the cached result list would match).  Do you mean something else here?

MA> Ted Zlatanov <tzz@lifelogs.com> writes:
>> I think we should support the Google Chrome schema, at least:
>> 
>> username_value => user
>> origin_url => protocol (using the protocol piece of the URL) and host
>> (using the host piece)
>> 
>> Those values won't work for creation (meaning we won't be able to create
>> valid Chrome entries without user assistance), but at least there's a
>> good chance that the user can reuse their Chrome passwords.  Is that
>> reasonable?  Do you know of any other software that has its own schema
>> like Chrome does?

MA> Once I've heard that Firefox will use it as well. I haven't seen it.

MA> I know that it is used by Nautilus, UbuntuOne, Desktop Couch, Deja Dup
MA> backup, some IRC clients, ...

I'm concerned that this will break the idempotent searches (compare to
the netrc backend, where every entry returned will match itself as a
search spec).  To do it like netrc does, we would require a full
retrieve of all the objects, which is prohibitively slow.  So I won't do
it for now, though I'd love to hear suggestions on how to handle this
problem better.

Ted

Re: tzz-auth-source-rewrite branch

[Ted Zlatanov]

On Sun, 06 Feb 2011 09:05:20 -0800 Lars Ingebrigtsen <larsi@gnus.org> wrote: 

LI> Ted Zlatanov <tzz@lifelogs.com> writes:
>> The `auth-source-user-or-password' wrapper tries to create an entry
>> currently, which is not OK.  So it's not ready for use.  When I think
>> it's OK, I'll update the manual and merge the branch back into the
>> master branch.

LI> Sounds good.  Let me know when it's ready to use, and we can start
LI> adapting nnimap and/or nntp to use it.

I put it one last change I did last night
(060d648dcb691ba9f4620e320b1cb8409c060fad) to fix some stupid bugs.

I get this warning while compiling:

In auth-source-search:
auth-source.el:507:33:Warning: `error' called with 2 args to fill 0 format
    field(s)

In auth-source-netrc-search:
auth-source.el:712:32:Warning: `error' called with 2 args to fill 0 format
    field(s)

but am not sure why.

You can just grab auth-source.el from my branch and test with that, or
"git checkout tzz-auth-source-rewrite" if you prefer to test in my
branch.  I think it will work either way.  Merging the ChangeLog has
been a pain (as you can see from the many commit messages).

I tested `auth-source-user-or-password' compatibility and it seemed OK:
I was able to use Gnus with the new auth-source.el, using both
unauthenticated NNTP connections and authenticated NNTP and IMAP
connections.

When you think we're ready, I'll merge my branch back into master and
change all the invocations of `auth-source-user-or-password' in Gnus to
DTRT.  Then when Yamaoka-san merges with Emacs I'll change all the Emacs
libraries to DTRT as well.

Thanks
Ted

Re: tzz-auth-source-rewrite branch

[Ted Zlatanov]

On Mon, 07 Feb 2011 14:47:35 -0600 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> You can just grab auth-source.el from my branch and test with that, or
TZ> "git checkout tzz-auth-source-rewrite" if you prefer to test in my
TZ> branch.  I think it will work either way.  Merging the ChangeLog has
TZ> been a pain (as you can see from the many commit messages).

TZ> I tested `auth-source-user-or-password' compatibility and it seemed OK:
TZ> I was able to use Gnus with the new auth-source.el, using both
TZ> unauthenticated NNTP connections and authenticated NNTP and IMAP
TZ> connections.

TZ> When you think we're ready, I'll merge my branch back into master and
TZ> change all the invocations of `auth-source-user-or-password' in Gnus to
TZ> DTRT.  Then when Yamaoka-san merges with Emacs I'll change all the Emacs
TZ> libraries to DTRT as well.

See mail-source.el, nntp.el, and nnimap.el in my branch for fully
functional (though uncached) implementations using
`auth-source-search'.  I still need to change the
`auth-source-forget-user-or-password' call in nnimap.el but I need the
cache implementation first.

Testing would be most welcome.  Just do
"git checkout tzz-auth-source-rewrite" and you're ready
(do "git checkout master" to go back).

Ted

Re: tzz-auth-source-rewrite branch

[Ted Zlatanov]

Caching is implemented.  I will merge my branch to master tonight, which
will possibly break people's passwords.

Ted

Re: tzz-auth-source-rewrite branch

[Lars Ingebrigtsen]

Ted Zlatanov <tzz@lifelogs.com> writes:

> In auth-source-search:
> auth-source.el:507:33:Warning: `error' called with 2 args to fill 0 format
>     field(s)
>
> In auth-source-netrc-search:
> auth-source.el:712:32:Warning: `error' called with 2 args to fill 0 format
>     field(s)
>
> but am not sure why.

The cl macros are virtually unreadable since they use such weird
old-stylee forms, but I think this is the gist of the assert macro:

(list 'progn
      (list 'or form
            (if string
                (list* 'error string (append sargs args)))))

Rewritten to modern macrology, that would be something like:
                
`(progn
   (or ,form
        (if ,string
            (error string ,sargs ,args))))

And we see that, indeed, `error' is called with two parameters.  I've
now just stuck to %s forms into the strings, and that seems to make the
warning go away, as you'd expect.

Aidan Kehoe has been rewriting cl*.el in XEmacs, and I think somebody
should import all that stuff into Emacs, so you'd actually be able to
read the code.
            
-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: tzz-auth-source-rewrite branch

[Lars Ingebrigtsen]

Ted Zlatanov <tzz@lifelogs.com> writes:

> Caching is implemented.  I will merge my branch to master tonight, which
> will possibly break people's passwords.

Seems to be working for me.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: tzz-auth-source-rewrite branch

[Ted Zlatanov]

On Sun, 13 Feb 2011 19:28:42 -0800 Lars Ingebrigtsen <larsi@gnus.org> wrote: 

LI> Aidan Kehoe has been rewriting cl*.el in XEmacs, and I think somebody
LI> should import all that stuff into Emacs, so you'd actually be able to
LI> read the code.

That would be nice.  And maybe we could actually use cl*.el freely.

Ted

Re: tzz-auth-source-rewrite branch

[Andreas Schwab]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> The cl macros are virtually unreadable since they use such weird
> old-stylee forms, but I think this is the gist of the assert macro:
>
> (list 'progn
>       (list 'or form
>             (if string
>                 (list* 'error string (append sargs args)))))
>
> Rewritten to modern macrology, that would be something like:

`(progn
   (or ,form
       ,(if string
            `(error ,string ,@sargs ,@args))))

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."