HUE-688 [beeswax] Beeswax shouldn't show saved queries from other users

Review Request #2040 — Created April 23, 2012 and submitted

commit 9c9f833cfb0d2910c7fe927989e3cfa6ee5ed6f4
Author: Romain Rigaux <>
Date:   Wed Apr 18 11:06:21 2012 -0700

    HUE-688 [beeswax] Beeswax shouldn't show saved queries from other users
    Share saved queries with all users. If 'share_saved_queries' is set
    to false, saved queries are visible only to the owner and administrators.

:100644 100644 cb2d39c... a18d3ac... M  apps/beeswax/src/beeswax/
:100644 100644 cfe4b71... 6befe11... M  apps/beeswax/src/beeswax/
:100644 100644 28e6e25... 8c8b666... M  apps/beeswax/src/beeswax/
:100644 100644 ce6edd7... c36709f... M  desktop/conf/pseudo-distributed.ini.tmpl
Ran unit tests
The added unit tests are not ran but will follow soon with
Visual testing by login with a super user and a normal user and flipping the switch

Implementation: it could be possible to refactor list_design and my_queries views but then we still need to play with the prefixes and we still want to be able to combine 'show all the saved queries' + filter with the 'user' URL parameter.
  • 2
  • 0
  • 15
  • 0
  • 17
Description From Last Updated
Same. We could just use SavedQuery.objects.get(id=design_id). bcwalrus bcwalrus
I don't understand this. When will user.username equal "_all"? bcwalrus bcwalrus
  1. Looks good. Nit on variable naming.
  2. apps/beeswax/src/beeswax/ (Diff revision 1)
    Would call this "SHARE_SAVED_QUERIES". It's more concise and direct. Note that if you make this change, you need to flip the boolean values.
    1. Indeed it is cleaner, thanks.
  3. apps/beeswax/src/beeswax/ (Diff revision 1)
    Would default to the current behaviour, so minimize disruption.
  4. apps/beeswax/src/beeswax/ (Diff revision 1)
    You need coerce_bool here. Otherwise, bool("false") -> True.
  5. apps/beeswax/src/beeswax/ (Diff revision 1)
    Would change to:
        Share saved queries with all users. If set to false, saved queries are visible only to the owner and administrators.
  1. Thanks!
  2. desktop/conf.dist/hue.ini (Diff revision 2)
    This is fine, for origin/master. No need to change anything.
    Note on packaging behaviour: If a newer RPM changes a conf file, RPM could move the existing conf file to .rpmsave. The user would need to rename it back, which is a hassle. The spec file may include an option to say keep the current one and save the new one to .rpmnew.
    1. Ok for the packaging behavior.
      If we remove it from the config file, is it a problem if it is not documented anywhere else? (but the pseudo-distributed template)
      I remove it for the branch-2.0 too?
    2. There is no need to change anything for master and branch-2.0, because the configs have changed so much already anyways.
  1. Man, it's hard. I think we're almost there.
    1. Thanks a lot for the review!
      I looked about the table but could not find a model a bout them. Are the permissions about them set on the HDFS? (because all the tables seem to be public)
  2. apps/beeswax/src/beeswax/ (Diff revision 4)
    set_for_testing() returns a callable. When you invoke it, it'll restore the previous config. To prevent test interference, you should do that.
    1. I did not know, thank you
  3. apps/beeswax/src/beeswax/ (Diff revision 4)
    Would also return the design object. Would return None on not found (instead of raising an exception). You can rename it to "authorized_get_design()".
    Same for history.
  4. apps/beeswax/src/beeswax/ (Diff revision 4)
    Nit: It's better to specify the format arguments as a tuple, i.e.:
      "Design %s does not exist" % (design_id,)
    Try this:
      design_id = (1, 2, 3)
      print "Design %s does not exist" % (design_id,)
      print "Design %s does not exist" % design_id
    1. I see, I changed all of them.
  5. apps/beeswax/src/beeswax/ (Diff revision 4)
    safe_get_design() returns a new design if design_id is None. So check_design_permission() shouldn't throw exception in that case.
    1. It should be good, check_design_permission() does nothing when design_id is None.
  6. apps/beeswax/src/beeswax/ (Diff revision 4)
    SavedQuery.get() verifies that the request user is the owner. (Even admin is disallowed here.)
    It doesn't make sense to check permission twice. If check_design_permission() has an extra arg to ensure actual ownership, then we don't have to use SavedQuery.get().
  7. apps/beeswax/src/beeswax/ (Diff revision 4)
    Same. We could just use SavedQuery.objects.get(id=design_id).
  8. apps/beeswax/src/beeswax/ (Diff revision 4)
    Since you don't need to filter on prefix:
      mutable_querydict = copy.copy(request.GET)
    1. Exact, I changed it.
  9. apps/beeswax/src/beeswax/ (Diff revision 4)
    I don't understand this. When will user.username equal "_all"?
    1. I was maybe over-thinking but if someone picks '_all' as username he will have access to all the history objects which could be a big problem?
    1. thanks again for the review.
      How about the Tables?
      If I create a table and then login as another user I can still see it. I did not see any Python table model: are the permissions managed by HDFS? (and we need to make sure the correction permissions are set on each table)
  2. apps/beeswax/src/beeswax/ (Diff revision 5)
    You implicitly return None, which is bad form. Would explicitly start with
        if design_id is None:
          return None
    (Same for get_history)
    1. Ok, I changed both of them.
  3. apps/beeswax/src/beeswax/ (Diff revision 5)
    (design_id), -> (design_id,)
    The comma needs to be inside the paren, because you want to declare a tuple. Without the comma, it's just grouping (no-op).
  4. apps/beeswax/src/beeswax/ (Diff revision 5)
    The first call gives you `design' already.
  5. apps/beeswax/src/beeswax/ (Diff revision 5)
    design = authorized_get_design(...)
    if design is None:
  6. apps/beeswax/src/beeswax/ (Diff revision 5)
    Same. Would combine the two calls.
  7. apps/beeswax/src/beeswax/ (Diff revision 5)
    Combine and check for None. Or enhance authorized_get_xyz() with an optional param to throw when not found.
    1. I see, I added a must_exist parameter to authorized_get_xyz(request, id, must_exist=True).
  8. apps/beeswax/src/beeswax/ (Diff revision 5)
    Combine and check for None.
  9. apps/beeswax/src/beeswax/ (Diff revision 5)
    You can assign the result to `query_history', and avoid another call to the DB.
  10. apps/beeswax/src/beeswax/ (Diff revision 5)
    Combine and check for None.
    1. It will throw an exception if the QueryHistory does not exist.
  11. apps/beeswax/src/beeswax/ (Diff revision 5)
    dict.copy() ?
    1. Sorry I thought I did it. 
    2. Actually I reverted it to the _copy_prefix(), this was creating a bug as request.GET is a special dictionary.
      print _copy_prefix('', request.GET)
      print dict.copy(request.GET)
      <QueryDict: {}>
  12. apps/beeswax/src/beeswax/ (Diff revision 5)
    We repurpose `user' to mean something else here. This is confusing. Not your fault though. Can we call this variable `user_filter' instead?
    One solution to the `_all' issue is to change it to `:all'. Colon is not a legal username character.
    1. I renamed the variable.
      Good idea for ':all'.
  2. apps/beeswax/src/beeswax/ (Diff revisions 5 - 6)
    This should be conditioned on must_exist.
    1. Exact and SavedQuery.objects.get(id=None) will also trigger a DoesNotExist.
  3. apps/beeswax/src/beeswax/ (Diff revisions 5 - 6)
  4. apps/beeswax/src/beeswax/ (Diff revisions 5 - 6)
    1. Good catch, if not we will get a 500.
  5. apps/beeswax/src/beeswax/ (Diff revisions 5 - 6)
    1. Same problem indeed, fixed.
  6. apps/beeswax/src/beeswax/ (Diff revisions 5 - 6)
    Thank you!
  7. apps/beeswax/src/beeswax/ (Diff revision 6)
  8. apps/beeswax/src/beeswax/ (Diff revision 6)
    Tip: I found those instance using `git grep -w _all apps/beeswax/src'.
    1. Good catch, I do the same with Eclipse but had only done 'apps/beeswax/src/templates'.
  1. Ship It!
Review request changed

Status: Closed (submitted)