From: Tim A. <tna...@sh...> - 2009-06-27 02:41:43
|
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#ffffff" text="#000000"> <ol> <li>Regarding changing the table name and no update path. My thinking was that we had guaranteed an upgrade path between beta 1 and beta 2, we had said nothing about the in between steps. I'll be more careful in the future.</li> <li>I'm now confused: in your pt 2, you talk about needing view full permission in order to access the full image to print, otherwise there is this security whole. Then we talk about using the print_proxy if you don't have access to view the full size image. To me these two requirements seem mutually exclusive (i.e. there is no need for a print proxy).<br> My view of how this should work, is that the admin (me) wants to remove view full size permissions from everybody, but I want to allow a visitor to print an image if they like it. so I want them to press the print button and they enter their order in digibug. So in my scenario they don't have access to the full size image, but Digibug via the print proxy has access. How do we address your security concern but leave this scenario in place.</li> <li>As for the admin page, I figure, its there, we have other priorities lets declare a victory and move on.</li> <li>And I had a task to clean up expired entries, but I refactored that to be done when an image is requested via the proxy. Again, I think that not using the proxy adds a complication and adds nothing to the functionality. Again, lets declare a victory and move on.<br> </li> </ol> <br> Bharat Mediratta wrote: <blockquote cite="mid:4A4...@me..." type="cite">Tim Almdal wrote: <br> <blockquote type="cite">Here's what's left: <br> <br> 1. "we only need to use the proxy for photos where you don't have <br> view_full privs" <br> </blockquote> > <br> <blockquote type="cite"> I think its risky to do that. I'd prefer to leave the proxy for <br> everything. One its less code (don't have to check about <br> permissions). Ok, its only a couple of lines. Secondly, there is <br> a time delay between order placement and order fulfillment <br> permissions could have been removed. <br> </blockquote> <br> Yes, but then you're also overloading the proxy table which might require you to have a task to do cleanup, etc. If you use the public url, there are less moving parts for most operations because the remote server is just dealing with publicly available files. Removing permissions during the time delay is an extreme edge case here. The time delay is going to approach zero in most cases. <br> <br> <blockquote type="cite"> 2. "there's no security in print_photo() -- I can use it to get access <br> to full size photos even if I can't view them" <br> </blockquote> > <br> <blockquote type="cite"> I guess, if you know the url you can, I'm open to suggestions <br> regarding closing that hole. In some cases that is point of this. <br> On my gallery I will probably remove view fullsize from every one <br> but allow them to print it which requires access to the fullsize <br> behind the scenes. <br> </blockquote> <br> Digibug_Controller::print_photo() takes an item id and creates a proxy url for it, then returns the proxy form. This means that I can go to your website, guess and id (say, 35) and create a url like: <br> <br> <a class="moz-txt-link-freetext" href="http://example.com/gallery3/print_photo/35">http://example.com/gallery3/print_photo/35</a> <br> <br> This will give me a url that lets me view the full size. No permission checks have been run. This is a security hole. print_photo needs to verify that the caller can view the full size before it creates a proxy for it. <br> <br> Are you confusing print_photo() and print_proxy() here? print_proxy() requires the $uuid (might help if you renamed the variables in the method definition to make that more clear). <br> <br> <br> <blockquote type="cite"> 3. "I'd simplify all of this and ditch the advanced mode altogether. It's way too much choice and text for G3. Plus all the marketing <br> stuff on the settings page.. that all looks like G2." <br> </blockquote> > <br> <blockquote type="cite"> I've cut out most of the cruft so its a lot lighter and not so g2 <br> like. I'd prefer being able to enter my own company and event id, <br> so I'd rather not have to go to advanced settings. In fact, if we <br> weren't time constrained I would have implemented the patch I had <br> created for g2 to allow event ids to be assigned to groups and users. </blockquote> <br> You're an edge case. Don't optimize for yourself here. Right now if I go to that page I'm mightily confused. I'm presented with a signup page and a couple of empty text boxes. I don't care about any of that stuff-- I just want to allow people to make prints. Start with the simplest possible thing-- hardcode the values and don't make them editable. The point in making Admin > Settings > Advanced is so that we *don't* have to create a confusing UI just to support our power users. More work needed here. <br> <br> <blockquote type="cite"> 4. "ditch basic_company_id and basic_event_id and just have the <br> company_id and event_id prefilled. Don't even allow users to <br> change them for now (advanced users can always change those values <br> in Admin > Settings > Advanced later on. Perhaps add some simple <br> text and a link to the Digibug site and their logo" <br> </blockquote> > <br> <blockquote type="cite"> I like having the basic settings, so an admin can switch back and <br> forth. I guess what I could do (and maybe this what your saying) <br> is bag the tabs, just have the content that's on the advanced and <br> add a "default" button to reset to gallery's company and event ids <br> </blockquote> <br> More of what I said above. This is still too confusing. Let's start by supporting our 80% case. Those users want to activate the module, do zero configuration, and have print buttons appear. Realistically, we could just ditch the Digibug settings page altogether for 3.0 and put up a FAQ telling people how to sign up and change their ids and. <br> <br> <blockquote type="cite"> 6. Just thought of this... how do you want to handle non jpeg <br> images. The Digibug api will only handle jpegs. Not show the <br> button if its not a jpeg? (simple and quick) or do a conversion <br> (more complete, but a pita to manage the converted image). I'm <br> leaning to the first option. <br> </blockquote> <br> I like #1. <br> <br> <blockquote type="cite"> 7. Will try to put the digibug window into an iframe in a dialog and <br> see how that works (Chad's suggestion) <br> </blockquote> <br> I just tried printing and got this error: <br> <br> There was an SQL error: Table 'g3dev.g3_digibug_proxies' doesn't exist - SHOW COLUMNS FROM `g3_digibug_proxies` <br> <br> I haven't looked at the code, but I'm guessing this means that you modified the database but didn't add upgrade support? I guess we don't have a policy for what we do in that case, but now that we're post-beta we should do our best to support upgrades for any changes that we make. <br> <br> I think this means that we don't commit stuff back to the main repo until it's passed some level of review, and once its in the main repo we support the proper upgrade path. <br> <br> I doubt many people have been bitten by this so far, so I think the easiest thing to do is to seek forgiveness in this case, but in the future we should probably keep modules out of the main repo until you and I agree on the structure first to avoid writinga lot of upgrade code. <br> <br> -Bharat <br> <br> PS: for anybody playing along at home, you might want to drop the "proxies" table from your g3dev if you installed the digibug module yesterday. <br> <pre wrap=""> <hr size="4" width="90%"> No virus found in this incoming message. Checked by AVG - <a class="moz-txt-link-abbreviated" href="http://www.avg.com">www.avg.com</a> Version: 8.5.339 / Virus Database: 270.12.91/2201 - Release Date: 06/25/09 06:22:00 </pre> </blockquote> </body> </html> |