Re: [PATCH 5 of 6 v2] auth: return early in LoginRequired on API key validation
On May 13, 2015 1:16:17 AM CEST, Mads Kiilerich m...@kiilerich.com wrote: On 05/10/2015 08:22 PM, Thomas De Schampheleire wrote: # HG changeset patch # User Thomas De Schampheleire thomas.de.schamphele...@gmail.com # Date 1427743622 -7200 # Mon Mar 30 21:27:02 2015 +0200 # Node ID b8ff1ec9f8e70a4540ab03db822367cde8ea1df2 # Parent 126d600ac54455fc07d40b65f511b73577090757 auth: return early in LoginRequired on API key validation Simplify the logic in the LoginRequired decorator when Kallithea is accessed using an API key. Either: - the key is valid and API access is allowed for the accessed method (continue), or - the key is invalid (redirect to login page), or - the accessed method does not allow API access (403 Forbidden) In none of these cases does it make sense to continue checking for user authentication, so return early. diff --git a/kallithea/lib/auth.py b/kallithea/lib/auth.py --- a/kallithea/lib/auth.py +++ b/kallithea/lib/auth.py @@ -59,6 +59,7 @@ from kallithea.lib.utils import get_repo get_user_group_slug, conditional_cache from kallithea.lib.caching_query import FromCache +from webob.exc import HTTPForbidden log = logging.getLogger(__name__) @@ -746,31 +747,31 @@ class LoginRequired(object): cls = fargs[0] user = cls.authuser loc = %s:%s % (cls.__class__.__name__, func.__name__) +log.debug('Checking access for user %s @ %s' % (user, loc)) # check if our IP is allowed if not user.ip_allowed: return redirect_to_login(_('IP %s not allowed' % (user.ip_addr))) -# check if we used an APIKEY and it's a valid one -# defined whitelist of controllers which API access will be enabled -_api_key = request.GET.get('api_key', '') -api_access_valid = allowed_api_access(loc, api_key=_api_key) - -# explicit controller is enabled or API is in our whitelist -if self.api_access or api_access_valid: -log.debug('Checking API KEY access for %s' % cls) -if _api_key and _api_key in user.api_keys: -api_access_valid = True -log.debug('API KEY %s is VALID' % _api_key[-4:]) +# check if we used an API key and it's a valid one +api_key = request.GET.get('api_key') +if api_key is not None: +# explicit controller is enabled or API is in our whitelist +if self.api_access or allowed_api_access(loc, api_key=api_key): +if api_key in user.api_keys: +log.info('user %s authenticated with API key %s @ %s' + % (user, api_key[-4:], loc)) +return func(*fargs, **fkwargs) +else: +log.warning('API key %s is NOT valid' % api_key[-4:]) +return redirect_to_login(_('Invalid API key')) else: -api_access_valid = False -if not _api_key: -log.debug(API KEY *NOT* present in request) -else: -log.warning(API KEY %s *NOT* valid % _api_key[-4:]) +# controller does not allow API access +log.warning('API access to %s is not allowed' % loc) +raise HTTPForbidden() I pushed these two, thanks .. but used abort(403) to be consistent with the other 403 in the same function. Yes, I wondered about that. What is the difference? Also, any particular reason why you did not apply 6of6, which was also generic cleanup? /Mads ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
[PATCH] tests: restrict pytest test collection to kallithea/tests
# HG changeset patch # User Thomas De Schampheleire thomas.de.schamphele...@gmail.com # Date 1431544206 -7200 # Wed May 13 21:10:06 2015 +0200 # Node ID edce9d365e2d6711e1336ee58a1b20520d8283ae # Parent e19127e4040d1817bbec77118ca7377644f76a64 tests: restrict pytest test collection to kallithea/tests When the kallithea root directory contains a populated virtualenv, pytest would also collect tests in python packages installed there. Restrict the tests to be considered to any test_*.py file inside kallithea/tests. With this change, the 'norecursedirs' value of .* and *.egg are no longer relevant. diff --git a/setup.cfg b/setup.cfg --- a/setup.cfg +++ b/setup.cfg @@ -11,7 +11,8 @@ detailed-errors = 1 nologcapture = 1 [pytest] -norecursedirs = .* *.egg kallithea/tests/scripts +norecursedirs = kallithea/tests/scripts +python_files = kallithea/tests/**/test_*.py [compile_catalog] domain = kallithea ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: [PATCH] tests: restrict pytest test collection to kallithea/tests
Yeah, that seems reasonable. On Wed, May 13, 2015 at 12:34 PM, Mads Kiilerich m...@kiilerich.com wrote: On 05/13/2015 09:26 PM, Thomas De Schampheleire wrote: # HG changeset patch # User Thomas De Schampheleire thomas.de.schamphele...@gmail.com # Date 1431544206 -7200 # Wed May 13 21:10:06 2015 +0200 # Node ID edce9d365e2d6711e1336ee58a1b20520d8283ae # Parent e19127e4040d1817bbec77118ca7377644f76a64 tests: restrict pytest test collection to kallithea/tests When the kallithea root directory contains a populated virtualenv, pytest would also collect tests in python packages installed there. Restrict the tests to be considered to any test_*.py file inside kallithea/tests. With this change, the 'norecursedirs' value of .* and *.egg are no longer relevant. Marc, do you agree with this? Should we also just rename the test_ files in the scripts folder and get rid of the norecurse option? /Mads diff --git a/setup.cfg b/setup.cfg --- a/setup.cfg +++ b/setup.cfg @@ -11,7 +11,8 @@ detailed-errors = 1 nologcapture = 1 [pytest] -norecursedirs = .* *.egg kallithea/tests/scripts +norecursedirs = kallithea/tests/scripts +python_files = kallithea/tests/**/test_*.py [compile_catalog] domain = kallithea ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
[PATCH] pullrequest/compare: add logical changeset index to clarify the order
# HG changeset patch # User Thomas De Schampheleire thomas.de.schamphele...@gmail.com # Date 1431287504 -7200 # Sun May 10 21:51:44 2015 +0200 # Node ID e19127e4040d1817bbec77118ca7377644f76a64 # Parent 6e8effd028bf41a132aee02e52ffc0bf990dadf4 pullrequest/compare: add logical changeset index to clarify the order Is the parent-most changeset in a changeset the one at the top or at the bottom? When the revision numbers are not shown, it is not obvious to determine this. This commit adds a logical changeset index to the commit list in a pullrequest or compare view. The index starts at 1 (the parent-most commit) and has no relation whatsoever with the commit hash or revision number. diff --git a/kallithea/public/css/style.css b/kallithea/public/css/style.css --- a/kallithea/public/css/style.css +++ b/kallithea/public/css/style.css @@ -2311,6 +2311,13 @@ h3.files_location { margin-right: -3px; } +.changeset-logical-index { +color: #66; +font-style: italic; +font-size: 85%; +padding-right: 0.5em; +} + .changeset_hash { color: #00; } diff --git a/kallithea/templates/compare/compare_cs.html b/kallithea/templates/compare/compare_cs.html --- a/kallithea/templates/compare/compare_cs.html +++ b/kallithea/templates/compare/compare_cs.html @@ -1,5 +1,6 @@ ## Changesets table ! div class=container + % num_cs = len(c.cs_ranges) % %if not c.cs_ranges: span class=empty_data${_('No changesets')}/span %else: @@ -39,7 +40,7 @@ td style=width: 140pxspan class=tooltip title=${h.tooltip(h.age(cs.date))}${cs.date}/span/td tddiv class=gravatar commit_id=${cs.raw_id}${h.gravatar(h.email_or_none(cs.author), size=14)}/div/td tddiv class=author${h.person(cs.author)}/div/td - td${h.link_to(h.show_id(cs),h.url('changeset_home',repo_name=c.cs_repo.repo_name,revision=cs.raw_id))}/td +tdspan class=changeset-logical-index${num_cs - cnt}/span ${h.link_to(h.show_id(cs),h.url('changeset_home',repo_name=c.cs_repo.repo_name,revision=cs.raw_id))}/td td %if cs.branch: span class=branchtag${h.link_to(cs.branch,h.url('changelog_home',repo_name=c.cs_repo.repo_name,branch=cs.branch))}/span ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: [PATCH] tests: restrict pytest test collection to kallithea/tests
On 05/13/2015 10:17 PM, Marc Abramowitz wrote: Yeah, that seems reasonable. Thomas' patch and/or my suggestion? ;-) /Mads On Wed, May 13, 2015 at 12:34 PM, Mads Kiilerich m...@kiilerich.com mailto:m...@kiilerich.com wrote: On 05/13/2015 09:26 PM, Thomas De Schampheleire wrote: # HG changeset patch # User Thomas De Schampheleire thomas.de.schamphele...@gmail.com mailto:thomas.de.schamphele...@gmail.com # Date 1431544206 -7200 # Wed May 13 21:10:06 2015 +0200 # Node ID edce9d365e2d6711e1336ee58a1b20520d8283ae # Parent e19127e4040d1817bbec77118ca7377644f76a64 tests: restrict pytest test collection to kallithea/tests When the kallithea root directory contains a populated virtualenv, pytest would also collect tests in python packages installed there. Restrict the tests to be considered to any test_*.py file inside kallithea/tests. With this change, the 'norecursedirs' value of .* and *.egg are no longer relevant. Marc, do you agree with this? Should we also just rename the test_ files in the scripts folder and get rid of the norecurse option? /Mads diff --git a/setup.cfg b/setup.cfg --- a/setup.cfg +++ b/setup.cfg @@ -11,7 +11,8 @@ detailed-errors = 1 nologcapture = 1 [pytest] -norecursedirs = .* *.egg kallithea/tests/scripts +norecursedirs = kallithea/tests/scripts +python_files = kallithea/tests/**/test_*.py [compile_catalog] domain = kallithea ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Example of test failure output with pytest
Hi Some pytest feedback: I'm sorry for being lazy and not approaching upstream, but it is so appealing to talk directly to the friendly ambassadors ;-) One use case, with https://bitbucket.org/conservancy/kallithea/commits/9b8ba0f1c87b4fe89481b2c2c8723f6636d147cd , I got (valid) test failures: ... ../kallithea-venv/lib/python2.7/site-packages/WebHelpers-1.3-py2.7.egg/webhelpers/pylonslib/flash.py:349: UnicodeEncodeError -- Captured stdout call --- 7 literal(u'Created repository a href=/vcs_test_hg_new%C4%85%C4%99%C5%82vcs_test_hg_new\u0105\u0119\u0142/a') __ TestHomeController.test_index_with_anonymous_access_disabled ___ self = kallithea.tests.functional.test_home.TestHomeController testMethod=test_index_with_anonymous_access_disabled def test_index_with_anonymous_access_disabled(self): with fixture.anon_access(False): response = self.app.get(url(controller='home', action='index'), status=302) kallithea/tests/functional/test_home.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ build/bdist.linux-x86_64/egg/webob/request.py:1049: in get_response ??? build/bdist.linux-x86_64/egg/webob/request.py:1022: in call_application ??? ../kallithea-venv/lib/python2.7/site-packages/WebTest-1.4.3-py2.7.egg/webtest/lint.py:179: in lint_app iterator = application(environ, start_response_wrapper) .eggs/Paste-2.0.1-py2.7.egg/paste/gzipper.py:31: in __call__ return self.application(environ, start_response) .eggs/Paste-2.0.1-py2.7.egg/paste/cascade.py:130: in __call__ return self.apps[-1](environ, start_response) .eggs/Paste-2.0.1-py2.7.egg/paste/registry.py:379: in __call__ app_iter = self.application(environ, start_response) kallithea/lib/middleware/wrapper.py:43: in __call__ return self.application(environ, start_response) kallithea/lib/base.py:277: in __call__ return self._handle_request(environ, start_response) kallithea/lib/middleware/simplegit.py:68: in _handle_request return self.application(environ, start_response) kallithea/lib/base.py:277: in __call__ return self._handle_request(environ, start_response) kallithea/lib/middleware/simplehg.py:73: in _handle_request return self.application(environ, start_response) ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/middleware.py:150: in __call__ self.app, environ, catch_exc_info=True) ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/util.py:48: in call_wsgi_application app_iter = application(environ, start_response) ../kallithea-venv/lib/python2.7/site-packages/WebError-0.10.3-py2.7.egg/weberror/errormiddleware.py:156: in __call__ return self.application(environ, start_response) ../kallithea-venv/lib/python2.7/site-packages/Beaker-1.6.4-py2.7.egg/beaker/middleware.py:155: in __call__ return self.wrap_app(environ, session_start_response) ../kallithea-venv/lib/python2.7/site-packages/Routes-1.13-py2.7.egg/routes/middleware.py:131: in __call__ response = self.app(environ, start_response) ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/wsgiapp.py:107: in __call__ response = self.dispatch(controller, environ, start_response) ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/wsgiapp.py:312: in dispatch return controller(environ, start_response) kallithea/lib/base.py:383: in __call__ return WSGIController.__call__(self, environ, start_response) ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/controllers/core.py:211: in __call__ response = self._dispatch_call() ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/controllers/core.py:162: in _dispatch_call response = self._inspect_call(func) ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/controllers/core.py:105: in _inspect_call result = self._perform_call(func, args) ../kallithea-venv/lib/python2.7/site-packages/Pylons-1.0-py2.7.egg/pylons/controllers/core.py:57: in _perform_call return func(**args) string:2: in index ??? kallithea/lib/auth.py:785: in __wrapper return redirect_to_login() kallithea/lib/auth.py:726: in redirect_to_login h.flash(h.literal(message), category='warning') _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _