semanticscuttle-devel

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

[Mark P. <mpe...@gm...>]

Hey guys, I've pushed the latest changes to the privatekey branch that
contains two things.

First, I did some code cleanup which helped me get more familiar with the
code base.

Second, I'm mostly done with the private RSS feed changes as per feature
request # *3164013*.

What's still pending:
1) make either an update utility or change the tables.sql to include some
code that auto populates the new user column with a unique md5() value.
2) I've only changed the bookmarks.php page to show the addiitonal RSS feed.
 Before I propagate that change to the other pages with rss feeds, I'd like
to specifically ask for thoughts on distinguishing the difference between
the feeds.  Is the hover-text enough, or do we need to have a different icon
for the private feed (like the common blue one).

I don't think there is anything else pending, but am certainly open to any
changes.

Thanks all,
Mark Pemberton

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

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

Hi Mark,



> Hey guys, I've pushed the latest changes to the privatekey branch that
> contains two things.
> 
> First, I did some code cleanup which helped me get more familiar with
> the code base.
> 
> Second, I'm mostly done with the private RSS feed changes as per
> feature request # *3164013*.
> 
> What's still pending:
> 1) make either an update utility or change the tables.sql to include
> some code that auto populates the new user column with a unique md5()
> value.
The update utility is something I'd like to see ([1], [2]), but it's
not as easy as one might think :)
For now, please add the upgrade SQL statements to doc/UPGRADE.txt
and make sure that people without a private key in the database cannot
use the private bookmark feature.

> 2) I've only changed the bookmarks.php page to show the
> additional RSS feed. Before I propagate that change to the other
> pages with rss feeds, I'd like to specifically ask for thoughts on
> distinguishing the difference between the feeds.  Is the hover-text
> enough, or do we need to have a different icon for the private feed
> (like the common blue one).
I'd actually remove the feed icon from the pager and only keep the
icons in the HTML head (<link rel="alternate"/>).

Did you run the unittests yet? I think you have to adjust them and add
tests for your new functionality. Beware that running them will empty
the database, so back it up first.



[1] http://cweiske.de/tagebuch/php-installer-first.htm
[2]
http://cweiske.de/tagebuch/Generic%20PHP%20application%20installers.htm

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

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

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 ≡=-

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

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

Hello Mark,



> Ok, pushed some additional updates.  I'm authenticating using the
> user name and the private key.  Sounds quite reasonable.  I didn't
> want to monkey with the login() function so I copied it and  simply
> altered it to authenticate the user.  I haven't completed the unit
> testing for this yet, but will do so this week as time permits.  If
> you look at www/rss.php, you'll notice I log the user out.
> 
> At your leisure, please let me know if this is closer to what you were
> wanting.

Yes, the login code looks better now.

I'm still pondering what to do with the javascript regeneration.
Downside of your JS code is that it works only with JavaScript while
most of SC works fine without JS. Also, you do not have any error
handling on the javascript side yet if the key regeneration fails. This
is what I hate/fear most when doing js things; it's most time not easier
to do than the "normal" way if you do it correctly.

And yes, I meant that the private RSS feed on the main page is missing.

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

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