database bugs: wrong table name and failure to escape single quotes

[kyle.james.oconnor@…]

I just installed your plugin using easy_install from the trunk and I got warnings about non-existant table 'ad_cache' because it has been changed to 'dir_cache'. Also, you aren't using prepared statements for your SQL commands and therefore you have not escaped necessary characters such as single quote. Since this is an AD plugin and names (like mine) can have apostrophes, it needs a fix. Otherwise, very nice plugin.

I attached a patch where I fixed these things quick and dirty in my environment.

[kyle.james.oconnor@…]

q&d patch to fix my reported issues

[Ryan J Ollos]

Looks good! If I may recommend some Trac Fu here ... I think that string formatting should not be used for preparing the SQL statement. See ​t:TracDev/DatabaseApi#Parameterpassing for details.

cur.execute("INSERT OR REPLACE INTO session_attribute (sid, authenticated, name, value) VALUES ('%s', 1, 'name', '%s')" % (uname, to_unicode(displayname.replace("'","''"))))

->

cur.execute("""
    INSERT OR REPLACE INTO session_attribute (sid, authenticated, name, value)
    VALUES ('%s', 1, 'name', '%s')
    """,
    (uname, to_unicode(displayname)))

That change is untested since I have no access to or any experience with Active Directory. I think you probably don't need single quotes around the two format parameters (%s).

Could you try improving your patch in that way? The other part of your patch definitely needs to be applied since the plugin appears to be totally broken. If sandinak is not available, I'll apply the patch provided we can properly fix that SQL statement.

[kyle.james.oconnor@…]

I realized this is actually two bugs but it was late and I was too lazy to open two tickets. I figured Trac had a better way to do the SQL statements besides string formatting. Unfortunately, the plugin is riddled with them. I'll fix the one in question but maybe we can fork off a task to go back and fix the rest at a later date. I made your suggested changes and tested it. It's working for me. Also, I'm not sure if you want to include it but I'm a big fan of bumping the version number for every little change in something like a plugin so that's still included in the patch. Thanks!

[kyle.james.oconnor@…]

improved patch

[Ryan J Ollos]

Great, thanks for revising the patch. I will commit it this evening or tomorrow morning, and if you would kindly test the latest trunk just to make sure it's working correctly after the commit, I would appreciate it.

If you decide to fix the other SQL issues now or later on, I'd be happy to continue helping by reviewing and pushing the changes.

[Ryan J Ollos]

Replying to kyle.james.oconnor@gmail.com:

[...] Also, I'm not sure if you want to include it but I'm a big fan of bumping the version number for every little change in something like a plugin so that's still included in the patch.

I'll make sure to bump the version number as well.

[Ryan J Ollos]

(In [12961]) Fixes #11007:

• Fixed incorrect imports.
• Replaced string formatting in SQL statement with proper argument passing to the DB API.
• Fixed multiple PEP8 violations.

Patch by kyle.james.oconnor@….

[Ryan J Ollos]

(In [12962]) Refs #11007: Fixed inconsistent indentation.

[Ryan J Ollos]

The file had a combination of 4-space and 2-space indentation, which I attempted to fix in [12962]. This definitely needs a bit of testing. Please report back if you can. Thanks!

[kyle.james.oconnor@…]

Looks good, but I found a orphaned quote causing problems. See 11007.129632.fix.diff (attached)

[Ryan J Ollos]

(In [12965]) Refs #11007: Remove orphaned quote (error in [12961]). Thanks to kyle.james.oconnor@… for the patch.

[Ryan J Ollos]

Replying to kyle.james.oconnor@gmail.com:

Looks good, but I found a orphaned quote causing problems. See 11007.129632.fix.diff (attached)

Thanks for testing and reporting that. I knew that I wouldn't be lucky enough to change so many lines without testing and leaving at least one error!

[Ryan J Ollos]

Most of the changes from [12961], [12962] and [12965] were reverted by the plugin author in [12971] and [12972], so the issue persists following r12972.

[Ryan J Ollos]

ad_cache table name used in queries replaced by dir_cache again in #11495.

[Ryan J Ollos]

These issues should be fixed in [14503] and [14694], however more testing is needed.