kevinoconnor7 / osTicket-auth-cas

JASIG CAS Authentication plugin for osTicket
GNU General Public License v2.0
13 stars 8 forks source link

String or Array when setting email #19

Open dpenezic opened 5 years ago

dpenezic commented 5 years ago

Hi, I found issue when LDAP/SSO return multi value for email attribute.

osTicket error is like follow :

DB Error #1054

[SELECT A1.* FROM `ost_staff` A1 WHERE (A1.`0` = 'pero@pero.hr' AND A1.`1` = 'mali@mali.hr')] Unknown column 'A1.0' in 'where clause'

 ---- Povratno praćenje ----
 #0 (root)/include/mysqli.php(204): osTicket->logDBError('DB Error #1054',  '[SELECT A1.* FR...')
 #1 (root)/include/class.orm.php(3133): db_query('SELECT A1.* FRO...',  true,  true)
 #2 (root)/include/class.orm.php(3180): MySqlExecutor->execute()
 #3 (root)/include/class.orm.php(1771): MySqlExecutor->getArray()
 #4 (root)/include/class.orm.php(1815): ModelInstanceManager->{closure}()
 #5 (root)/include/class.orm.php(1794): CallbackSimpleIterator->next()
 #6 (root)/include/class.orm.php(1803): CallbackSimpleIterator->rewind()
 #7 (root)/include/class.orm.php(1463): CallbackSimpleIterator->valid()
 #8 (root)/include/class.orm.php(1480): CachedResultSet->fillTo(9223372036854775807)
 #9 (root)/include/class.orm.php(1149): CachedResultSet->asArray()
 #10 (root)/include/class.orm.php(1172): QuerySet->all()
 #11 (root)/include/class.orm.php(545): QuerySet->one()
 #12 (root)/include/class.staff.php(768): VerySimpleModel::lookup(Array)
 #13 (root)/include/class.usersession.php(169): Staff::lookup(Array)
 #14 phar://(root)/include/plugins/auth-cas.phar/cas.php(148): StaffSession::lookup(Array)
 #15 (root)/include/class.auth.php(276): CasStaffAuthBackend->signOn()
 #16 (root)/scp/login.php(68): AuthenticationBackend::processSignOn(Array,  false)
 #17 {main}

osTicket expect string for email, but dont test if array is present instead. I may submit patch for osTicket-auth-cas , but am not familiar with osTicket policy for external authentication. Do external authentication plugin must check that return value sre string or not ?

kevinoconnor7 commented 5 years ago

Ah interesting. I hadn't considered this.

So we end up calling lookup [1] with an array rather than a string. It looks like method just passes arrays to the parent method and it gets handled by the ORM, resulting the broken query.

What needs to happen is either:

  1. We explicitly select one e-mail address, however I'm not sure the best method for choosing one.
  2. Manually construct the array to pass to lookup such that we end up doing something like email IN ($emails).

For the second solution, the ORM does expose some operations, like IN, but it's unclear how this is supposed to be structured [2]. I believe we want something like: array('email__in' => $emails), but further experimentation is needed.

[1] https://github.com/osTicket/osTicket/blob/19712cd86c6cf19aa524af3c0201074810fe32bd/include/class.staff.php#L765 [2] https://github.com/osTicket/osTicket/blob/19712cd86c6cf19aa524af3c0201074810fe32bd/include/class.orm.php#L2019

dpenezic commented 5 years ago

Hi, it isnt so simple with multi-value LDAP attribute, for some reason many LDAP client treat multi-value LDAP attribute on follow way :

So I will suggest that both field email and name be aware of multi-value nature.

How decide what to put in osTicket is something other, I will talk tomorrow morning with my colleague who is in process of implementing osTicket on our institution, from my pragmatic point of view for email all value need to be test against osTicket DB , and for name longest of value(s) received.

We are more then happy to test new code, when will be available.

kevinoconnor7 commented 5 years ago

I'll try to look into getting something working this week. No promises though as I really only work on this project when my free time allows.

kevinoconnor7 commented 5 years ago

I added a multiple_emails branch that adds this functionality. Can you test it out for me?

dpenezic commented 5 years ago

Hi, I will do that tomorrow morninig, and send you info . According source diff I think that may work correctly.

dpenezic commented 5 years ago

I added a multiple_emails branch that adds this functionality. Can you test it out for me?

Hi we did test and everything work O.K. . What about Name ?