when geni-ar can't talk to the database, LDAP actions can still be taken

[ahelsing]

If geni-ar is successfully talking to LDAP but not successfully talking to the postgresql database, it is still possible to take account actions, which are then not audited.

For instance, from display_accounts.php, it is possible to delete an account without postgresql configured correctly. The delete succeeds, but is not logged. This is bad, because it means that account modifications can happen without auditing if the system is in a bad state, and i think it would be easy to improve at least somewhat.

Imported from trac ticket #22, created by chaos on 09-08-2013 at 16:01, last modified: 09-13-2013 at 16:07

[ahelsing]

A couple of suggestions --- maybe pick one that y'all like philosophically, and that's easy:

• Database select errors like:

[Sun Sep 08 16:01:04 2013] [error] [client 128.89.254.98] DB ERROR: "connect failed" for query "SELECT * FROM idp_account_request WHERE request_state='DELETED'", referer: https://shib-idp1.gpolab.bbn.com/manage/acct_actions.php

could be fatal --- that error shows up in the apache error log when i browse to [https://shib-idp1.gpolab.bbn.com/manage/display_accounts.php], but it doesn't prevent me from loading that page or taking actions. It could.

• Modify actions could write to the log before'' modifying LDAP, and refuse to modify LDAP if the log write failed. (Of course, then you have the problem in which if the LDAP action fails, you need some way to get a record of ''that.)
• If a database modify fails, the attempted SQL statement could be logged in the error log, so someone could look up what had happened at least until the logs got rotated. Right now, when i delete my LDAP account without postgresql hooked up, [https://shib-idp1.gpolab.bbn.com/manage/acct_actions.php] shows me:

COULD NOT CHANGE STATUS OF REQUEST FOR DELETED ACCOUNT WITH USERNAME=chaos

but that failure doesn't get logged at all as far as i can tell.

Trac comment by chaos on 09-08-2013 at 16:05

[ahelsing]

For approve/deny/delete, log first. If log fails, do not perform action. If log succeeds and action fails, add FAILED comment to log For all errors, print message in tool and in apache error log

Trac comment by phelinek on 09-09-2013 at 16:26

[ahelsing]

I'm still seeing this issue using the geni-ar-0.2 package:

• On aslund (shib-idp1):

aslund,[~],14:05(0)$ dpkg -l geni-ar
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Cfg-files/Unpacked/Failed-cfg/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name           Version        Description
+++-==============-==============-============================================
ii  geni-ar        0.2            GENI account request management
aslund,[~],14:08(0)$ 

• The accreq database does not yet exist on aslund
• Verifying that my LDAP account exists:

aslund,[~],14:08(0)$ sudo slapcat -F /etc/ldap/slapd.d | grep uid=chaos
dn: uid=chaos,ou=people,dc=shib-idp1,dc=gpolab,dc=bbn,dc=com
aslund,[~],14:08(0)$ 

• I browse to [https://shib-idp1.gpolab.bbn.com/manage/]
• I click "Manage Current Accounts":
  • The apache error log (/var/log/apache2/error.log) shows:

[Wed Sep 11 14:10:18 2013] [error] [client 128.89.73.13] DB ERROR: "connect failed" for query "SELECT * FROM idp_account_request WHERE request_state='DELETED'", referer: https://shib-idp1.gpolab.bbn.com/manage/
[Wed Sep 11 14:10:18 2013] [error] [client 128.89.73.13] PHP Warning:  Invalid argument supplied for foreach() in /usr/share/geni-ar/protected/display_accounts.php on line 94, referer: https://shib-idp1.gpolab.bbn.com/manage/

• I get taken to [https://shib-idp1.gpolab.bbn.com/manage/display_accounts.php], and can see a list of accounts, including the "chaos" account
  • In the row containing "chaos", i make sure the slider says "DELETE", and click "SUBMIT":
  • The apache error log shows:

[Wed Sep 11 14:11:51 2013] [error] [client 128.89.73.13] Failed to change request state for deleted account for chaos, referer: https://shib-idp1.gpolab.bbn.com/manage/display_accounts.php
[Wed Sep 11 14:11:51 2013] [error] [client 128.89.73.13] DB ERROR: "connect failed" for query "SELECT * FROM idp_account_request WHERE request_state='DELETED'", referer: https://shib-idp1.gpolab.bbn.com/manage/display_accounts.php
[Wed Sep 11 14:11:51 2013] [error] [client 128.89.73.13] PHP Warning:  Invalid argument supplied for foreach() in /usr/share/geni-ar/protected/display_accounts.php on line 94, referer: https://shib-idp1.gpolab.bbn.com/manage/display_accounts.php

• I get taken back to [https://shib-idp1.gpolab.bbn.com/manage/display_accounts.php]. The "chaos" account now does not appear on the page
• slapcat confirms that the account is gone:

aslund,[~],14:12(0)$ sudo slapcat -F /etc/ldap/slapd.d | grep uid=chaos
aslund,[~],14:13(1)$ 

Trac comment by chaos on 09-11-2013 at 14:13

[tcmitchell]

The function add_log() in browser:geni-ar/protected/log_actions.php isn't returning a value so the check in browser:geni-ar/protected/acct_actions.php can't work.

Trac comment by tmitchel (github user: tcmitchell) on 09-11-2013 at 14:18

[ahelsing]

That was a bug - I added the return code to add_log_with_comment() and add_comment() but not to add_log(). It should be fixed now, but I'll hold off closing the ticket until Chaos verifies in her test environment

Trac comment by phelinek on 09-11-2013 at 15:18

[ahelsing]

I've tested this pretty thoroughly, turning postgres off, also tested with ldap off. So I'm closing the ticket.

Trac comment by phelinek on 09-12-2013 at 18:26

[ahelsing]

I agree, this is now fixed. Thanks!

Trac comment by chaos on 09-13-2013 at 13:15