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

Make 'splitAtSign' more consistent #1905

Merged
merged 2 commits into from Oct 28, 2019

Conversation

@plettich
Copy link
Member

plettich commented Oct 25, 2019

The splitAtSign configuration did not affect the /auth endpoint thus
leading to inconsistencies.
This PR tries to mimic the user-resolving behaviour of the
/validate/check endpoint considering the splitAtSign configuration.

Fixes #1808

The `splitAtSign` configuration did not affect the `/auth` endpoint thus
leading to inconsistencies.
This PR tries to mimic the user-resolving behaviour of the
`/validate/check` endpoint considering the `splitAtSign` configuration.

Fixes #1808
@plettich plettich requested a review from cornelinux Oct 25, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #1905 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1905      +/-   ##
==========================================
+ Coverage   97.13%   97.13%   +<.01%     
==========================================
  Files         151      151              
  Lines       18475    18481       +6     
==========================================
+ Hits        17946    17952       +6     
  Misses        529      529
Impacted Files Coverage Δ
privacyidea/api/auth.py 96.92% <100%> (+0.14%) ⬆️
privacyidea/lib/user.py 99.29% <100%> (ø) ⬆️
privacyidea/api/lib/postpolicy.py 97.83% <100%> (ø) ⬆️

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 a2bac9b...3bcd19a. Read the comment docs.

Copy link
Member

cornelinux left a comment

lgtm. Thanks a lot.

I have only some style requests and some understanding requests.

In addition I think we should somewhere document more precisely how the handling is done.
But we can also open a new "documentation" issue for that.

tests/test_api_validate.py Show resolved Hide resolved
from privacyidea.lib.config import set_privacyidea_config


class AuthApiTestCase(MyApiTestCase):

This comment has been minimized.

Copy link
@cornelinux

cornelinux Oct 27, 2019

Member

Could you please add a test case for an internal admin?
We have this one "testadmin"/"testpw", but in theory you could also define an internal admin like "admin@int", even if the "int" realm does not exist. And I think the code will and should work, since we do not raise an error (which is fine) the pseudo realm "int" would be split and the admin is verify with the complete username "admin@int".

This comment has been minimized.

Copy link
@plettich

plettich Oct 28, 2019

Author Member

will do

This comment has been minimized.

Copy link
@plettich

plettich Oct 28, 2019

Author Member

Done in 3bcd19a

@@ -199,7 +202,27 @@ def test_09_get_user_from_param(self):
"realm": self.realm2}
user = get_user_from_param(param)
self.assertEqual("{0!s}".format(user), "<cornelius.resolver1@realm2>")


# test with splitAtSign set to '0'

This comment has been minimized.

Copy link
@cornelinux

cornelinux Oct 27, 2019

Member

Why did you remove this one? Did it change or did you move it to the other tests?
I think then we can also delete it - not only comment it.

This comment has been minimized.

Copy link
@plettich

plettich Oct 28, 2019

Author Member

This is a remnant from when i thought we could fix this all in one place...
But actually, the splitAtSign configuration isn't tested here (it is used in get_user_from_param(), not in split_user()) so i would add some tests for the library functions as well.

This comment has been minimized.

Copy link
@plettich

plettich Oct 28, 2019

Author Member

removed the commented code and added tests for splitAtSign in 3bcd19a

@cornelinux

This comment has been minimized.

Copy link
Member

cornelinux commented Oct 28, 2019

I added an issue #1908 for the documentation. So my generic comment is obsolete.
Only see my three comments inline.

- the `splitAtSign` configuration is `True` by default and will be always
  be set. So we don't need a `default` option when requesting the config.
- set the `spligAtSign` configuration explicitly in tests
- test for admin authentication
- test user lib with `splitAtSign` configuration
@cornelinux cornelinux merged commit aff239e into master Oct 28, 2019
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 97.13%)
Details
codecov/project 97.13% (+<.01%) compared to a2bac9b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@cornelinux cornelinux deleted the 1808/split_at_at_sign branch Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.