Re: [PATCH 5 of 6 v2] auth: return early in LoginRequired on API key validation

2015-05-13 Thread Thomas De Schampheleire
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

2015-05-13 Thread Thomas De Schampheleire
# 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

2015-05-13 Thread Marc Abramowitz
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

2015-05-13 Thread Thomas De Schampheleire
# 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

2015-05-13 Thread Mads Kiilerich

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

2015-05-13 Thread Mads Kiilerich

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')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _