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_review
     Priority:  blocker      |  Milestone:
    Component:  Tor Support  |    Version:
   Resolution:               |   Keywords:  SponsorO
Actual Points:               |  Parent ID:  #10755
       Points:               |
-----------------------------+--------------------------

Comment (by lunar):

 Is it really needed to have a `pups_project` sub-directory? Probably
 related question: shouldn't be the `stats` and `webchat` modules be sub-
 modules of the `pups` module?

 Is `settings.py` meant to be configured by the local administrator? Then
 it should probably not be commited as is. Something like
 `settings_sample.py' should be in the repository instead and we should
 expect administrators to copy and adjust before running the application.
 It's worth being documented in the README.

 On the topic, could it be documented how to configure Apache to run both a
 staging and a production environment from the same codebase?  This looks
 unfortunately more complicated than it should:
 https://docs.djangoproject.com/en/dev/howto/deployment/wsgi/#configuring-
 the-settings-module
 https://stackoverflow.com/a/11515629
 Looks like WSGI daemon mode and multiple âwsgi.pyâ files are required.

 `wsgi.py` contains some boilerplate at the end that should probably be
 removed.

 `ChangePassForm.change_password()` control flow should be inverted (so
 that the âhappy pathâ can be easily seen):
 {{{
 if not new_pass_is_right():
     return False
 do_something_when_pass_is_right()
 return True
 }}}

 I don't understand the meaning of the `success` variable
 `Token.revoke_token()`.

 The semantics of `Token.revoke_token()` are still weird. I can pass it a
 list of token great. If it returns True, then I know that all the list was
 revoked, great. But when it returns False? I know that there's one in the
 list that was not good. But I don't know which one and with the current
 code, the ones after that will not have been modifiedâ Not good.  I'm not
 sure if Django handles database COMMIT/ROLLBACK properly, but if that's
 the case, the best course of action is to have the function raise an
 Exception so the previous changes are rolled back.

 Sorry we misunderstood each others for `Token.get_token()`. It should
 read:
 {{{
 def get_token(self, token):
     try:
         return Token.objects.get(token=token)
     except ObjectDoesNotExist:
         return []
 }}}

 `webchat/urls.py` contains mapping for `/chpass` and `/logged` URLs which
 do not belong to the webchat part of Pups. Same with `webchat/views.py`,
 so things might require to be moved around some more. Or everything merged
 together, that's probably fine given the current size.

 In `webchat/views.py`:

 `change_password()`, `create_token()`, `revoke_token()`, `chat()` all have
 `if â: return â; else` constructs.

 For `change_password()` is there an error message when the form is not
 valid?

 This should really be turned into its own method for readability:
 {{{
 token.get_assistant_tokens(User.objects.get(id =
 request.user.id)).filter(expires_at__gt=F('created_at')),
 }}}

 In `tokens_page()`, `params` is not used before the `render()` call at the
 end. It should be moved closer to that.

 Why use `HttpResponse` in `chat()` and not a proper template? Also can it
 be made that the HTTP error codes are 404 for invalid tokens and 410 for
 expired ones?

 Unless I'm mistaken `webchat/templates/tokens.html` directly contain the
 value of `token.comment`. It should be escaped to be displayed in an HTML
 context, otherwise that's a security issue.

 Please make the code PEP8 compliant unless there's a good reason for it:
 {{{
 $ pep8 pups_project | wc -l
 91
 }}}

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/10754#comment:26>
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