Re: [Semanticscuttle-devel] Updates to privatekey branch (aka Private Rss Feed)

[Christian W. <cw...@cw...>]

Hi Mark,


> Ok, I think I'm done.  Unit tested (added an additional test on the
> private key generation and exists).  Updated as per your comments
> below.  I think it's pretty good.  I'm all for constructive
> criticism, so feel free to let me know what you think.
I went through all commits and noted what I think could be better. Some
of the "bad" changes have been reverted in later commits (i.e.
htaccess), but I still note them here.

ce9124bfd576394b75c5e7a8f3a97dff7a89e747 mostly done 
- scripts/create-testbookmarks.php 
- javascript for key re-generation? why?
- "onClick" -> onclick
- utf8_strtolower?
- "function updateUser($uId, $password, $name, $privateKey, $email,
  $homepage, $uContent) {"
  breaks BC - backwards compatibility. If someone used the
  semanticscuttle API from his script, it would behave .. strangely now
  or stop working. I know that SC is not stable yet so BC breaks are
  allowed, but nevertheless it's not nice.
- "function PrivateKeyExists" - methods begin with a lowercase letter.
  Classes with an uppercase one
- same method: $output is a bad variable name since it's a return
  value. $exists would be better
- src/SemanticScuttle/db/mysqli.php: echo $sql_report?
- Changes to www/.htaccess? Why?
- why change getBookmarks() to add privatekey?
  why not "log in" the user temporarily?

97717684c429ee4e4827f4e4a120a019cbd3f78f added new file
- commitdescription is .. ambiguous
- function getNewPrivateKey() should be in the user model, it belongs
  there. maybe someone else wants to re-generate the key not with ajax

a32c9a1578c218728e0c40219d993b0a709033e7 added secondary rss feed to
bookmark page to test
- scripts/create-testbookmarks.php changed again
- www/bookmarks.php: "(private)" not translatable. Use sprintf and a
  configurable string from the translation

43ad8e77250cbc8e67f7f22127314fbe7c76b23d Cleaned up User.php and moved
Private Key function to it
- src/SemanticScuttle/Service/User.php: constructors do not have a
  @return value since it's known what they return
- "@param integer $nb Max number of usrs"
- "@return array Array of users" - array of user objects? array of user
  data arrays from database? I have to look at the code to see what it
  really returns
- "function & getUsers($nb = 0)" -> add visibility modificator
  (public/protected/private) -- why "&"?

f6873268e58a4cd3a1e32d1aa03ce7f5795752bc Updated tests to reflect
private key changes
- "public function testTestUniquePrivateId()":
  "$this->assertTrue($this->us->privateKeyExists($randKey));" is the
  last test, but you should verify if the user you created is the one
  that got the key.


- main page misses private key support
- you would need to test getBookmarks() with private key to make sure
  that other user's private bookmarks do not get returned when I pass
  my private key
- wrong privatekey should return an auth error http status - currently
  the user does not know if it worked or not
- my main problem is that you changed getBookmarks() to add the
  privatekey parameter to it. This is unfortunate since it adds a new
  "hole" in the security of the application, deep inside the API. The
  current security measures (user authentication, fetching bookmarks
  for the authenticated user) are well tested and know to work. Now
  that you add a private key parameter, we need a new suite of tests
  covering all the cases like
  "privatekey, own private bookmarks, foreign private bookmarks"
    -> user needs only to get his own private bookmarks, not the
    private bookmarks of other users.
  I personally would prefer that when accessing the feed with a private
  key, the user is logged in but not stored in the session. With the
  current user being available, getBookmarks() would automatically work
  correctly. Not storing it in the session would make sure the user
  cannot continue to do other things with the private key.
- You often committed changes to the privatekey feature and cleanup work
  in one single commit. while cleaning up is great and needs to be
  done, please commit them in separate commits. This makes review and
  merging easier. Ideally, cleanup would be made in a separate branch,
  but even I am too lazy to do that :) Separate commits however are a
  must.

Generally, great first work on SemanticScuttle. Don't let my comments
scare you off, I really appreciate your work.

-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-