#6053 Check for disabled users in user lookups

v1.0.0
closed
nobody
42cc (433)
General
2015-08-20
2013-04-04
No

identify_sender() calls claimed_by_user() which should only select where disabled:false

We should survey for other places that should check the relatively-new disabled field for users.

Related

Tickets: #6053

Discussion

  • Dave Brondsema

    Dave Brondsema - 2013-04-05
    • size: --> 1
     
  • Dave Brondsema

    Dave Brondsema - 2013-04-19
    • Milestone: forge-apr-19 --> forge-may-03
     
  • Dave Brondsema

    Dave Brondsema - 2013-04-23
    • Milestone: forge-may-03 --> forge-may-17
     
  • Dave Brondsema

    Dave Brondsema - 2013-05-03
    • Milestone: forge-may-17 --> forge-may-31
     
  • Dave Brondsema

    Dave Brondsema - 2013-05-20
    • Milestone: forge-may-31 --> forge-jun-06
     
  • Dave Brondsema

    Dave Brondsema - 2013-05-22
    • labels: --> 42cc
    • Size: 1 --> 0
    • Milestone: forge-jun-06 --> forge-backlog
     
  • Igor Bondarenko

    Igor Bondarenko - 2013-05-23
    • status: open --> in-progress
     
  • Igor Bondarenko

    Igor Bondarenko - 2013-05-23

    Created #363: [#6053] Check for disabled users in user lookups (2cp)

     

    Related

    Tickets: #6053

  • Igor Bondarenko

    Igor Bondarenko - 2013-06-12
    • status: in-progress --> code-review
     
  • Igor Bondarenko

    Igor Bondarenko - 2013-06-12

    Closed #363. je/42cc_6053

    We've changed the claimed_by_user() to check disabled field.

    There are other places we think this check should be done also. But I don't sure about them all, can you review them, please? And then we can make the changes.

    1. User lookup for autocompletion (Allura/allura/controllers/project.py:406, ProjectController.user_search())
    2. Finding users with named roles for given project (Allura/allura/model/project.py:708, Project.users())
    3. Various lookups for notifications. Disabled users don't need to receive any, I guess.
      • Notification.send_direct (Allura/allura/model/notification.py:237)
      • Notification.send_digest (Allura/allura/model/notification.py:261)
      • sendmail() task (Allura/allura/tasks/mail_tasks.py:87, Allura/allura/tasks/mail_tasks.py:99)
      • sendsimplemail() task (Allura/allura/tasks/mail_tasks.py:149)
    4. Auth-related lookups
      • AuthenticationProvider.authenticate_request (Allura/allura/lib/plugin.py:86)
      • LocalAuthenticationProvider.by_username (Allura/allura/lib/plugin.py:236)
      • LocalAuthenticationProvider.user_by_project_shortname (Allura/allura/lib/plugin.py:254)
      • LdapAuthenticationProvider.by_username (Allura/allura/lib/plugin.py:318)
      • LdapAuthenticationProvider._login (Allura/allura/lib/plugin.py:332)
     
  • Dave Brondsema

    Dave Brondsema - 2013-06-13
    • QA: Dave Brondsema
     
  • Dave Brondsema

    Dave Brondsema - 2013-06-13

    I've merged your current commits. Go ahead and make the other changes as follows:

    1. yes
    2. yes
    3. yes. Make sure to add proper handling if no user is found. Just one example is send_direct() which assumes it'll always find a user.
    4. authenticate_request already checks for disabled. Yes for the rest.

    Don't be overly concerned about testing all these spots. Some will be easy & good to test, others (e.g. ldap) probably don't have any existing tests, so I don't think its necessary to spend time now building out tests in that area.

     
  • Dave Brondsema

    Dave Brondsema - 2013-06-13
    • status: code-review --> in-progress
     
  • Igor Bondarenko

    Igor Bondarenko - 2013-06-17

    Created #376: [#6053] Check for disabled users in user lookups (followup) (2cp)

     

    Related

    Tickets: #6053

  • Igor Bondarenko

    Igor Bondarenko - 2013-06-21
    • status: in-progress --> code-review
     
  • Igor Bondarenko

    Igor Bondarenko - 2013-06-21

    Closed #376. je/42cc_6053_followup

     
  • Dave Brondsema

    Dave Brondsema - 2013-06-25
    • status: code-review --> closed
    • Milestone: forge-backlog --> forge-jun-28
     
  • Dave Brondsema

    Dave Brondsema - 2013-06-25

    This change seems unnecessary and tests pass without it, so I'm going to merge without it, unless there's some reason we need it:

    --- Allura/allura/controllers/basetest_project_root.py
    +++ Allura/allura/controllers/basetest_project_root.py
    @@ -123,6 +123,8 @@ def __call__(self, environ, start_response):
             c.project = M.Project.query.get(shortname='test', neighborhood_id=self.p_nbhd._id)
             auth = plugin.AuthenticationProvider.get(request)
             user = auth.by_username(environ.get('username', 'test-admin'))
    +        if not user:
    +            user = M.User.by_username('test-admin')
             environ['beaker.session']['userid'] = user._id
             c.user = auth.authenticate_request()
             return WsgiDispatchController.__call__(self, environ, start_response)
    
     
  • Dave Brondsema

    Dave Brondsema - 2013-06-25

    Ah, I guess I didn't run the tests properly earlier. I see the failures in allura.tests.functional.test_home:TestProjectHome.test_user_search_for_disabled_user and allura.tests.functional.test_auth:TestAuth.test_disabled_user now.

    I've added similar code back, but set the user to *anonymous instead of test-admin if the auth lookup can't find a valid user. I also added matching code to basetest_neighborhood_root.py