Re: authz changes between 1.9 and 1.10

Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:

> Branko Čibej wrote on Mon, 30 Jul 2018 03:07 +0200:
>> It's definitely better to have consistent behaviour across all rule
>> types.
>
> +1

I like the idea of achieving consistency by making the duplicate entries
into an error: it changes the behaviour of 1.10 but anyone affected gets
an error. It's also a simpler version of my existing patch.

Consistency is more of a problem for the inheritance case:

  [/path]
  userA = rw
  [repo:/path]
  userB = rw

because any change will silently change the behaviour of 1.10. The glob
implementation made file order (sequence number in the implementation)
important and my experimental inheritance patch arbitrarily picks the
per-repository sequence number when inheriting but without any real
justification. The choice has no effect when using the glob
implementation on a 1.9 authz file, but the choice starts to matter when
glob rules are present.

The release notes for 1.10 didn't specify how glob rules are
prioritised, so anyone using them had to read the design docs or
experiment. How inheritance affects glob rules is the outstanding
behaviour question. Do the glob inherit like non-glob rules? How does
the sequence number of inherited rules get defined? One option is to
make :glob: into an error, and introduce :GLOB: with defined rules for
inheritance.

Here's a quick patch to implement the new duplicate error, and to
inherit non-glob rules only:

Index: subversion/libsvn_repos/authz.c
===================================================================
--- subversion/libsvn_repos/authz.c (revision 1837006)
+++ subversion/libsvn_repos/authz.c (working copy)
@@ -872,6 +872,24 @@ finalize_tree(node_t *node,
   combine_right_limits(sum, local_sum);
 }
 
+static authz_acl_t *
+combine_inherit(const authz_acl_t *prev_acl, const authz_acl_t *acl,
+ apr_pool_t *result_pool)
+{
+ authz_acl_t *combined = apr_palloc(result_pool, sizeof(authz_acl_t));
+
+ *combined = *acl;
+
+ combined->has_anon_access |= prev_acl->has_anon_access;
+ combined->anon_access |= prev_acl->anon_access;
+ combined->has_authn_access |= prev_acl->has_authn_access;
+ combined->authn_access |= prev_acl->authn_access;
+
+ combined->user_access = apr_array_append(result_pool, acl->user_access,
+ prev_acl->user_access);
+ return combined;
+}
+
 /* From the authz CONFIG, extract the parts relevant to USER and REPOSITORY.
  * Return the filtered rule tree.
  */
@@ -912,6 +930,9 @@ create_user_authz(authz_full_t *authz,
                                                    AUTHZ_ANY_REPOSITORY));
                   SVN_ERR_ASSERT_NO_RETURN(strcmp(acl->rule.repos,
                                                   AUTHZ_ANY_REPOSITORY));
+
+ if (!acl->glob_rule && !prev_acl->glob_rule)
+ acl = combine_inherit(prev_acl, acl, result_pool);
                   apr_array_pop(acls);
                 }
             }
Index: subversion/libsvn_repos/authz.h
===================================================================
--- subversion/libsvn_repos/authz.h (revision 1837006)
+++ subversion/libsvn_repos/authz.h (working copy)
@@ -254,6 +254,8 @@ typedef struct authz_acl_t
      matches. */
   int sequence_number;
 
+ svn_boolean_t glob_rule;
+
   /* The parsed rule. */
   authz_rule_t rule;
 
Index: subversion/libsvn_repos/authz_parse.c
===================================================================
--- subversion/libsvn_repos/authz_parse.c (revision 1837006)
+++ subversion/libsvn_repos/authz_parse.c (working copy)
@@ -127,6 +127,12 @@ typedef struct ctor_baton_t
   svn_membuf_t rule_path_buffer;
   svn_stringbuf_t *rule_string_buffer;
 
+ /* Accumulates the access entries when processing a rule. */
+ apr_hash_t *rule_entries;
+
+ /* For duplicating the access entries when processing a rule. */
+ apr_pool_t *rule_entries_pool;
+
   /* The parser's scratch pool. This may not be the same pool as
      passed to the constructor callbacks, that is supposed to be an
      iteration pool maintained by the generic parser.
@@ -229,6 +235,9 @@ create_ctor_baton(apr_pool_t *result_pool,
   svn_membuf__create(&cb->rule_path_buffer, 0, parser_pool);
   cb->rule_string_buffer = svn_stringbuf_create_empty(parser_pool);
 
+ cb->rule_entries = NULL;
+ cb->rule_entries_pool = svn_pool_create(parser_pool);
+
   cb->parser_pool = parser_pool;
 
   insert_default_acl(cb);
@@ -684,6 +693,8 @@ rules_open_section(void *baton, svn_stringbuf_t *s
 
   SVN_ERR(check_open_section(cb, section));
 
+ svn_pool_clear(cb->rule_entries_pool);
+
   /* Parse rule property tokens. */
   if (*rule != ':')
     glob = FALSE;
@@ -741,6 +752,8 @@ rules_open_section(void *baton, svn_stringbuf_t *s
       SVN_ERR(parse_rule_path(&acl.acl.rule, cb, glob, rule, rule_len,
                               section->data));
       SVN_ERR(check_unique_rule(cb, &acl.acl.rule, section->data));
+ cb->rule_entries = svn_hash__make(cb->rule_entries_pool);
+ acl.acl.glob_rule = glob;
     }
   else if (0 == strcmp(section->data, aliases_section))
     {
@@ -820,6 +833,13 @@ add_access_entry(ctor_baton_t *cb, svn_stringbuf_t
 
   SVN_ERR_ASSERT(acl != NULL);
 
+ if (svn_hash_gets(cb->rule_entries, name))
+ return svn_error_createf(SVN_ERR_AUTHZ_INVALID_CONFIG, NULL,
+ _("Duplicate entry '%s' in authz rule [%s]"),
+ name, section->data);
+ svn_hash_sets(cb->rule_entries,
+ apr_pstrmemdup(cb->rule_entries_pool, name, name_len), "");
+
   if (inverted)
     {
       ++name;

-- 
Philip