Re: [Semanticscuttle-devel] Privatekey
Brought to you by:
cweiske
Menu
▾
▴
Re: [Semanticscuttle-devel] Privatekey
|
From: Christian W. <cw...@cw...> - 2011-03-21 07:41:40
|
Hello Mark, > Ok Christian, I've backed out the bookmark query updates, and removed > the recently added User column, added a number of new tests, and > addressed most (if not all) of your concerns). I'm still a bit green > on unit testing, so be easy on me. :) Cheers to all. Have a great > weekend! > >>> I've got a little special configuration on my dev system: > >>> > $sitename = 'b"m.bo\'go'; > >>> The site name here contains both single and double quotes, > >>> something that helped me discovering quite a number of > >>> quoting/escaping bugs. It seems that you double-htmlencode the > >>> title in the rss feed so that I get > >>> > title="b&quot;m.bo'go: (private) b&quot;m.bo'go" > >>> instead of the > >>> > title="b"m.bo'go: Recent bookmarks" > >>> as it is for the normal feed in the HTML head. The problem still exists for me. Seems I did not describe it properly since you did remove the escaping in rss.tpl.php which needs to stay there. You don't double-encode it in the rss feed but in the rss feed link on the html main page: > <link rel="alternate" type="application/rss+xml" > title="b"m.bo'go: Recent bookmarks" > href="http://bm.bogo/rss.php?sort=date_desc" /> > <link rel="alternate" type="application/rss+xml" > title="b&quot;m.bo'go: (private) b&quot;m.bo'go" > href="http://bm.bogo/rss.php/test?sort=date_desc&privatekey=-e36a17f2717a3da7321aa742e328e92" /> It's already fixed on the main page. On the user bookmark page you unfortunately did not encode the & in the URL, making the page invalid. I either spot these things manually or use the W3C HTML Validator. With the Opera browser, you just have to right-click a page and "Validate", on firefox you may want to install the web developer toolbar - it has a "validate local html" button on the "extras" menu. You also removed the htmlspecialchars on the opensearchdescription title in top.inc.php - that needs to be kept, too. > >>> There is no fail message when an unknown privatekey is used. > >>> SemanticScuttle should send a status 400 (Bad request) with an > >>> explanatory message. I see that a 404 is sent, but I don't see any error message in the browser. This is due to the fact that the error is sent as HTML page, but the Content-Type in the headers is application/rss+xml: $ curl -i 'http://bm.bogo/rss.php/test?sort=date_desc&privatekey=foobar' HTTP/1.1 404 Not Found Date: Mon, 21 Mar 2011 06:46:04 GMT .... Content-Type: application/rss+xml; charset=utf-8 <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd"> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en"> One thing I noted on the profile page: When I uncheck the "enable" checkbox, I get a new key of "-e36a17f2717a3da7321aa742e328e92" the the checkbox still enabled. Also, the "enabled" text should be surrounded with a <label for="checkbox-id"> tag so that the user can click the text and does not have to aim on the checkbox itself. I just see that this is due to the SQL change that I did not apply yet on my database! The attribute "readonly" is ok, but please make it XHTML-compatible by using readonly="readonly". About this: > + * @param string $privateKey RSS Private Key > + * @param string $enablePrivateRSS RSS Private Key From that signature, I don't understand why you need two parameters here since they have the same type and the same description. Plus, the type is string but the default value in the method signature is still 0 :) Use a boolean type here and keep the original name of "enablePrivateKey", since we it does not have to be limited to the RSS feeds in the future. And I see that we misunderstood each other; you want to keep the original key and be able to re-enable the key in the profile while being able to restore the old one. I thought that setting it to NULL or empty string would suffice, but your idea of prepending a "-" is actually better. I like that you hide the implementation details of deactivating/activating the private key in the updateuser method by providing a "enableprivatekey" parameter. Unfortunately, you don't hide the details when checking for it and always rely on the 32 characters length: > www/index.php > + if ($currentUser->getPrivateKey() <> null && > strlen($currentUser->getPrivateKey()) == 32) { It'd be cool if you implemented and used $currentUser->isPrivateKeyAllowed() there. The API tests look really good. What I miss is tests that query the actual feed URLs with parameters; see the API tests that do ->getRequest()->send(). I'm beginning to see the finish line here now :) -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
