Author: svn-role Date: Wed Mar 6 04:00:10 2019 New Revision: 1854882 URL: http://svn.apache.org/viewvc?rev=1854882&view=rev Log: Merge the r1847598 group from trunk:
* r1847598, r1847697, r1847922, r1847924, r1847946 Fix SVN-4793: authz rights from inverted access selectors were not accounted for at the global level, causing wrong authz check results. Justification: Fixes a regression in the new authz parser and resolver. Notes: - r1847598 and r1847697 are only test suite changes, but by including them we can avoids creating a backport branch. - r1847924 and r1847946 are tiny but IMO relevant comment tweaks. Votes: +1: brane, stefan2, stsp Modified: subversion/branches/1.10.x/ (props changed) subversion/branches/1.10.x/STATUS subversion/branches/1.10.x/subversion/libsvn_repos/authz.h subversion/branches/1.10.x/subversion/libsvn_repos/authz_info.c subversion/branches/1.10.x/subversion/libsvn_repos/authz_parse.c subversion/branches/1.10.x/subversion/tests/cmdline/authz_tests.py subversion/branches/1.10.x/subversion/tests/cmdline/svnauthz_tests.py Propchange: subversion/branches/1.10.x/ ------------------------------------------------------------------------------ --- svn:mergeinfo (original) +++ svn:mergeinfo Wed Mar 6 04:00:10 2019 @@ -103,4 +103,4 @@ /subversion/branches/verify-at-commit:1462039-1462408 /subversion/branches/verify-keep-going:1439280-1546110 /subversion/branches/wc-collate-path:1402685-1480384 -/subversion/trunk:1817837,1817856,1818577-1818578,1818584,1818651,1818662,1818727,1818801,1818803,1818807,1818868,1818871,1819036-1819037,1819043,1819049,1819052,1819093,1819146,1819162,1819444,1819556-1819557,1819603,1819804,1819911,1820044,1820046-1820047,1820518,1820627,1820718,1820778,1821183,1821224,1821621,1821678,1822401,1822587,1822591,1822996,1823202-1823203,1823211,1823327,1823791,1823966,1823989,1824033,1825024,1825045,1825215,1825266,1825306,1825709,1825711,1825721,1825736,1825778,1825783,1825787-1825788,1825979,1826720-1826721,1826747,1826811,1826814,1826877,1826907,1826971,1827105,1827114,1827191,1827562,1827574,1827670,1828613,1829012,1829015,1829241,1829260,1829344,1830083,1830882-1830883,1830885,1830900-1830901,1831110,1831112,1831540,1833465,1833621,1833836,1833842,1833864,1833866,1833895,1833897,1833899,1833901,1835760,1836306,1836762,1836802,1836960,1836963,1836968,1836976,1837037,1837790,1838813,1839662,1839703,1839734,1839739,1840991,1842260,1842262,1842264,184 3888,1844882,1844987,1845204,1845261,1845408,1845555,1845577,1846299,1846403,1846406,1846704,1847181-1847182,1847188,1847264,1847572,1847596,1850348,1850621 +/subversion/trunk:1817837,1817856,1818577-1818578,1818584,1818651,1818662,1818727,1818801,1818803,1818807,1818868,1818871,1819036-1819037,1819043,1819049,1819052,1819093,1819146,1819162,1819444,1819556-1819557,1819603,1819804,1819911,1820044,1820046-1820047,1820518,1820627,1820718,1820778,1821183,1821224,1821621,1821678,1822401,1822587,1822591,1822996,1823202-1823203,1823211,1823327,1823791,1823966,1823989,1824033,1825024,1825045,1825215,1825266,1825306,1825709,1825711,1825721,1825736,1825778,1825783,1825787-1825788,1825979,1826720-1826721,1826747,1826811,1826814,1826877,1826907,1826971,1827105,1827114,1827191,1827562,1827574,1827670,1828613,1829012,1829015,1829241,1829260,1829344,1830083,1830882-1830883,1830885,1830900-1830901,1831110,1831112,1831540,1833465,1833621,1833836,1833842,1833864,1833866,1833895,1833897,1833899,1833901,1835760,1836306,1836762,1836802,1836960,1836963,1836968,1836976,1837037,1837790,1838813,1839662,1839703,1839734,1839739,1840991,1842260,1842262,1842264,184 3888,1844882,1844987,1845204,1845261,1845408,1845555,1845577,1846299,1846403,1846406,1846704,1847181-1847182,1847188,1847264,1847572,1847596,1847598,1847697,1847922,1847924,1847946,1850348,1850621 Modified: subversion/branches/1.10.x/STATUS URL: http://svn.apache.org/viewvc/subversion/branches/1.10.x/STATUS?rev=1854882&r1=1854881&r2=1854882&view=diff ============================================================================== --- subversion/branches/1.10.x/STATUS (original) +++ subversion/branches/1.10.x/STATUS Wed Mar 6 04:00:10 2019 @@ -87,18 +87,6 @@ Veto-blocked changes: Approved changes: ================= -* r1847598, r1847697, r1847922, r1847924, r1847946 - Fix SVN-4793: authz rights from inverted access selectors were not - accounted for at the global level, causing wrong authz check results. - Justification: - Fixes a regression in the new authz parser and resolver. - Notes: - - r1847598 and r1847697 are only test suite changes, but by including - them we can avoids creating a backport branch. - - r1847924 and r1847946 are tiny but IMO relevant comment tweaks. - Votes: - +1: brane, stefan2, stsp - * r1851676, r1851687, r1851791 Allow the use of empty groups in authz rules. Justification: Modified: subversion/branches/1.10.x/subversion/libsvn_repos/authz.h URL: http://svn.apache.org/viewvc/subversion/branches/1.10.x/subversion/libsvn_repos/authz.h?rev=1854882&r1=1854881&r2=1854882&view=diff ============================================================================== --- subversion/branches/1.10.x/subversion/libsvn_repos/authz.h (original) +++ subversion/branches/1.10.x/subversion/libsvn_repos/authz.h Wed Mar 6 04:00:10 2019 @@ -139,6 +139,10 @@ typedef struct authz_full_t svn_boolean_t has_authn_rights; authz_global_rights_t authn_rights; + /* Globally accumulated rights from inverted selectors. */ + svn_boolean_t has_neg_rights; + authz_global_rights_t neg_rights; + /* Globally accumulated rights, for all concrete users mentioned in the authz file. The key is the user name, the value is an authz_global_rights_t*. */ @@ -257,14 +261,19 @@ typedef struct authz_acl_t /* The parsed rule. */ authz_rule_t rule; - /* Access rights for anonymous users */ + + /* Access rights for anonymous users. */ svn_boolean_t has_anon_access; authz_access_t anon_access; - /* Access rights for authenticated users */ + /* Access rights for authenticated users. */ svn_boolean_t has_authn_access; authz_access_t authn_access; + /* Access rights from inverted selectors. */ + svn_boolean_t has_neg_access; + authz_access_t neg_access; + /* All other user- or group-specific access rights. Aliases are replaced with their definitions, rules for the same user or group are merged. */ Modified: subversion/branches/1.10.x/subversion/libsvn_repos/authz_info.c URL: http://svn.apache.org/viewvc/subversion/branches/1.10.x/subversion/libsvn_repos/authz_info.c?rev=1854882&r1=1854881&r2=1854882&view=diff ============================================================================== --- subversion/branches/1.10.x/subversion/libsvn_repos/authz_info.c (original) +++ subversion/branches/1.10.x/subversion/libsvn_repos/authz_info.c Wed Mar 6 04:00:10 2019 @@ -148,37 +148,50 @@ svn_authz__get_global_rights(authz_right { /* Check if we have explicit rights for anonymous access. */ if (authz->has_anon_rights) - return resolve_global_rights(rights_p, &authz->anon_rights, repos); + { + return resolve_global_rights(rights_p, &authz->anon_rights, repos); + } + else + { + /* Return the implicit rights, i.e., none. */ + rights_p->min_access = authz_access_none; + rights_p->max_access = authz_access_none; + return FALSE; + } } else { + svn_boolean_t combine_user_rights = FALSE; + svn_boolean_t access = FALSE; + /* Check if we have explicit rights for this user. */ const authz_global_rights_t *const user_rights = svn_hash_gets(authz->user_rights, user); if (user_rights) { - svn_boolean_t explicit - = resolve_global_rights(rights_p, user_rights, repos); - - /* Rights given to _any_ authenticated user may apply, too. */ - if (authz->has_authn_rights) - { - authz_rights_t authn; - explicit |= resolve_global_rights(&authn, &authz->authn_rights, - repos); - combine_rights(rights_p, rights_p, &authn); - } - return explicit; + access = resolve_global_rights(rights_p, user_rights, repos); + combine_user_rights = TRUE; + } + else if (authz->has_neg_rights) + { + /* Check if inverted-rule rights apply */ + access = resolve_global_rights(rights_p, &authz->neg_rights, repos); + combine_user_rights = TRUE; } - /* Check if we have explicit rights for authenticated access. */ + /* Rights given to _any_ authenticated user may apply, too. */ if (authz->has_authn_rights) - return resolve_global_rights(rights_p, &authz->authn_rights, repos); - } + { + authz_rights_t authn; + access |= resolve_global_rights(&authn, &authz->authn_rights, repos); - /* Fall-through: return the implicit rights, i.e., none. */ - rights_p->min_access = authz_access_none; - rights_p->max_access = authz_access_none; - return FALSE; + if (combine_user_rights) + combine_rights(rights_p, rights_p, &authn); + else + *rights_p = authn; + } + + return access; + } } Modified: subversion/branches/1.10.x/subversion/libsvn_repos/authz_parse.c URL: http://svn.apache.org/viewvc/subversion/branches/1.10.x/subversion/libsvn_repos/authz_parse.c?rev=1854882&r1=1854881&r2=1854882&view=diff ============================================================================== --- subversion/branches/1.10.x/subversion/libsvn_repos/authz_parse.c (original) +++ subversion/branches/1.10.x/subversion/libsvn_repos/authz_parse.c Wed Mar 6 04:00:10 2019 @@ -154,6 +154,8 @@ static const char anon_access_token[] = /* The authenticated access token. */ static const char authn_access_token[] = "$authenticated"; +/* Fake token for inverted rights. */ +static const char neg_access_token[] = "~~$inverted"; /* Initialize a rights structure. The minimum rights start with all available access and are later @@ -191,6 +193,8 @@ insert_default_acl(ctor_baton_t *cb) acl->acl.has_anon_access = TRUE; acl->acl.authn_access = authz_access_none; acl->acl.has_authn_access = TRUE; + acl->acl.neg_access = authz_access_none; + acl->acl.has_neg_access = TRUE; acl->acl.user_access = NULL; acl->aces = svn_hash__make(cb->parser_pool); acl->alias_aces = svn_hash__make(cb->parser_pool); @@ -208,6 +212,7 @@ create_ctor_baton(apr_pool_t *result_poo authz_full_t *const authz = apr_pcalloc(result_pool, sizeof(*authz)); init_global_rights(&authz->anon_rights, anon_access_token, result_pool); init_global_rights(&authz->authn_rights, authn_access_token, result_pool); + init_global_rights(&authz->neg_rights, neg_access_token, result_pool); authz->user_rights = svn_hash__make(result_pool); authz->pool = result_pool; @@ -758,6 +763,8 @@ rules_open_section(void *baton, svn_stri acl.acl.has_anon_access = FALSE; acl.acl.authn_access = authz_access_none; acl.acl.has_authn_access = FALSE; + acl.acl.neg_access = authz_access_none; + acl.acl.has_neg_access = FALSE; acl.acl.user_access = NULL; acl.aces = svn_hash__make(cb->parser_pool); @@ -958,6 +965,14 @@ add_access_entry(ctor_baton_t *cb, svn_s if (!aliased && *ace->name != '@') prepare_global_rights(cb, ace->name); } + + /* Propagate rights for inverted selectors to the global rights, otherwise + an access check can bail out early. See: SVN-4793 */ + if (inverted) + { + acl->acl.has_neg_access = TRUE; + acl->acl.neg_access |= access; + } } return SVN_NO_ERROR; @@ -1271,6 +1286,12 @@ expand_acl_callback(void *baton, update_global_rights(&cb->authz->authn_rights, acl->rule.repos, acl->authn_access); } + if (acl->has_neg_access) + { + cb->authz->has_neg_rights = TRUE; + update_global_rights(&cb->authz->neg_rights, + acl->rule.repos, acl->neg_access); + } SVN_ERR(svn_iter_apr_hash(NULL, cb->authz->user_rights, update_user_rights, acl, scratch_pool)); return SVN_NO_ERROR; Modified: subversion/branches/1.10.x/subversion/tests/cmdline/authz_tests.py URL: http://svn.apache.org/viewvc/subversion/branches/1.10.x/subversion/tests/cmdline/authz_tests.py?rev=1854882&r1=1854881&r2=1854882&view=diff ============================================================================== --- subversion/branches/1.10.x/subversion/tests/cmdline/authz_tests.py (original) +++ subversion/branches/1.10.x/subversion/tests/cmdline/authz_tests.py Wed Mar 6 04:00:10 2019 @@ -1663,6 +1663,53 @@ def remove_access_after_commit(sbox): expected_status, [], True) +@Issue(4793) +@Skip(svntest.main.is_ra_type_file) +def inverted_group_membership(sbox): + "access rights for user in inverted group" + + sbox.build(create_wc = False) + + svntest.actions.enable_revprop_changes(sbox.repo_dir) + write_restrictive_svnserve_conf(sbox.repo_dir) + write_authz_file(sbox, + {"/" : ("$anonymous =\n" + "~@readonly = rw\n" + "@readonly = r\n")}, + {"groups": "readonly = %s\n" % svntest.main.wc_author2}) + + expected_output = svntest.verify.UnorderedOutput(['A/\n', 'iota\n']) + + # User mentioned in the @readonly group can read ... + svntest.actions.run_and_verify_svn(expected_output, [], + 'list', + '--username', svntest.main.wc_author2, + sbox.repo_url) + + # ... but the access control entry for the inverted group isn't applied. + svntest.actions.run_and_verify_svn(expected_output, [], + 'list', + '--username', svntest.main.wc_author, + sbox.repo_url) + +@Skip(svntest.main.is_ra_type_file) +def group_member_empty_string(sbox): + "group definition ignores with empty member" + + sbox.build(create_wc = False) + + write_restrictive_svnserve_conf(sbox.repo_dir) + write_authz_file(sbox, + {"/" : ("$anonymous =\n" + "@readonly = r\n")}, + {"groups": "readonly = , %s\n" % svntest.main.wc_author}) + + expected_output = svntest.verify.UnorderedOutput(['A/\n', 'iota\n']) + svntest.actions.run_and_verify_svn(expected_output, [], + 'list', + '--username', svntest.main.wc_author, + sbox.repo_url) + ######################################################################## # Run the tests @@ -1700,6 +1747,8 @@ test_list = [ None, authz_file_external_to_authz, authz_log_censor_revprops, remove_access_after_commit, + inverted_group_membership, + group_member_empty_string, ] serial_only = True Modified: subversion/branches/1.10.x/subversion/tests/cmdline/svnauthz_tests.py URL: http://svn.apache.org/viewvc/subversion/branches/1.10.x/subversion/tests/cmdline/svnauthz_tests.py?rev=1854882&r1=1854881&r2=1854882&view=diff ============================================================================== --- subversion/branches/1.10.x/subversion/tests/cmdline/svnauthz_tests.py (original) +++ subversion/branches/1.10.x/subversion/tests/cmdline/svnauthz_tests.py Wed Mar 6 04:00:10 2019 @@ -898,6 +898,75 @@ def svnauthz_compat_mode_repo_test(sbox) repo_url + "/zilch" ) + +@Issue(4793) +def svnauthz_inverted_selector_test(sbox): + "test inverted selector rights" + + # build an authz file + authz_content = ("[groups]\n" + "grouped = grouper\n" + + "[aliases]\n" + "aliased = aliaser\n" + + "[A:/]\n" + "@grouped = r\n" + "~@grouped = rw\n" + + "[B:/]\n" + "&aliased = r\n" + "~&aliased = rw\n" + + "[C:/]\n" + "user = r\n" + "~user = rw\n" + + "[D:/]\n" + "~@grouped = rw\n" + "not-grouper = r\n" + + "[E:/]\n" + "~&aliased = rw\n" + "not-aliaser = r\n" + + "[F:/]\n" + "~user = rw\n" + "not-user = r\n") + + (authz_fd, authz_path) = tempfile.mkstemp() + svntest.main.file_write(authz_path, authz_content) + + def accessof(repos, user, expected_stdout): + return svntest.actions.run_and_verify_svnauthz( + expected_stdout, None, 0, False, 'accessof', + '--repository', repos, '--username', user, authz_path) + + accessof('A', 'grouper', 'r') + accessof('A', 'not-grouper', 'rw') + + accessof('B', 'aliaser', 'r') + accessof('B', 'not-aliaser', 'rw') + + accessof('C', 'user', 'r') + accessof('C', 'not-user', 'rw') + + accessof('D', 'grouper', 'no') + accessof('D', 'not-grouper', 'r') + accessof('D', 'other', 'rw') + + accessof('E', 'aliaser', 'no') + accessof('E', 'not-aliaser', 'r') + accessof('E', 'other', 'rw') + + accessof('F', 'user', 'no') + accessof('F', 'not-user', 'r') + accessof('F', 'other', 'rw') + + os.close(authz_fd) + os.remove(authz_path) + + ######################################################################## # Run the tests @@ -916,6 +985,7 @@ test_list = [ None, svnauthz_accessof_txn_test, svnauthz_compat_mode_file_test, svnauthz_compat_mode_repo_test, + svnauthz_inverted_selector_test, ] if __name__ == '__main__':