Page MenuHomePhabricator

Switch to restrictions-based access control
Closed, DeclinedPublic


After T135278 is resolved and all the restrictions are backfilled in RESTBase we cat switch from using revision_table_access_check_filter to access_check_filter and remove the former. It should be done in several steps:

  • Temporarily enable both filters and compare the results
  • Remove the revision-based filter

Unfortunately, currently it's not possible because of the bug in MCS that makes the new filter fail sometimes T148590

Event Timeline

GWicke triaged this task as Medium priority.Aug 8 2017, 9:42 PM

In today's team sync meeting, we briefly touched on the possibility of combining the migration to the restriction table with the move to Cassandra 3. I think combining the two is attractive, as it lets us leverage the parallel double write / read testing we are doing anyway to test the new restriction storage as well. Doing this migration also lets us drop the revision table, in favor of action API requests for the few direct requests to the /title/{title} endpoint (T158100).

Before we can migrate, we will need to re-do the back-fill for historical restriction data. We have a script for this already, so this shouldn't be too hard. To ensure that redirect information is filled in properly, we should deploy the restriction table before the big HTML migration. Redirects are extracted on parse, so we will ensure that we have captured redirects for anything in storage by starting with a clean slate.

So there's been an outstanding issue we've never resolved with the restriction table code - the sort order of the restriction table was wrong. I've revived and rebased a very old PR to revert the restriction table sort order here:

However, the tests are failing right now and that uncovers a minor design flaw in our approach. Many of our access control and redirection tests are running in parallel. In the revision table filter that wasn't a problem, because the filter went for a revision explicitly and then it went for HTML if the revision info stated that it's a redirect. So the order of test execution didn't matter - even if the test for summary is executed before the test for HTML, we ensured to have all the data before hand. With the restriction table it's not like that since we rely on the restriction table being complete and we only fill it when we serenader HTML. That means that if the test for the summary is executed before the test for HTML on the same page, the summary test will fail since the restriction table is not filled with the data yet.

In tests we can easily fix it by executing HTML tests before the summary/mobile tests, however this uncovers a race condition for production - if a request for summary/mobile content comes before ChangeProp rerendered HTML we will serve incorrect/restricted data and it will be cached in varnish. This is not a big issue because as soon as a serenader for HTML comes it will also rerender summary/mobile content and purge varnishes, so I think we can neglect this race condition.

@Pchelolo, agreed that the race is not critical. It is essentially just the normal delay in propagating new restrictions, and should be short in any case.

We've decided that due to complexity of the restriction-based access control we will not follow this path