ACCESS-52: Policy file should be configurable per database

Review Request #2844 — Created April 9, 2013 and submitted

brock
old-access
master
ACCESS-52
access
Implemented per-db configuration files. Per-db files can now be specified under the [databases] section.
Significant tests included and current tests pass.
  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
brock
prasadm
  1. Looks great overall !
    A few high level comments:
     - Some of the end-to-end tests have the untyped wildchars in the test generated policy file. Those tests needs to be updated after this patch. 
     - We should add end-to-end tests with the per-db policy format.
     - We need to test the impact on the cross database permissions with the per-db policy files in place ? Eg, If I join two tables from two different database which has two different policy files.
    I guess we can log separate tickets for fixing the broken tests and adding new tests.
    
    A few minor comments below.
    1. Hey thanks for the feedback!
      
      1) I updated those in ACCESS-19. That is not the "correct" patch but I noticed them while doing it...
      
      2) Agreed, I added those tests last week
      
      3) Agreed, I filed ACCESS-70 for that.
    2. cool. I will go ahead and approve it from my side. I believe Shreepadma is also reviewing it. will push the changes after her review.
      
  2. Nit: Should it be called something else to avoid confusion with core.Authorizable ..
    1. I am following the guava convention here. For example they have File utilities in Files and Executor utilities in Executors, List utils in Lists.., etc.
  3. Should the processing abort here ? If this is due to a typo in the file, it would end up bypassing the permissions. The error message in the log could be easily missed.
    1. That's a good question. So at that point in time we are processing the per-db ini file. I put that catch in place so that a single per-db file wouldn't cause cause issues for everyone else. I guess we should either remove it in which case a problem with a per-db file will cause a full aboard or change it to Exception since some of the constructors throw (KeyValue for example) throw an IllegalArgException.
      
      Thoughts?
    2. s/aboard/abort/
    3. I am in favor of failing the statement if the policy file format is not valid.
    4. Cool, I updated to patch to do this.
  4. 
      
prasadm
  1. +1
  2. 
      
brock
brock
Review request changed

Status: Closed (submitted)

Loading...