Possible bug in configurable failover

[Brian Candler]

> > As always, patches are welcome for suggested behavior.

For the delayed-return-when-you-don't-expect-it case:

    authenticate {
        ... previous module sets reject without returning (reject=1)
        if (cond) {
            update request {
               ... ok so far
            }
            !! return happens here !!
        }
    }

It seems to me that if the reject suppressed the return previously, then the
'if' block should suppress it too.  In fact, an 'if' block could behave like
an 'update' and simply perform no action at its end.  I've made a patch for
if/elsif/else/case/switch:

https://github.com/candlerb/freeradius-server/commit/5d4b9923010de8f24dff266133643156603385e0

This makes the authenticate { } section behave how I expected it to in the
first place.  You can still turn on the old behaviour if you want:

    if (cond) {
        ...
        reject = return
    }

I arrived at that patch after examining why the 'update' block has a zero
actions array.  This turns out to be the right thing: if you *did* have the
standard actions for 'update', and a 'reject' was set by a previous module,
then a subsequent update{...} statement will trigger an immediately return. 
That's not very useful.

Furthermore, the syntax for 'update' doesn't allow overriding return codes
like this:

    update reply {
        reject = 1
        Reply-Message += "foo"
    }

Hence it's important that update shouldn't take any action based on the
return code from the previous module.

What about update setting the result to 'updated', instead of propagating
the result of the previous module?  I tried that, and it was OK in simple
cases, but is probably undesirable in 'redundant' groups.  After all, you
don't want a simple update { } to mark the request as if it had done a
successful database lookup.

However, I still haven't worked out why the default AUTZ actions are being
used for AUTH modules in some circumstances. The following patch would undo
it, but it's probably a bad idea (I just don't understand why):

--- a/src/main/modcall.c
+++ b/src/main/modcall.c
@@ -1950,13 +1938,8 @@ static modcallable *do_compile_modsingle(modcallable *parent,
 	csingle = mod_singletocallable(single);
 	csingle->parent = parent;
 	csingle->next = NULL;
-	if (!parent || (component != RLM_COMPONENT_AUTH)) {
-		memcpy(csingle->actions, defaultactions[component][grouptype],
-		       sizeof csingle->actions);
-	} else { /* inside Auth-Type has different rules */
-		memcpy(csingle->actions, defaultactions[RLM_COMPONENT_AUTZ][grouptype],
-		       sizeof csingle->actions);
-	}
+        memcpy(csingle->actions, defaultactions[component][grouptype],
+               sizeof csingle->actions);
 	rad_assert(modrefname != NULL);
 	csingle->name = modrefname;
 	csingle->type = MOD_SINGLE;
@@ -2132,11 +2115,7 @@ static modcallable *do_compile_modgroup(modcallable *parent,
 	 */
 	for (i = 0; i < RLM_MODULE_NUMCODES; i++) {
 		if (!c->actions[i]) {
-			if (!parent || (component != RLM_COMPONENT_AUTH)) {
-				c->actions[i] = defaultactions[component][parentgrouptype][i];
-			} else { /* inside Auth-Type has different rules */
-				c->actions[i] = defaultactions[RLM_COMPONENT_AUTZ][parentgrouptype][i];
-			}
+                        c->actions[i] = defaultactions[component][parentgrouptype][i];
 		}
 	}
 
Redundant and append groups are identical between AUTH and AUTZ anyway, so
the only difference is in the behaviour of a simple group, depending on
whether it's at the top level or not (!parent)

Regards,

Brian.