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

The getUserId() function returns only strings #1829

Merged
merged 2 commits into from Aug 29, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -60,12 +60,13 @@
import yaml
import threading
import functools
import six

from .UserIdResolver import UserIdResolver

import ldap3
from ldap3 import MODIFY_REPLACE, MODIFY_ADD, MODIFY_DELETE
from ldap3 import Server, Tls, Connection
from ldap3 import Tls
from ldap3.core.exceptions import LDAPOperationResult
from ldap3.core.results import RESULT_SIZE_LIMIT_EXCEEDED
import ssl
@@ -76,7 +77,6 @@
from passlib.hash import ldap_salted_sha1
import hashlib
import binascii
from privacyidea.lib.crypto import urandom, geturandom
from privacyidea.lib.utils import is_true
from privacyidea.lib.framework import get_app_local_store
import datetime
@@ -87,7 +87,6 @@
import uuid
from ldap3.utils.conv import escape_bytes
from operator import itemgetter
from six import string_types

CACHE = {}

@@ -539,7 +538,7 @@ def _ldap_attributes_to_user_object(self, attributes):
if ldap_k == map_v:
if ldap_k == "objectGUID":
# An objectGUID should be no list, since it is unique
if isinstance(ldap_v, string_types):
if isinstance(ldap_v, six.string_types):
ret[map_k] = ldap_v.strip("{").strip("}")
else:
raise Exception("The LDAP returns an objectGUID, that is no string: {0!s}".format(type(ldap_v)))
@@ -573,6 +572,7 @@ def getUserId(self, LoginName):
:param LoginName: The login name from the credentials
:type LoginName: string
:return: UserId as found for the LoginName
:rtype: str
"""
userid = ""
self._bind()
@@ -626,11 +626,11 @@ def getUserId(self, LoginName):
LoginName))

for entry in r:
userid = self._get_uid(entry, self.uidtype)
userid = str(self._get_uid(entry, self.uidtype))

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier Aug 28, 2019

Contributor

I guess we need to use six.text_type here (or to_unicode?). If we use str(), this breaks under Python 2.7 with uids that contain non-ASCII characters (like dn with an umlaut):

Traceback (most recent call last):
  File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 2446, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1951, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1820, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1947, in full_dispatch_request
    rv = self.preprocess_request()
  File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 2241, in preprocess_request
    rv = func()
  File "/home/fred/privacyidea/privacyidea/privacyidea/api/auth.py", line 350, in decorated_function
    return f(*args, **kwargs)
  File "/home/fred/privacyidea/privacyidea/privacyidea/api/before_after.py", line 99, in before_user_request
    before_request()
  File "/home/fred/privacyidea/privacyidea/privacyidea/api/before_after.py", line 143, in before_request
    request.User = get_user_from_param(request.all_data)
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/log.py", line 154, in log_wrapper
    return func(*args, **kwds)
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/user.py", line 582, in get_user_from_param
    resolver=param.get("resolver"))
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/log.py", line 154, in log_wrapper
    return func(*args, **kwds)
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/user.py", line 98, in __init__
    self._get_user_from_userstore()
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/usercache.py", line 63, in cache_wrapper
    return wrapped_function(*args, **kwds)
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/user.py", line 106, in _get_user_from_userstore
    self._get_resolvers()
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/user.py", line 207, in _get_resolvers
    if self._locate_user_in_resolver(resolvername):
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/user.py", line 227, in _locate_user_in_resolver
    uid = y.getUserId(self.login)
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/resolvers/LDAPIdResolver.py", line 258, in cache_wrapper
    f_result = func(self, *args, **kwds)
  File "/home/fred/privacyidea/privacyidea/privacyidea/lib/resolvers/LDAPIdResolver.py", line 629, in getUserId
    userid = str(self._get_uid(entry, self.uidtype))
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf6' in position 14: ordinal not in range(128)

This comment has been minimized.

Copy link
@plettich

plettich Aug 29, 2019

Author Member

Since we do the unicode conversion in LDAP-Resolver now, this should be fixed in 61743ea

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier Aug 28, 2019

Contributor

Also, what do you think about moving the stringification into _get_uid?

This comment has been minimized.

Copy link
@plettich

plettich Aug 29, 2019

Author Member

Done in 61743ea


return userid

def getUserList(self, searchDict):
def getUserList(self, searchDict=None):
"""
:param searchDict: A dictionary with search parameters
:type searchDict: dict
@@ -253,7 +253,7 @@ def getUsername(self, userId):
:param userid: The userid in this resolver
:type userid: string
:return: username
:rtype: string
:rtype: str
'''
fields = self.descDict.get(userId)
index = self.sF["username"]
@@ -269,7 +269,7 @@ def getUserId(self, LoginName):
# We do not encode the LoginName anymore, as we are
# storing unicode in nameDict now.
if LoginName in self.nameDict:
return self.nameDict[LoginName]
return str(self.nameDict[LoginName])

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier Aug 28, 2019

Contributor

I think we have the same problem here with Python 2.7, we should use six.text_type.

This comment has been minimized.

Copy link
@plettich

plettich Aug 29, 2019

Author Member

Fixed in 61743ea

else:
return ""

@@ -293,7 +293,7 @@ def getSearchFields(self, searchDict=None):

return self.searchFields

def getUserList(self, searchDict):
def getUserList(self, searchDict=None):
"""
get a list of all users matching the search criteria of the searchdict
@@ -133,7 +133,7 @@ def getUserId(self, loginName):
# ("userName", loginName)})
#return res.get("Resources", [{}])[0].get("externalId")
# It seems that the userName is the userId
return loginName
return str(loginName)

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier Aug 28, 2019

Contributor

I haven't tested it, but this may also break under Python 2.7 -- we should use text_type instead.

This comment has been minimized.

Copy link
@plettich

plettich Aug 29, 2019

Author Member

Fixed in 61743ea


def getUserList(self, searchDict=None):
"""
@@ -34,12 +34,13 @@
import yaml
import binascii
import re
import six

from privacyidea.lib.resolvers.UserIdResolver import UserIdResolver

from sqlalchemy import and_
from sqlalchemy import create_engine
from sqlalchemy import Integer
from sqlalchemy import Integer, cast, String
from sqlalchemy.orm import sessionmaker, scoped_session

import traceback
@@ -356,9 +357,11 @@ def getUserInfo(self, userId):
def _get_userid_filter(self, userId):
column = getattr(self.TABLE, self.map.get("userid"))
if isinstance(column.type, Integer):
return column == userId
# since our user ID is usually a string we need to cast
return column == int(userId)
else:
return column.like(userId)
# otherwise we cast the column to string (in case of postgres UUIDs)
return cast(column, String).like(userId)

def getUsername(self, userId):
"""
@@ -378,6 +381,7 @@ def getUserId(self, LoginName):
:param LoginName: The login name from the credentials
:type LoginName: string
:return: UserId as found for the LoginName
:rtype: str
"""
userid = ""

@@ -395,7 +399,7 @@ def getUserId(self, LoginName):
raise Exception("More than one user with loginname"
" %s found!" % LoginName)
user = self._get_user_from_mapped_object(r)
userid = user["id"]
userid = str(user["id"])

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier Aug 28, 2019

Contributor

Also here we might have the same problem, I guess six.text_type would be safer

This comment has been minimized.

Copy link
@plettich

plettich Aug 29, 2019

Author Member

Fixed in 61743ea

except Exception as exx: # pragma: no cover
log.error("Could not get the userinformation: {0!r}".format(exx))

@@ -128,7 +128,7 @@ def getUserId(self, loginName):
:param loginName: The login name of the user
:type loginName: sting
:return: The ID of the user
:rtype: string or int
:rtype: str
"""
return "dummy_user_id"

@@ -182,7 +182,7 @@ def test_01_sqlite_resolver(self):

user = "cornelius"
user_id = y.getUserId(user)
self.assertTrue(user_id == 3, user_id)
self.assertEqual(user_id, '3', user_id)

rid = y.getResolverId()
self.assertTrue(rid.startswith("sql."))
@@ -296,8 +296,9 @@ def test_05_add_user_update_delete(self):
stored_password = y.TABLE.filter_by(username="achmed").first().password
self.assertTrue(stored_password.startswith("{SSHA256}"), stored_password)

# we assume here the uid is of type int
uid = y.getUserId("achmed")
self.assertTrue(uid > self.num_users)
self.assertGreater(int(uid), self.num_users)

r = y.update_user(uid, {"username": "achmed2",
"password": "test"})
@@ -440,7 +440,7 @@ def test_12_multiple_resolvers(self):
# Now, query the user and populate the cache
self.assertEqual(UserCache.query.count(), 0)
user1 = User('wordpressuser', self.sql_realm)
self.assertEqual(user1.uid, 6)
self.assertEqual(user1.uid, '6')
# Assert it was found in reso_b (as it does not have a phone number)!
self.assertEqual(user1.resolver, 'reso_b')
self.assertEqual(UserCache.query.filter(UserCache.username == 'wordpressuser',
@@ -458,7 +458,7 @@ def test_12_multiple_resolvers(self):
'reso_b')
# Now, it should be located in reso_a!
user2 = User('wordpressuser', self.sql_realm)
self.assertEqual(user2.uid, 6)
self.assertEqual(user2.uid, '6')
self.assertEqual(user2.resolver, 'reso_a')
# ... but the cache still contains entries for both!
resolver_query = UserCache.query.filter(UserCache.username == 'wordpressuser',
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.