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

Conversation

@plettich
Copy link
Member

commented Aug 28, 2019

privacyIDEA works with a user ID type string so we enforce that the
getUserId()-function only returns strings.

Fixes #1825
Also fixes #862 (yay!)

The getUserId() function returns only strings
privacyIDEA works with a user ID type string so we enforce that the
`getUserId()`-function only returns strings.

Fixes #1825
Also fixes #862 (yay!)

@plettich plettich requested a review from fredreichbier Aug 28, 2019

@plettich plettich added this to In progress in privacyIDEA 3.2 via automation Aug 28, 2019

@plettich plettich self-assigned this Aug 28, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 28, 2019

Codecov Report

Merging #1829 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1829      +/-   ##
=========================================
+ Coverage   97.09%   97.2%   +0.11%     
=========================================
  Files         149     149              
  Lines       18264   19206     +942     
=========================================
+ Hits        17733   18669     +936     
- Misses        531     537       +6
Impacted Files Coverage Δ
privacyidea/lib/resolvers/UserIdResolver.py 100% <ø> (ø) ⬆️
privacyidea/lib/resolvers/PasswdIdResolver.py 99.13% <100%> (ø) ⬆️
privacyidea/lib/resolvers/LDAPIdResolver.py 93.42% <100%> (-0.02%) ⬇️
privacyidea/lib/resolvers/SQLIdResolver.py 98.03% <100%> (ø) ⬆️
privacyidea/lib/resolvers/SCIMIdResolver.py 100% <100%> (ø) ⬆️
privacyidea/lib/decorators.py 100% <0%> (ø) ⬆️
privacyidea/lib/event.py 99.1% <0%> (+0.07%) ⬆️
privacyidea/models.py 99.07% <0%> (+0.14%) ⬆️
privacyidea/lib/config.py 95.79% <0%> (+0.36%) ⬆️
privacyidea/lib/token.py 96.9% <0%> (+0.86%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c90807...61743ea. Read the comment docs.

@fredreichbier
Copy link
Member

left a comment

I only have some comments regarding python 2.7 compatibility

@@ -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

Member

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

@@ -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

Member

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

@@ -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

Member

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

@@ -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

Member

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

@@ -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

Member

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

privacyIDEA 3.2 automation moved this from In progress to Review in progress Aug 28, 2019

privacyIDEA 3.2 automation moved this from Review in progress to Reviewer approved Aug 29, 2019

@fredreichbier fredreichbier merged commit f1543ce into master Aug 29, 2019

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 97.09%)
Details
codecov/project 97.2% (+0.11%) compared to 2c90807
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

privacyIDEA 3.2 automation moved this from Reviewer approved to Done Aug 29, 2019

@fredreichbier fredreichbier deleted the 1825/enforce_uid_type_str branch Sep 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.