Remove not_transient_error_if and add transient_error_if_cmd.

ltratt · ltratt · commit 467439a7a19e · 2023-04-29T08:48:41.000+01:00
I've long felt `not_transient_error_if` was an awkward name, so it's time to
bite the bullet and change it. `transient_error_if_cmd` fits in better
with pizauth's other option names, but does require changing the way the
command runs. Fortunately, in general, simply changing:

```
not_transient_error_if = "shell-cmd";
```

to:

```
transient_error_if_cmd = "! shell-cmd";
```

is all that is required.
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,3 +1,27 @@
+# pizauth 0.3.0 (2023-XX-YY)
+
+## Breaking changes
+
+* `not_transient_error_if` has been removed as a global and per-account option.
+  In its place has been added the global option `transient_error_if_cmd`.
+
+  If you have an existing `not_transient_error_if` option, you will need
+  to reconsider the shell command you execute. One possibility is to change:
+
+```
+not_transient_error_if = "shell-cmd";
+```
+
+to:
+
+```
+transient_error_if_cmd = "! shell-cmd";
+```
+
+  `transient_error_if_cmd` sets the environment variable `$PIZAUTH_ACCOUNT` to
+  allow you to perform different actions for different accounts if you desire.
+
+
 # pizauth 0.2.2 (2023-04-03)
 
 * Added a global `token_event_cmd` option, which runs a command whenever an
diff --git a/pizauth.conf.5 b/pizauth.conf.5
@@ -42,20 +42,20 @@ specifies the address for the
 HTTP server to listen on.
 Defaults to
 .Qq 127.0.0.1:0 .
-.It Sy not_transient_error_if = Qo Em shell-cmd Qc ;
+.It Sy transient_error_if_cmd = Qo Em shell-cmd Qc ;
 specifies a shell command to be run when pizauth repeatedly encounters
 errors when trying to refresh a token.
 One special environment variable is set:
 .Em $PIZAUTH_ACCOUNT
 is set to the account name.
 If
 .Em shell-cmd
-returns a zero exit code, or exceeds a 3 minute timeout, pizauth treats
-the errors as permanent: the access token is invalidated (forcing the user
-to later reauthenicate).
+returns a zero exit code, the transient errors are ignored.
 If
 .Em shell-cmd
-returns a non-zero exit code, the transient errors are ignored.
+returns a non-zero exit code, or exceeds a 3 minute timeout, pizauth treats
+the errors as permanent: the access token is invalidated (forcing the user
+to later reauthenicate).
 Defaults to ignoring non-fatal errors if not specified.
 .It Sy refresh_at_least = Em time ;
 specifies the maximum period of time before an access token will be forcibly
diff --git a/src/config.l b/src/config.l
@@ -21,7 +21,7 @@ client_secret "CLIENT_SECRET"
 error_notify_cmd "ERROR_NOTIFY_CMD"
 http_listen "HTTP_LISTEN"
 login_hint "LOGIN_HINT"
-not_transient_error_if "NOT_TRANSIENT_ERROR_IF"
+transient_error_if_cmd "TRANSIENT_ERROR_IF_CMD"
 refresh_retry "REFRESH_RETRY"
 redirect_uri "REDIRECT_URI"
 refresh_before_expiry "REFRESH_BEFORE_EXPIRY"
diff --git a/src/config.rs b/src/config.rs
@@ -34,7 +34,7 @@ pub struct Config {
     pub auth_notify_interval: Duration,
     pub error_notify_cmd: Option<String>,
     pub http_listen: String,
-    pub not_transient_error_if: Option<String>,
+    pub transient_error_if_cmd: Option<String>,
     refresh_at_least: Option<Duration>,
     refresh_before_expiry: Option<Duration>,
     refresh_retry: Option<Duration>,
@@ -69,7 +69,7 @@ impl Config {
         let mut auth_notify_interval = None;
         let mut error_notify_cmd = None;
         let mut http_listen = None;
-        let mut not_transient_error_if = None;
+        let mut transient_error_if_cmd = None;
         let mut refresh_at_least = None;
         let mut refresh_before_expiry = None;
         let mut refresh_retry = None;
@@ -130,12 +130,12 @@ impl Config {
                                 http_listen,
                             )?)
                         }
-                        config_ast::TopLevel::NotTransientErrorIf(span) => {
-                            not_transient_error_if = Some(check_not_assigned_str(
+                        config_ast::TopLevel::TransientErrorIfCmd(span) => {
+                            transient_error_if_cmd = Some(check_not_assigned_str(
                                 &lexer,
-                                "not_transient_error_if",
+                                "transient_error_if_cmd",
                                 span,
-                                not_transient_error_if,
+                                transient_error_if_cmd,
                             )?)
                         }
                         config_ast::TopLevel::RefreshAtLeast(span) => {
@@ -188,7 +188,7 @@ impl Config {
                 .unwrap_or_else(|| Duration::from_secs(AUTH_NOTIFY_INTERVAL_DEFAULT)),
             error_notify_cmd,
             http_listen: http_listen.unwrap_or_else(|| HTTP_LISTEN_DEFAULT.to_owned()),
-            not_transient_error_if,
+            transient_error_if_cmd,
             refresh_at_least,
             refresh_before_expiry,
             refresh_retry,
@@ -637,7 +637,7 @@ mod test {
             auth_notify_interval = 88m;
             error_notify_cmd = "j";
             http_listen = "127.0.0.1:56789";
-            not_transient_error_if = "k";
+            transient_error_if_cmd = "k";
             token_event_cmd = "q";
             account "x" {
                 // Mandatory fields
@@ -661,7 +661,7 @@ mod test {
         assert_eq!(c.auth_notify_cmd, Some("g".to_owned()));
         assert_eq!(c.auth_notify_interval, Duration::from_secs(88 * 60));
         assert_eq!(c.http_listen, "127.0.0.1:56789".to_owned());
-        assert_eq!(c.not_transient_error_if, Some("k".to_owned()));
+        assert_eq!(c.transient_error_if_cmd, Some("k".to_owned()));
         assert_eq!(c.token_event_cmd, Some("q".to_owned()));
 
         let act = &c.accounts["x"];
@@ -718,8 +718,8 @@ mod test {
             Err(s) if s.contains("Mustn't specify 'token_event_cmd' more than once") => (),
             _ => panic!(),
         }
-        match Config::from_str(r#"not_transient_error_if = "a"; not_transient_error_if = "b";"#) {
-            Err(s) if s.contains("Mustn't specify 'not_transient_error_if' more than once") => (),
+        match Config::from_str(r#"transient_error_if_cmd = "a"; transient_error_if_cmd = "b";"#) {
+            Err(s) if s.contains("Mustn't specify 'transient_error_if_cmd' more than once") => (),
             _ => panic!(),
         }
         match Config::from_str(r#"http_listen = "a"; http_listen = "b";"#) {
@@ -816,7 +816,7 @@ mod test {
         "#,
         )
         .unwrap();
-        assert_eq!(c.not_transient_error_if, None);
+        assert_eq!(c.transient_error_if_cmd, None);
         assert_eq!(c.refresh_at_least, None);
         assert_eq!(c.refresh_before_expiry, None);
         assert_eq!(c.refresh_retry, None);
@@ -829,7 +829,7 @@ mod test {
         // Global only
         let c = Config::from_str(
             r#"
-            not_transient_error_if = "e";
+            transient_error_if_cmd = "e";
             refresh_at_least = 1s;
             refresh_before_expiry = 2s;
             refresh_retry = 3s;
@@ -842,7 +842,7 @@ mod test {
         "#,
         )
         .unwrap();
-        assert_eq!(c.not_transient_error_if, Some("e".to_owned()));
+        assert_eq!(c.transient_error_if_cmd, Some("e".to_owned()));
         assert_eq!(c.refresh_at_least, Some(Duration::from_secs(1)));
         assert_eq!(c.refresh_before_expiry, Some(Duration::from_secs(2)));
         assert_eq!(c.refresh_retry, Some(Duration::from_secs(3)));
@@ -868,7 +868,7 @@ mod test {
         )
         .unwrap();
 
-        assert_eq!(c.not_transient_error_if, None);
+        assert_eq!(c.transient_error_if_cmd, None);
         assert_eq!(c.refresh_at_least, None);
         assert_eq!(c.refresh_before_expiry, None);
         assert_eq!(c.refresh_retry, None);
@@ -881,7 +881,7 @@ mod test {
         // Local overrides global
         let c = Config::from_str(
             r#"
-            not_transient_error_if = "e";
+            transient_error_if_cmd = "e";
             refresh_at_least = 1s;
             refresh_before_expiry = 2s;
             refresh_retry = 3s;
@@ -897,7 +897,7 @@ mod test {
         "#,
         )
         .unwrap();
-        assert_eq!(c.not_transient_error_if, Some("e".to_owned()));
+        assert_eq!(c.transient_error_if_cmd, Some("e".to_owned()));
         assert_eq!(c.refresh_at_least, Some(Duration::from_secs(1)));
         assert_eq!(c.refresh_before_expiry, Some(Duration::from_secs(2)));
         assert_eq!(c.refresh_retry, Some(Duration::from_secs(3)));
@@ -910,7 +910,7 @@ mod test {
         // Local overrides global
         let c = Config::from_str(
             r#"
-            not_transient_error_if = "e";
+            transient_error_if_cmd = "e";
             refresh_at_least = 1s;
             refresh_before_expiry = 2s;
             refresh_retry = 3s;
@@ -941,7 +941,7 @@ mod test {
         "#,
         )
         .unwrap();
-        assert_eq!(c.not_transient_error_if, Some("e".to_owned()));
+        assert_eq!(c.transient_error_if_cmd, Some("e".to_owned()));
         assert_eq!(c.refresh_at_least, Some(Duration::from_secs(1)));
         assert_eq!(c.refresh_before_expiry, Some(Duration::from_secs(2)));
         assert_eq!(c.refresh_retry, Some(Duration::from_secs(3)));
diff --git a/src/config.y b/src/config.y
@@ -17,7 +17,7 @@ TopLevel -> Result<TopLevel, ()>:
   | "AUTH_NOTIFY_INTERVAL" "=" "TIME" ";" { Ok(TopLevel::AuthNotifyInterval(map_err($3)?)) }
   | "ERROR_NOTIFY_CMD" "=" "STRING" ";" { Ok(TopLevel::ErrorNotifyCmd(map_err($3)?)) }
   | "HTTP_LISTEN" "=" "STRING" ";" { Ok(TopLevel::HttpListen(map_err($3)?)) }
-  | "NOT_TRANSIENT_ERROR_IF" "=" "STRING" ";" { Ok(TopLevel::NotTransientErrorIf(map_err($3)?)) }
+  | "TRANSIENT_ERROR_IF_CMD" "=" "STRING" ";" { Ok(TopLevel::TransientErrorIfCmd(map_err($3)?)) }
   | "REFRESH_AT_LEAST" "=" "TIME" ";" { Ok(TopLevel::RefreshAtLeast(map_err($3)?)) }
   | "REFRESH_BEFORE_EXPIRY" "=" "TIME" ";" { Ok(TopLevel::RefreshBeforeExpiry(map_err($3)?)) }
   | "REFRESH_RETRY" "=" "TIME" ";" { Ok(TopLevel::RefreshRetry(map_err($3)?)) }
diff --git a/src/config_ast.rs b/src/config_ast.rs
@@ -7,7 +7,7 @@ pub enum TopLevel {
     AuthNotifyInterval(Span),
     ErrorNotifyCmd(Span),
     HttpListen(Span),
-    NotTransientErrorIf(Span),
+    TransientErrorIfCmd(Span),
     RefreshAtLeast(Span),
     RefreshBeforeExpiry(Span),
     RefreshRetry(Span),
diff --git a/src/server/refresher.rs b/src/server/refresher.rs
@@ -22,8 +22,8 @@ use super::{
 
 /// How many times can a transient error be encountered before we try `not_transient_error_if`?
 const TRANSIENT_ERROR_RETRIES: u64 = 6;
-/// How long to run `not_transient_error_if` commands before killing them?
-const NOT_TRANSIENT_ERROR_IF_TIMEOUT: Duration = Duration::from_secs(3 * 60);
+/// How long to run `transient_error_if_cmd` commands before killing them?
+const TRANSIENT_ERROR_IF_CMD_TIMEOUT: Duration = Duration::from_secs(3 * 60);
 
 /// The outcome of an attempted refresh.
 #[derive(Debug)]
@@ -100,7 +100,7 @@ impl Refresher {
                                             == 0
                                         {
                                             if let Some(ref cmd) =
-                                                ct_lk.config().not_transient_error_if
+                                                ct_lk.config().transient_error_if_cmd
                                             {
                                                 let cmd = cmd.to_owned();
                                                 drop(ct_lk);
@@ -173,17 +173,17 @@ impl Refresher {
                 .args(["-c", &cmd])
                 .spawn()
             {
-                Ok(mut child) => match child.wait_timeout(NOT_TRANSIENT_ERROR_IF_TIMEOUT) {
+                Ok(mut child) => match child.wait_timeout(TRANSIENT_ERROR_IF_CMD_TIMEOUT) {
                     Ok(Some(status)) => {
-                        if !status.success() {
+                        if status.success() {
                             Ok(())
                         } else {
                             Err(format!(
                                 "'{cmd:}' returned {}",
                                 status
                                     .code()
                                     .map(|x| x.to_string())
-                                    .unwrap_or_else(|| "<Unknown exit code".to_string())
+                                    .unwrap_or_else(|| "<Unknown exit code>".to_string())
                             ))
                         }
                     }
