HUE-701 [jb] Job browser should not show the jobs from other users

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

romain
old-hue-rw
HUE-701
hue
bcwalrus
commit 2544f91e2f2141ce4bf215c04206ba6f1411d2bd
Author: Romain Rigaux <romain@cloudera.com>
Date:   Mon Apr 23 17:48:54 2012 -0700

    HUE-701 [jb] Job browser should not show the jobs from other users

:000000 100644 0000000... 66409c5... A	apps/jobbrowser/src/jobbrowser/conf.py
:100644 100644 0995185... 9d9ad12... M	apps/jobbrowser/src/jobbrowser/tests.py
:100644 100644 e6b2aa6... 44879b1... M	apps/jobbrowser/src/jobbrowser/views.py
:100644 100644 ce6edd7... f275c96... M	desktop/conf/pseudo-distributed.ini.tmpl
hadoop tests are also disabled
manual testing
  • 2
  • 0
  • 4
  • 0
  • 6
Description From Last Updated
You can "and" them together. bcwalrus bcwalrus
Great catch romain romain
romain
romain
bcwalrus
  1. Looks good. Minor comments below.
  2. apps/jobbrowser/src/jobbrowser/conf.py (Diff revision 1)
     
     
    "Share submitted job info ..."
  3. apps/jobbrowser/src/jobbrowser/views.py (Diff revision 1)
     
     
    The name is not entirely correct. It's owner or admin. Also, it's not required if the sharing is enabled.
    
    Perhaps call this "check_job_permission"?
  4. apps/jobbrowser/src/jobbrowser/views.py (Diff revision 1)
     
     
    """Ensure that the user has access to the job. Assumes that the wrapped function takes a `jobid' param."""
  5. apps/jobbrowser/src/jobbrowser/views.py (Diff revision 1)
     
     
     
    You can "and" them together.
    1. Yes but it won't fit and the first line tests for the config/super user permission. The second tests for the real condition (does this job belongs to the user). So cutting the tests in two seems simpler?
    2. changed to
      
          if not conf.SHARE_JOBS.get() and not request.user.is_superuser \
            and job.user != request.user.username:
  6. apps/jobbrowser/src/jobbrowser/views.py (Diff revision 1)
     
     
    Nit: " the job: %s" -> " job %s."
    
    (You don't need the `the' here.)
  7. apps/jobbrowser/src/jobbrowser/views.py (Diff revision 2)
     
     
     
     
     
     
     
     
    Would make get_matching_jobs() understand and check job visibility, rather than fixing the caller here. Probably don't need `user_exact'.
    1. Indeed, it should be cleaner now.
  8. 
      
romain
romain
bcwalrus
  1. Looks good. Thanks!
  2. apps/jobbrowser/src/jobbrowser/views.py (Diff revision 4)
     
     
    if not check_permission or request.user.is_superuser or j.profile.user == request.user)
  3. 
      
romain
  1. 
      
  2. apps/jobbrowser/src/jobbrowser/views.py (Diff revision 4)
     
     
    Great catch
  3. 
      
romain
romain
romain
Review request changed

Status: Closed (submitted)

Loading...