ltratt / pizauth

Command-line OAuth2 authentication daemon
https://tratt.net/laurie/src/pizauth/
Other
164 stars 5 forks source link

Implement `pizauth revoke`. #30

Closed ltratt closed 10 months ago

ltratt commented 10 months ago

Currently this is local-only as OAuth2 provides no standard way of revoking remote tokens (though some providers do provide ad-hoc extensions to do so).

Fixes https://github.com/ltratt/pizauth/issues/29. @hseg Please see if this works for you (I have lightly, but not yet extensively, tested it).

hseg commented 10 months ago

Will check tomorrow, thanks!

ltratt commented 10 months ago

@hseg Any luck with this one?

hseg commented 10 months ago

On Sun, Jan 07, 2024 at 12:33:04AM -0800, Laurence Tratt wrote:

@hseg Any luck with this one?

Just tested, rebases cleanly and seems to be working -- cannot distinguish difficulties from general neomutt jankiness, but the results LGTM.

hseg commented 10 months ago

You do want to add this to the bash completion, and I would've appreciated the ping on testing this coming after a rebase onto master (building a package on an off-master branch is supported in my build system, playing around with the git history isn't (so I need to hope an automated rebase goes through)), but those are only tangentially on-topic quibbles.

hseg commented 10 months ago

On Sun, Jan 07, 2024 at 12:33:04AM -0800, Laurence Tratt wrote:

@hseg Any luck with this one?

So testing ended up with me authenticating with a stale link, which pizauth notified me of. I then revoked and restarted the authentication flow, which worked. Am more confident that it works now.

ltratt commented 10 months ago

If you have a suggested patch for the bash completion, I'd very happily incorporate it!

hseg commented 10 months ago

On Sun, Jan 07, 2024 at 10:06:15AM -0800, Laurence Tratt wrote:

If you have a suggested patch for the bash completion, I'd very happily incorporate it!

You just need to copy the $COMP_CWORD=3 case for refresh|show into the $COMP_CWORD=2 case for revoke, skipping the check for -u:

diff --git a/share/bash/completion.bash b/share/bash/completion.bash
index 70a1fc7..30e3be6 100644
--- a/share/bash/completion.bash
+++ b/share/bash/completion.bash
@@ -40,6 +40,14 @@ _pizauth()
                     mapfile -t COMPREPLY < \
                         <(compgen -W "${accounts[*]}" -- "$cur")
                     ;;
+                revoke)
+                        local accounts
+                        mapfile -t accounts < <(_accounts)
+                        mapfile -t COMPREPLY < \
+                            <(compgen -W "${accounts[*]}" -- "$cur")
+                        ;;
+                    *) COMPREPLY=()
+                    ;;
             esac
             ;;
         3)

BTW, looking at the README in https://github.com/scop/bash-completion, I see they ask for completions to be installed at pkg-config --variable completionsdir bash-completion, and offer the following autoconfig snippets for it:

PKG_CHECK_VAR(bashcompdir, [bash-completion], [completionsdir], ,
 bashcompdir="${sysconfdir}/bash_completion.d")
AC_SUBST(bashcompdir)
bashcompdir = @bashcompdir@
dist_bashcomp_DATA = your-completion-file # completion files go here

Though in practice I see many packages hardcoding /usr/share/bash-completion/.

ltratt commented 10 months ago

@hseg I had to slightly tweak the whitespace, so please check that c8e5ff2 still does what you hope.

pkg-config --variable completionsdir bash-completion

Using this should be easy, though interestingly on the Linux box I have access to it returns /usr/share/bash-completion/completions rather than /usr/share/bash-completion/ ?

hseg commented 10 months ago

Ah, you're right -- misread the installation scripts I was looking at.

The directive doesn't seem to be universally followed, though -- indeed, on my machine, only 5/64 packages installing completions actually bother checking with pkg-config: gstreamer, libxkbcommon, p11-kit, systemd, tracker3, and util-linux. And some packages completely ignore the /usr/share/ default and instead install to /etc/bash_completion.d/, eg youtube-dl (though for such packages, the PKGBUILD files redirect the installation explicitly to /usr/share/bash-completion/completions

So it's up to you if you want to do things "properly" -- I can always just move the completions to the right place in the PKGBUILD

ltratt commented 10 months ago

I'm going to merge this PR, and investigate the pkg-config aspect in another PR.

hseg commented 10 months ago

On Tue, Jan 09, 2024 at 06:20:39AM -0800, Laurence Tratt wrote:

I'm going to merge this PR, and investigate the pkg-config aspect in another PR.

Just noticed in testing that I missed one place needing edits -- the line local cmds=(...) in share/bash/completion.bash should have revoke added to it. As it stands now, pizauth revoke <TAB> completes available accounts, but pizauth re<TAB> doesn't mention revoke.

Checking for alternative solutions to avoid this duplication in the future, I notice some of the programs I use use https://hackage.haskell.org/package/optparse-applicative. The logic that library uses is to have a hidden set of flags (--bash-completion-index, --bash-completion-word) to pass the argument parsing code the current commandline, at which point it responds with replacements/completions to offer at the current index.

Looking around a little at the rust ecosystem, apparently there's several libraries in this space -- one that seems to be cropping up frequently is https://github.com/clap-rs/clap -- perhaps that might help?

Also, bash-completion offers the functions _comp_compgen_help and _comp_compgen_usage, which are basically docopt implementations -- they parse usage messages to construct the completions.

ltratt commented 10 months ago

Might you be able to raise a PR with the necessary changes?

hseg commented 10 months ago

I'm going to merge this PR, and investigate the pkg-config aspect in another PR.

OK, in the meantime I pushed a PKGBUILD to AUR moving the bash completion to /usr/share/bash-completion/completions/ where it's expected