Modify

Opened 21 months ago

Last modified 11 months ago

#11007 new defect

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

Reported by: kyle.james.oconnor@… Owned by: sandinak
Priority: normal Component: DirectoryAuthPlugin
Severity: normal Keywords:
Cc: Trac Release: 1.0

Description

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.

Attachments (3)

database_bugs.diff (2.5 KB) - added by kyle.james.oconnor@… 21 months ago.
q&d patch to fix my reported issues
database_bugs.2.diff (2.5 KB) - added by kyle.james.oconnor@… 21 months ago.
improved patch
11007.12962.fix.diff (613 bytes) - added by kyle.james.oconnor@… 21 months ago.

Download all attachments as: .zip

Change History (16)

Changed 21 months ago by kyle.james.oconnor@…

q&d patch to fix my reported issues

comment:1 Changed 21 months ago by rjollos

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.

comment:2 follow-up: Changed 21 months ago by 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!

Changed 21 months ago by kyle.james.oconnor@…

improved patch

comment:3 Changed 21 months ago by rjollos

  • Owner changed from sandinak to rjollos
  • Status changed from new to assigned

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.

comment:4 in reply to: ↑ 2 Changed 21 months ago by rjollos

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.

comment:5 Changed 21 months ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

(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@….

comment:6 Changed 21 months ago by rjollos

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

comment:7 Changed 21 months ago by rjollos

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!

comment:8 follow-up: Changed 21 months ago by kyle.james.oconnor@…

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

Changed 21 months ago by kyle.james.oconnor@…

comment:9 Changed 21 months ago by rjollos

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

comment:10 in reply to: ↑ 8 Changed 21 months ago by rjollos

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!

comment:11 Changed 20 months ago by rjollos

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:12 Changed 20 months ago by rjollos

  • Owner changed from rjollos to sandinak
  • Status changed from reopened to new

comment:13 Changed 11 months ago by rjollos

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

Last edited 11 months ago by rjollos (previous) (diff)

Add Comment

Modify Ticket

Action
as new The owner will remain sandinak.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.