Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String or Array when setting email #19

Open
dpenezic opened this issue Oct 22, 2018 · 6 comments
Open

String or Array when setting email #19

dpenezic opened this issue Oct 22, 2018 · 6 comments

Comments

@dpenezic
Copy link

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` = '[email protected]' AND A1.`1` = '[email protected]')] 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
Copy link
Owner

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
Copy link
Author

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

  • only one value present in LDAP - return string

  • more then one value present in LDAP - return array

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
Copy link
Owner

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
Copy link
Owner

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

@dpenezic
Copy link
Author

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

@dpenezic
Copy link
Author

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants