Package: elpa;
Reported by: Stefan Kangas <stefankangas <at> gmail.com>
Date: Sat, 10 Feb 2024 11:17:01 UTC
Severity: normal
View this message in rfc822 format
From: Philip Kaludercic <philipk <at> posteo.net> To: Stefan Kangas <stefankangas <at> gmail.com> Cc: 69013 <at> debbugs.gnu.org, Michael Albinus <michael.albinus <at> gmx.de>, Vivek Das Mohapatra <vivek <at> etla.org> Subject: bug#69013: New package for NonGNU ELPA: totp-auth Date: Sat, 10 Feb 2024 18:20:16 +0000
Stefan Kangas <stefankangas <at> gmail.com> writes:
> Package: elpa
>
> See also the corresponding discussion on emacs-devel:
> https://lists.gnu.org/r/emacs-devel/2024-02/msg00206.html
>
> -------------------- Start of forwarded message --------------------
> Date: Mon, 5 Feb 2024 15:35:40 +0000
> To: emacs-devel <at> gnu.org
> From: Vivek Das Mohapatra <vivek <at> etla.org>
> Subject: New package for NonGNU ELPA : totp-auth
I'll repost my message that might or might not still be encrypted here
again:
--8<---------------cut here---------------start------------->8---
Vivek Das Mohapatra <vivek <at> etla.org> writes:
> Hi - I've recently made a package that implements RFC6238 TOTP and was
> wondering if nongnu elpa would consider carrying it:
>
> totp-auth.el - Time-based One Time Password support for emacs
Sounds useful, would certainly be a good addition.
> This package generates RFC6238 Time-based One Time Passwords
> (in other words, what Google Authenticator implements)
> and displays them (as well as optionally copying them to
> the clipboard/primary selection), updating them as they expire.
>
> It retrieves the shared secrets used to generate TOTP tokens
> with ‘auth-sources’ and/or the freedesktop secrets API (aka
> Gnome Keyring or KWallet).
>
> You can call it with the command ‘totp-auth’, ie:
>
> M-x totp-auth RET
>
> You can tab-complete based on the label of the secret.
> Depending on the setting of ‘totp-auth-display-token-method’ the
> TOTP token will be displayed (and kept up to date) either in
> an emacs buffer or a freedesktop notification.
>
> From dd15c0496cd530fb1bd18e7a79a827e76bb29a57 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Vivek=20Das=C2=A0Mohapatra?= <vivek <at> collabora.com>
> Date: Mon, 5 Feb 2024 15:19:34 +0000
> Subject: [PATCH] Add the totp-auth package and its dependency (base32)
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> This package generates RFC6238 Time-based One Time Passwords
> (in other words, what Google Authenticator implements)
> and displays them (as well as optionally copying them to
> the clipboard/primary selection), updating them as they expire.
>
> It retrieves the shared secrets used to generate TOTP tokens
> with ‘auth-sources’ and/or the freedesktop secrets API (aka
> Gnome Keyring or KWallet).
> ---
> elpa-packages | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/elpa-packages b/elpa-packages
> index 1f9a16311c..8ed3955547 100644
> --- a/elpa-packages
> +++ b/elpa-packages
> @@ -49,6 +49,10 @@
> (autothemer :url "https://github.com/jasonm23/autothemer.git"
> :readme "README.md")
>
> + (base32 :url "https://gitlab.com/fledermaus/totp.el"
> + :ignored-files ("totp-auth*.el" "Makefile" "*.md" "*.html" "tests" "README")
> + :version-map (("0.2" "1.0" "v1.0")))
IIUC you are developing two packages in the same repository, right?
While possible, it would be preferable if these wouldn't overlap,
e.g. by maintaining the code in separate branches.
> +
> (bash-completion :url "https://github.com/szermatt/emacs-bash-completion"
> :readme "README.md")
>
> @@ -760,6 +764,10 @@
> (toc-org :url "https://github.com/snosov1/toc-org.git"
> :ignored-files ("COPYING" ".travis.yml" "toc-org-test.el"))
>
> + (totp-auth :url "https://gitlab.com/fledermaus/totp.el"
> + :ignored-files ("base32.el" "Makefile" "*.md" "*.html" "tests")
You should be able to add an .elpaignore file to your repository to
ignore some of the files you don't want in both packages like "Makefile"
"*.md" "*.html" and "tests".
> + :version-map (("0.4" "1.0" "v1.0")))
> +
> (treeview :url "https://github.com/tilmanrassy/emacs-treeview"
> :readme "README.md"
> :ignored-files ("LICENSE"))
Here are a few comments from skimming over the code:
diff --git a/base32.el b/base32.el
index 1e4304b..09df366 100644
--- a/base32.el
+++ b/base32.el
@@ -1,5 +1,7 @@
;;; base32.el --- Base32 support -*- mode: emacs-lisp; lexical-binding: t; -*-
+
;; Copyright © 2022,2023 Vivek Das Moapatra <vivek <at> etla.org>
+
;; SPDX-License-Identifier: GPL-3.0-or-later
;; Author: Vivek Das Mohapatra <vivek <at> etla.org>
@@ -9,9 +11,11 @@
;; Package-Requires: ((emacs "27.1"))
;;; Commentary:
+
;; A base32 encoder/decoder.
;;; Code:
+
(defconst base32-dictionary
[?A ?B ?C ?D ?E ?F ?G ?H ?I ?J ?K ?L ?M ?N ?O ?P
?Q ?R ?S ?T ?U ?V ?W ?X ?Y ?Z ?2 ?3 ?4 ?5 ?6 ?7 ?=]
@@ -32,6 +36,10 @@ Shift rightwards if C is negative.
Any bits shifted in are 0.
Suppress opinionated (and in our case wrong) warning about ’lsh’."
(with-suppressed-warnings ((suspicious lsh))
+ ;; One would usually use `ash', adding a comment as to why you are
+ ;; using `lsh' and ignoring the warning would be helpful.
+ ;; Especially since this seems to be the only macro that raises
+ ;; the minimum version of your code to 27.1.
(lsh v c)))
(defun base32-thesaurus (&optional dictionary)
@@ -114,9 +122,9 @@ Dictionary should match ‘base32-dictionary’ in format."
"Encode INPUT bytes according to DICTIONARY.
DICTIONARY defaults to ‘base32-dictionary’."
(or dictionary (setq dictionary base32-dictionary))
- (let (input-byte-count input-shortfall output-padding output output-length)
- (setq input-byte-count (length input)
- input-shortfall (mod input-byte-count 5))
+ (let* ((input-byte-count (length input))
+ (input-shortfall (mod input-byte-count 5))
+ output-padding output output-length)
(when (> input-shortfall 0)
(setq input-shortfall (- 5 input-shortfall))
(setq input (concat input (make-string input-shortfall 0))
@@ -125,40 +133,38 @@ DICTIONARY defaults to ‘base32-dictionary’."
((eq input-shortfall 2) 3)
((eq input-shortfall 1) 1))))
(setq output-length (/ (* (length input) 8) 5)
- output (make-vector output-length 0))
+ output (make-string output-length 0))
(dotimes (i output-length)
(aset output i (aref dictionary (base32--nth-5bit-get input i))))
(when output-padding
(let ((padc (or (aref dictionary 32) ?=)))
(dotimes (i output-padding)
(aset output (- output-length i 1) padc))))
- (concat output)))
+ output))
(defun base32-decode (input &optional dictionary)
"Decode INPUT bytes according to DICTIONARY.
DICTIONARY defaults to ‘base32-dictionary’."
- (let ((thesaurus
+ (let* ((thesaurus
(base32-thesaurus (or dictionary base32-dictionary)))
- input-byte-count input-shortfall
- output output-byte-count output-shorten chunk)
- (setq input-byte-count (length input)
- input-shortfall (mod input-byte-count 8)
- output-shorten 0)
+ (input-byte-count input-byte-count (length input))
+ (input-shortfall (mod input-byte-count 8))
+ (output-shorten 0)
+ output output-byte-count chunk)
(cond ((memq input-shortfall '(6 4 3 1)) ;; needs padding?
(setq input (concat input (make-string input-shortfall ?=))
input-byte-count (+ input-shortfall input-byte-count)
- output-shorten (cond ((eq -6 input-shortfall) -4)
+ output-shorten (cond ((eq -6 input-shortfall) -4) ;why `eq'?
((eq -4 input-shortfall) -3)
((eq -3 input-shortfall) -2)
((eq -1 input-shortfall) -1)
(t 0))))
((eq 0 input-shortfall) ;; already padded
(setq output-shorten
- (cond ((equal (substring input -6) "======") -4)
- ((equal (substring input -4) "====") -3)
- ((equal (substring input -3) "===") -2)
- ((equal (substring input -1) "=") -1)
- (t 0))))
+ ;; this seems to work
+ (if (string-match "=\\{1,6\\}\\'" input)
+ (- (1+ (/ (- (match-end 0) (match-beginning 0)) 2)))
+ 0)))
(t (error "Invalid base32 payload length: %d" input-byte-count)))
(setq output-byte-count (* (/ input-byte-count 8) 5)
output (make-string output-byte-count 0))
diff --git a/totp-auth.el b/totp-auth.el
index 4d42a21..b7cdaec 100644
--- a/totp-auth.el
+++ b/totp-auth.el
@@ -1,16 +1,16 @@
;;; totp-auth.el --- RFC6238 TOTP -*- mode: emacs-lisp; lexical-binding: t; -*-
+
;; Copyright © 2022,2023 Vivek Das Mohapatra <vivek <at> etla.org>
-;; SPDX-License-Identifier: GPL-3.0-or-later
;; Author: Vivek Das Mohapatra <vivek <at> etla.org>
;; Keywords: 2FA two-factor totp otp password comm
+;; SPDX-License-Identifier: GPL-3.0-or-later
;; URL: https://gitlab.com/fledermaus/totp.el
;; Version: 0.4
;; Package-Requires: ((emacs "27.1") (base32 "0.1"))
;;; Commentary:
-;; totp-auth.el - Time-based One Time Password support for Emacs
-;;
+
;; This package generates RFC6238 Time-based One Time Passwords
;; (in other words, what Google Authenticator implements)
;; and displays them (as well as optionally copying them to
@@ -38,8 +38,9 @@
;; can customize ‘totp-auth-auto-copy-password’.
;;; Code:
+
(eval-and-compile
- (let ((load-path load-path)
+ (let ((load-path load-path) ;are you sure you want this?
(this-file (or load-file-name
byte-compile-current-file
buffer-file-name)))
@@ -58,17 +59,18 @@
(require 'mailcap))
(defgroup totp-auth nil "Time-based One Time Passwords."
- :prefix "totp"
+ :prefix "totp-"
:group 'data)
(defconst totp-auth-xdg-schema "org.freedesktop.Secret.TOTP")
(defcustom totp-auth-alt-xdg-schemas
+ ;; perhaps some context here would be nice to add.
'("com.github.bilelmoussaoui.Authenticator")
"A list of fallback XDG schemas which are associated with TOTP secrets.
This is used only to read TOTP secrets stored by other applications."
:type '(repeat string)
- :group 'totp-auth)
+ :group 'totp-auth) ;you can drop :groups in this file
(defcustom totp-auth-minimum-ui-grace 3
"The minimum time to expiry a TOTP must have for interactive use.
@@ -77,8 +79,8 @@ interactive code MAY instead generate the next TOTP in sequence
and wait until it is valid before giving it to the user.
Noninteractive TOTP code MUST return TOTP values along with their
lifespan (at the time of generation) and their absolute expiry time."
- :type 'integer
- :group 'totp)
+ :type 'integer ;or antonym?
+ :group 'totp) ;is this the right group?
(defcustom totp-auth-max-tokens 1024
"The maximum number of tokens totp will try to fetch and process."
@@ -92,6 +94,7 @@ lifespan (at the time of generation) and their absolute expiry time."
:type '(repeat string))
(defcustom totp-auth-file-export-command
+ ;;why not use symbols for the placeholders?
'("qrencode" "-l" "M" "@type@" "-o" "@file@")
"The command and parameters used to convert a data stream to a QR code.
@file@ is a placeholder for the target filename.
@@ -161,7 +164,7 @@ If unset (the default) this will be initialised to a list
consisting of the contents of ‘auth-sources’ with the freedesktop
secrets service login session prepended to it, if it is available."
:group 'totp-auth
- :type `(repeat :tag "Authentication Sources"
+ :type `(repeat :tag "Authentication Sources" ;consider storing the type in a separate variable/constant
(choice
(const :tag "TOTP Secrets Collection" "secrets:TOTP")
(const :tag "Default Secrets Collection" default)
@@ -261,7 +264,7 @@ authenticators."
(allowed (cons ?@ url-unreserved-chars)))
(or (memq digits '(6 8 10))
(setq digits 6))
- (if (> (length user) 0)
+ (if (not (null user))
(format "otpauth://totp/%s%%3A%s?secret=%s;digits=%d"
(url-hexify-string service allowed)
(url-hexify-string user allowed)
@@ -321,20 +324,18 @@ Each entry is an alist of the form:
((:source . function ‘auth-source-backend’ object)
(:handler . :secrets for a desktop secrets API or :default)
(:encrypted . t if the backend is nontrivially encrypted, nil otherwise))"
- (delq nil
- (mapcar (lambda (s &optional secure type source handler)
- (setq source (slot-value s 'source)
- type (slot-value s 'type)
- secure (or (eq type 'secrets)
- (eq type 'plist)
- (equal (file-name-extension source) "gpg"))
- handler (if (eq type 'secrets) :secrets :default))
- (if (and encrypted (not secure))
- nil
- (list (cons :source s)
- (cons :handler handler)
- (cons :encrypted secure))))
- (mapcar #'auth-source-backend-parse (totp-auth-sources)))))
+ (mapcan
+ (lambda (s &optional secure type source handler)
+ (setq source (slot-value s 'source)
+ type (slot-value s 'type)
+ secure (or (eq type 'secrets)
+ (eq type 'plist)
+ (equal (file-name-extension source) "gpg"))
+ handler (if (eq type 'secrets) :secrets :default))
+ (if (and encrypted (not secure))
+ nil
+ `(((:source ,s) (:handler ,handler) (:encrypted ,secure)))))
+ (mapcar #'auth-source-backend-parse (totp-auth-sources))))
(defun totp-auth-get-secrets-from-secrets-source (source)
"Return an alist of secrets from SOURCE (a desktop secrets API auth-source).
@@ -380,8 +381,7 @@ TOTP secret."
(defun totp-auth-get-secrets-from-backend (backend)
"Fetch secrets from a specific auth-source BACKEND."
(when (cdr (assq :encrypted backend))
- (let (source)
- (setq source (cdr (assq :source backend)))
+ (let ((source (cdr (assq :source backend))))
(cond ((eq (cdr (assq :handler backend)) :secrets)
(totp-auth-get-secrets-from-secrets-source source))
(t
@@ -389,7 +389,7 @@ TOTP secret."
(defun totp-auth-same-secret (a b)
"Test whether secrets A and B are the same.
-\nNOTE: This is not a strict test of equality - rather we are checking to
+NOTE: This is not a strict test of equality - rather we are checking to
see if the user and service components of the secret identifier are the
same, ie probably intended for the same target."
(and (equal (assq :service a) (assq :service b))
@@ -404,7 +404,7 @@ same, ie probably intended for the same target."
(setq vault (car backends)
secrets (totp-auth-get-secrets-from-backend vault)
backends (cdr backends))
- (if (cl-member s secrets :test 'totp-auth-same-secret)
+ (if (cl-member s secrets :test #'totp-auth-same-secret)
(setq target vault)))
(or target default)))
@@ -525,10 +525,7 @@ that labels do not have to be unique to a single secret).
Any other value is used for a simple substring match with the label.
\nEach Item of the returned list is a cons of a label and
a structure conforming to ‘totp-auth-unwrap-otp-blob’."
- (let (secrets)
- (setq secrets
- (apply #'nconc
- (mapcar #'totp-auth-get-secrets-from-backend
+ (let ((secrets (mapcon #'totp-auth-get-secrets-from-backend
(totp-auth-storage-backends))))
;; If we were given a filter, apply it:
(if (and (stringp match) (not (zerop (length match))))
@@ -722,7 +719,7 @@ Usually called from a timer set by ‘totp-auth-display-token-notification’."
(setq otp (totp-auth-generate-otp secret)
ttl (nth 1 otp)
text (if (>= totp-auth-minimum-ui-grace ttl)
- "Generating… [⌛]"
+ "Generating… [⌛]" ;please avoid using emojis
(format "%s [%02ds]" (nth 0 otp) ttl)))
(notifications-notify
:title label
@@ -811,6 +808,7 @@ DIGITS defaults to 6 if not otherwise specified."
(list (cdr (assoc key secrets)) key)))
(totp-auth-display-token secret label))
+;; why manually write out these autoloads? all files will get scraped.
(autoload 'totp-auth-import-file "totp-auth-interop"
"Import an RFC6238 TOTP secret or secrets from FILE.
FILE is processed by ‘totp-load-file’ and each secret extracted
I have to admit that your
(let (foo)
(setq foo bar)
...)
pattern confuses me, it seems intentional but unnecessary, so I didn't
comment on every instance.
--8<---------------cut here---------------end--------------->8---
--
Philip Kaludercic
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.