Re: [tor-bugs] #10754 [Tor Support]: Implement an invitation based token system into webchat

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#10754: Implement an invitation based token system into webchat
-----------------------------+----------------------------
     Reporter:  Sherief      |      Owner:  Sherief
         Type:  task         |     Status:  needs_revision
     Priority:  blocker      |  Milestone:
    Component:  Tor Support  |    Version:
   Resolution:               |   Keywords:  SponsorO
Actual Points:               |  Parent ID:  #10755
       Points:               |
-----------------------------+----------------------------

Comment (by Sherief):

 Replying to [comment:15 lunar]:

 > Comments after a first look at the code:
 >
 > `webchat/webchatapp/django.wsgi`:
 >
 > There is a hardcoded path and is probably missing documentation.


 Useless file, deleted.

 >
 > `webchat/webchatapp/models.py`:
 >
 > Sorry to be ignorant of Django idioms. In Rails world, we would call the
 columns `created_at` and `expire_at`.  Why not make `comment` a TEXT
 field?

 Fixed the *_at part but I think allowing more than 128 chars will clutter
 the page.

 >
 > I believe indentation is not PEP8 friendly.


 Probably fixed.


 >
 > `if <something>: return True; else: return False` constructions are
 usually redundant. `return q.t_id is not None` is nicer to read.


 Fixed.


 > `delete_token()` has weird semantics. There is no way a caller can
 properly handle failures.


 I tried to make it better, please check it again.


 > I had the impression that tokens would only expire and not be deleted so
 that users would be able to know that a token has been revoked

 I will add a revoked flag in the Token() model to differentiate between
 expiration and revoke. But I will also create a custom CLI script that the
 sys admin can use if one dat the db explodes or becomes too slow. Also,
 "Delete Selected" will be renamed as "Revoke Selected".

 > The `q` variable in `get_token()` should be removed.


 I just didn't want to query the DB twice for performance but I removed it
 anyway.


 >
 > `webchat/webchatapp/settings.py`:
 >
 > Path to `webchatDB/webchatDB.db` should be configurable. Probably
 through an environment variable. At least we need an easy way to configure
 a staging and a production environment.


 You can add more than one db because DATABASES is a dict of dicts and the
 current db is the default so we can do the following:

 {{{
 #!div style="font-size: 80%"
   {{{#!python
 DATABASES = {
     'default': {
         'ENGINE': 'django.db.backends.sqlite3',
         'NAME': os.path.abspath(os.path.join(os.path.dirname( __file__ ),
 'databases/pups.db')),
         'PASSWORD': '',
         'HOST': '',
         'PORT': '',
     } ,

     'testing':
     {
       'ENGINE': ...
     }
 }
 }}}
 }}}


 > Some of the boilerplate should probably be removed.
 >
 > `webchat/webchatapp/urls.py`:
 >
 > I guess the boilerplate could also be removed.


 Done.


 > `webchat/webchatapp/utils.py`:
 >
 > This one does not look to have much purpose.


 Yes, I had the intention to use it and forgot about it.


 > `webchat/webchatapp/views.py`:
 >
 > The name of this file is weird. In the MVC model, it contains
 controllers.


 That's how django names controllers. It can be renamed of course but that
 would look odd.


 >
 > `if something: return; else: â` is bad style. The `else` can be avoided
 entirely as the control flow will return anyway.


 I mitigated this.


 > The control flow of `change_password` is hard to follow.  See remark
 just above.
 >
 > One should always redirect when an action has been successful.
 Otherwise, clicking ârefreshâ will trigger the action again.


 I rewrote all of change_password() and added validation.


 > `token_page()` needs to be splitted up. One function by action.


 Done.


 > `chat()` should make the distinction between an expired token and a non-
 existing token.


 Consider it done.

 Thanks a bunch for this review, I appreciate it.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/10754#comment:18>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs