Enable ticket based authentication for Perforce

Review Request #1445 — Created March 3, 2010 and discarded

Information

Review Board

Reviewers

Introduced a new repository parameter named ticket_based_auth for use with perforce repositories.
When the parameter is set for a perforce repository it will used the ticket based authentication mechanism provided by perforce.
This security mechanism is required depending on the security level set for the perforce server.
The change also rewrites the PerforceTool get_file method to use the perforce API instead of calling the p4 command line utility.
I setup a local test Reviewboard and tried posting and viewing a review request against both a repository with ticket based authentication and one without.
chipx86
  1. 
      
  2. This file wasn't included.
  3. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
     
    "Ticket-based"
  4. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
     
    Rather than say to only use it with Perforce, I'd like to see some new logic in templates/admin/scmtools/repository/change_form.html to dynamically show/hide this form, like we do with the hosting settings.
  5. reviewboard/scmtools/models.py (Diff revision 1)
     
     
    I'm not sure I want to add repository-specific fields. What I would like to do, and now is a good time to do it I suppose, is add a "metadata" or "extra_config" (or something) JSONField where we can store additional data like this. That would be more expansive and even allow tools to store additional data per-repository, which could be very useful down the road.
    1. I'll have a look into this and the comment above and will rework the patch.
      Could take some time though as I have to find time to dig into those areas of before.
      I guess the raw file URL parameter could be converted to the proposed extra_config field aswell.
  6. reviewboard/scmtools/perforce.py (Diff revision 1)
     
     
     
    Should be alphabetical.
  7. reviewboard/scmtools/perforce.py (Diff revision 1)
     
     
    This might end up causing a lot of spew.
  8. reviewboard/scmtools/perforce.py (Diff revision 1)
     
     
    I think we used to have this but that there was some incompatibility with a version of P4API. I'm not sure. I'm going to have to see if David remembers the story here.
    1. I would prefer to require a newer version of the P4API then.
      Having this workaround in place would require even another command line call when using ticket based authentication.
  9. reviewboard/scmtools/perforce.py (Diff revision 1)
     
     
     
    Blank line between these.
  10. 
      
TR
  1. Florian, does this change REQUIRE you use ticket based auth?  When I looked at it a few days ago, it kind of seemed that way, but haven't had time to dig in.
    
    Right now we simply enter the ticket into our user password.  It would seem easier to have something automated, so I am interested in this change.
    1. The changes enables the Reviewboard backend to use ticket based authentication when talking to the perforce server. If checked it will always use this authentication mechanism.
      However this does not have any impact on the frontend (i.e. post-review).
  2. 
      
AR
AR
chipx86
  1. Make sure your branch is merged or rebased onto master. That's likely why the additional changes are on there.
  2. 
      
AR
SE
  1. 
      
  2. reviewboard/scmtools/perforce.py (Diff revision 4)
     
     
    I believe it will cause performance issues on the ticked based authentication P4 servers.
    I tested on my server, it's much slower than before. And I'm also surprised that it's faster after I moved these into self._connect(). In fact, I find out that run_login is just need in get_file() on my server.
    Please check again.
    1. Can't believe that it's slower as moving the login into connect will cause it to be called with each connect while it's only called once here.
      However it's clear that enabling the feature will cause an additional API call and slower the overall process. If you're enforcing login at server level you're trading security for performance.
      
      I guess you can configure for which calls authentication is needed at perforce server level, but I'm no perforce administrator. What I found is that there are no unauthenticated calls allowed on our server.
      Making the login feature configureable on a API call basis seems quite an overkill for me.
      
    2. Please refer to the comments below.
  3. 
      
SE
  1. 
      
  2. reviewboard/scmtools/perforce.py (Diff revision 4)
     
     
    I'd prefer to this:
    def _login(self):
            if repository.metadata.get('ticket_based_auth', False) and \
               self.p4.user and self.p4.password:
                self.p4.run_login()
    
    def _connect(self):
            if not self.p4.connected():
                self.p4.connect()
                self._login()
    
    1. See above:
      I don't want to rerun login on every connect/disconnect cycle.
  3. 
      
SE
  1. 
      
  2. reviewboard/scmtools/perforce.py (Diff revision 4)
     
     
    Meanwhile need to remove all self._disconnect() calls below except in __del__().
    Thus the performance is much better.
    1. I also wonder why the perforce scmtool disconnects after each call.
      However it was not the purpose of my change to redesign the PerforceTool class so I left that unchanged.
      If you wouldn't disconnect at all it would be sufficient to do the connect only once during initialization I guess.
  3. reviewboard/scmtools/perforce.py (Diff revision 4)
     
     
    Good idea for new P4API.
  4. 
      
chipx86
  1. 
      
  2. reviewboard/scmtools/core.py (Diff revision 4)
     
     
    Can you document this briefly?
  3. Add a trailing comma.
  4. initial=True? Not sure that makes any sense at all.
  5. reviewboard/scmtools/forms.py (Diff revision 4)
     
     
    "Ticket-based"
  6. reviewboard/scmtools/forms.py (Diff revision 4)
     
     
    "Perforce"
  7. reviewboard/scmtools/forms.py (Diff revision 4)
     
     
    Should be removed.
  8. reviewboard/scmtools/perforce.py (Diff revision 4)
     
     
     
    Should use spaces for indentation.
  9. reviewboard/scmtools/perforce.py (Diff revision 4)
     
     
     
    Use parens instead of \
  10. reviewboard/scmtools/perforce.py (Diff revision 4)
     
     
    We used to use run_print, but it would output extra blank lines which broke us. I don't feel comfortable making this change without very extensive testing.
    1. I would love to do some testing on that issue but would need further information.
      Do you know where perforce put the extra blank lines? And maybe the problem Reviewboard had with it?
      
      As another option:
      We could still correct the perforce print result instead of building our own print function based on command line calls.
    2. I'm not really sure what the repro case was. David might remember. Either way, we pretty much wouldn't be able to get this feature into 1.5.x with this particular change in, since we may end up breaking things. It would have to be part of a more fresh in-development release.
  11. 
      
AR
AR
AR
Review request changed
Status:
Discarded