regex_policy should use apache like ALLOW/DENY

[Henrik Ingo]

In auth_regex the pair of commands ACCEPT/DENY is confusing. It should have been either ACCEPT/REJECT (see iptables) or ALLOW/DENY (see apache httpd). My proposal is to make it ALLOW/DENY.

The fix should of course be backward compatible and still also support ACCEPT/DENY, but documentation should mark this usage as deprecated.

[Stewart Smith (stewart)]

On Tue, 14 Feb 2012 08:20:51 -0000, Henrik Ingo <email address hidden> wrote:
> In auth_regex the pair of commands ACCEPT/DENY is confusing. It should
> have been either ACCEPT/REJECT (see iptables) or ALLOW/DENY (see apache
> httpd). My proposal is to make it ALLOW/DENY.

Maybe allow all 4, with ACCEPT/REJECT being synonyms for ALLOW/DENY and
then it should all "just work"? :)

--
Stewart Smith

[Clint Byrum (clint-fewbar)]

Excerpts from Stewart Smith's message of Tue Feb 14 23:36:37 UTC 2012:
> On Tue, 14 Feb 2012 08:20:51 -0000, Henrik Ingo <email address hidden> wrote:
> > In auth_regex the pair of commands ACCEPT/DENY is confusing. It should
> > have been either ACCEPT/REJECT (see iptables) or ALLOW/DENY (see apache
> > httpd). My proposal is to make it ALLOW/DENY.
>
> Maybe allow all 4, with ACCEPT/REJECT being synonyms for ALLOW/DENY and
> then it should all "just work"? :)
>

+1 from me.

Note that this plugin *really* needs to have my other branch merged which
helps to reduce its memory impact on the system. It caches every lookup
right now. This branch fixes that by limiting the cache size:

https://code.launchpad.net/~clint-fewbar/drizzle/regex-policy-cache-limiter/+merge/53536

I wouldn't recommend using the plugin without that branch merged (failed
on some boost bug in SuSE)

[Anshu Kumar (ansharyan015)]

Hey,
I have started working on this. Was searching for the exact location of modification. Fixing!

[Anshu Kumar (ansharyan015)]

I have uploaded a patch supporting all 4 values, namely, ACCEPT, REJECT, ALLOW and DENY.
You can find the updated branch here.
lp:~ansharyan015/drizzle/trunk-bug-931917/

[Henrik Ingo (hingo)]

Hi Anshu

Thanks for the patch! Great to see you dive into Drizzle just like that :-)

I reviewed your patch. But first I need to comment on the above comments:

****
Ok, so we allow all of them. But we should still choose one as the recommended syntax (use this in examples and documentation) and the other should be considered backward compatiblity feature. So my proposal still is that ALLOW/DENY is the preferred pair actions and ACCEPT/REJECT is secondary.

With that in mind, I have a comment on the patch now:

*****

In policy.h:

> 104 return action == POLICY_ACCEPT ? "ACCEPT" : "DENY";

This should now return ALLOW/DENY (regardless of what the user has used in the config file).

Alternatively, each PolicyItem should store the actual string that was provided by the user and return that. (It seems this is only used to print a debug message, so I'm for the above and simpler solution.)

Apart from this, it looks good. For your GSoC application this is already a great showcase. But before this can be merged, you need to also add tests to cover the new action words and documentation. See tests/ and docs/ directories in the regex_policy directory.

[Anshu Kumar (ansharyan015)]

Hey Henrik,
The corresponding changes are done. I changed the default tests to check for ALLOW DENY policies. Additionally I have added a tests for Deprecated headers, i.e. ACCEPT and REJECT. The corresponding test name is basic_deprecated.test.
Also, the corresponding changes were made in the plugin docs to clearly mention that the policies should be "ALLOW" and "DENY". Although you can still use "ACCEPT" and "REJECT" but their use is deprecated.

Please check the updated branch.
lp:~ansharyan015/drizzle/trunk-bug-931917/

[Henrik Ingo (hingo)]

Is there a commit missing? Everything looks fine but I don't see that t/basic.policy would have changed as you claim.

A few comments on the docs:

Important: These docs are published on docs.drizzle.org and the same manual should cover all versions of Drizzle. So you must write something like:

In Drizzle 7 and Drizzle 7.1 the action words ACCEPT and DENY were used. Beginning with Drizzle 7.2.0 [yeah, I'm assuming this is the version where this code is published, but you would of course update it if it is not merged as expected] you should use ALLOW and DENY, but also ACCEPT and REJECT are supported for backward compatibility.

[Then continue with examples using ALLOW/DENY as you now do.]

You could remove the Examples section completely. Since the "Regex Policy File Format" does contain an example, there is no need to apologize.

[Henrik Ingo (hingo)]

Oh, you should also change the version of the plugin. Since this is a visible API change, I propose changin to 2.0. This should happen at the bottom of module.cc and end of documentation page.

[Anshu Kumar (ansharyan015)]

All changes done.
1. Version no. changed
2. basic.policy added (forgot to do this earlier)
3. Doc information changed
4. Examples section removed

Please review this.
lp:~ansharyan015/drizzle/trunk-bug-931917/

[Henrik Ingo (hingo)]

Looks great. Please make a merge request against trunk.

[Clint Byrum (clint-fewbar)]

Excerpts from Henrik Ingo's message of Sat Mar 31 16:04:22 UTC 2012:
> Looks great. Please make a merge request against trunk.
>

BTW, the simplest way to do this is, after a commit/push to

lp:~yourusername/drizzle/something

Is to run:

bzr lp-propose

It will automatically figure out how to propose a merge against drizzle from
the path prefix (lp:~yourusername/drizzle)

Enjoy! :)