Possible bug in configurable failover

[Brian Candler]

Pasted below is the output from 'dump_tree'. By the way, you need to change
RDEBUG to DEBUG inside dump_mc() and dump_tree() for it to compile
successfully: patch at
https://github.com/candlerb/freeradius-server/commit/3a90c3e2721e3e3794df69874496e129d2db21c0

Observations:

* For 'if' and 'ok', the action codes I the ones I would expect for
  authorize, not authenticate (e.g. noop=2, ok=3, updated=4).

This appears to be an explicit decision:

        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];
                        }
                }
        }

But I'm not sure why it's choosing the defaultactions for AUTZ when inside
an AUTH group, rather than just setting the right values in
defaultactions[RLM_COMPONENT_AUTH].  I see the same logic is in both
do_compile_modsingle and do_compile_modgroup.

This doesn't seem to be mentioned in doc/configurable_failover.rst
("For authenticate, the default is to return on success *or* reject, and
only try the second and following items if the first one fails")

* For 'update' the action codes are all zero, which means no return and no
  update to the current result. Perhaps that's intentional, but it means
  that results like RLM_MODULE_INVALID or RLM_MODULE_FAIL from an update
  are likely to be silently ignored.

* I see the parser has reject=return both inside policy { } and if { },
  which is consistent. So I think what's happening in my example case is:

  - reject is set on entry (default for auth module)
  - the update { } block returns RLM_MODULE_UPDATED, but this is ignored,
    so the result remains as reject
  - on exit from the update { } block, the action code 0 means that the
    reject is ignored
  - if the update was inside an if block, then on exit from the if block
    the reject causes an immediate return
  - if the update was at the top level policy { }, the action code 0 means
    that the reject is ignored and then we step to the next entry in the group

Phew. I think it would be simpler if:
1. MOD_UPDATE had a set of non-zero default actions, including reject=return
2. a successful update { } block caused myresult to be set to RLM_UPDATED

but maybe there are reasons why these aren't done today.

And possibly, default actions could be copied from parent node rather than
setting defaults at every level, such as inside an 'if' block.

Regards,

Brian.

[authenticate]
group {
	pap {
		reject = 1
		fail = return
		ok = return
		handled = return
		invalid = return
		userlock = return
		notfound = 1
		noop = 2
		updated = 4
	}
	policy {
		update {
			reject = 0
			fail = 0
			ok = 0
			handled = 0
			invalid = 0
			userlock = 0
			notfound = 0
			noop = 0
			updated = 0
		}
		if {
			update {
				reject = 0
				fail = 0
				ok = 0
				handled = 0
				invalid = 0
				userlock = 0
				notfound = 0
				noop = 0
				updated = 0
			}
			reject = return
			fail = return
			ok = 3
			handled = return
			invalid = return
			userlock = return
			notfound = 1
			noop = 2
			updated = 4
		}
		ok {
			reject = return
			fail = return
			ok = 3
			handled = return
			invalid = return
			userlock = return
			notfound = 1
			noop = 2
			updated = 4
		}
		reject = return
		fail = return
		ok = 3
		handled = return
		invalid = return
		userlock = return
		notfound = 1
		noop = 2
		updated = 4
	}
	reject = return
	fail = 1
	ok = return
	handled = return
	invalid = 1
	userlock = return
	notfound = return
	noop = 1
	updated = 1
}