search

pressbooks / pressbooks-saml-sso

SAML2 Single Sign-On integration for Pressbooks (Shibboleth, ADFS, Google Apps, Etc.)
GNU General Public License v3.0
3 stars 1 forks source link

Improve logging #88

Closed SteelWagstaff closed 3 years ago

SteelWagstaff commented 3 years ago

We have a few problems with the logs:

  1. We're logging information a little too late: https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L368-L370 It would be good to log the contents of self::USER_DATA at the same time we store it to the $attributes variable so we can see what values we have from the claim even before we attempt to set the username and email address. We should have information in the log about the claim information received by Pressbooks in cases when login fails (maybe especially so in this cases). We should also switch the order of lines 369 and 368 so that we log net_id before we log email.
  2. We're unneccesarily duplicating some logging information Screenshot from 2021-09-23 22-00-06 Each successful login sends the same information twice, except once with some "Auth SAML data" and once without it. See https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L577
  3. The post authentication log already includes all of the information in the two earlier log statements. Not sure why we're producing and recording all three of them.

It would be good to allow network managers to download and inspect these logs from the SSO config page, as we allow with the OIDC plugin's logs.

Finally, logs do not appear to be automatically configured the plugin is activated or used in our production network. Not sure what needs to be done so that necessary S3 buckets or CloudWatch groups are created, but if the plugin is active on one of our hosted networks, logs should automatically be kept.

SteelWagstaff commented 3 years ago

Things we attempt to log:

  1. email, net_id (uid), SAML settings after authentication (if net_id exists): https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L368-L370
  2. Errors from SAML Auth, Last SAML Error Reason, during authentication if we have errors: https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L531-L532
  3. NameID of the assertion, NameID SP NameQualifier of the assertion (after authentication): https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L541-L542
  4. Auth SAML data (just after we storeAuthDataInSession): https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L577
  5. Cookies, Username matched, Session after logged [Matched] after a matching user if found and logged in: https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L752-L754
  6. User metadata stored (after a user has been linked to their SAML identity): https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L861
  7. Cookies, Username associated, Session after logged [Associated] after a new user if created and logged in: https://github.com/pressbooks/pressbooks-saml-sso/blob/6b887a3c62bbe8eb1cb16bec101cbe3cfddf0622/inc/class-saml.php#L971-L973

Things included in first log statement generated for each successful login attempt:

Things included in second log statement (generated at almost the same time as first statement)

Things included in third log statement: if user matched:

if new user created

SteelWagstaff commented 3 years ago

Logs look great for SamlTest on integrations. Will try to test with alternate IdP

SteelWagstaff commented 3 years ago

Client IdP I hoped to test with is down. Without ability to test further, will consider the issue resolved.