Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5364)

Issue 6593070: CAS auth rework

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by remco.boerma
Modified:
11 years, 6 months ago
Reviewers:
thomas.j.waldmann, Reimar Bauer
Visibility:
Public.

Description

CAS auth rework

Patch Set 1 #

Total comments: 20

Patch Set 2 : after 1st review #

Total comments: 7

Patch Set 3 : Reworked comments, added inline comments #

Total comments: 33

Patch Set 4 : More PEP 8 as requested, more doc, fixes as requested #

Total comments: 8

Patch Set 5 : Added the patched asked by Thomas #

Unified diffs Side-by-side diffs Delta from patch set Stats (+112 lines, -34 lines) Patch
M MoinMoin/auth/cas.py View 1 2 3 4 3 chunks +112 lines, -34 lines 0 comments Download

Messages

Total messages: 12
Reimar Bauer
some hints can there be a unit test too for verification of the changes. http://codereview.appspot.com/6593070/diff/1/MoinMoin/auth/cas.py ...
11 years, 7 months ago (2012-10-03 08:15:42 UTC) #1
remco.boerma
In response to your comments. I'll upload the new version. http://codereview.appspot.com/6593070/diff/1/MoinMoin/auth/cas.py File MoinMoin/auth/cas.py (right): http://codereview.appspot.com/6593070/diff/1/MoinMoin/auth/cas.py#newcode62 ...
11 years, 7 months ago (2012-10-03 08:49:36 UTC) #2
Reimar Bauer
it would be good the inserted code has exactly the same license as moin can ...
11 years, 7 months ago (2012-10-03 09:04:41 UTC) #3
ThomasJWaldmann
http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py File MoinMoin/auth/cas.py (right): http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py#newcode62 MoinMoin/auth/cas.py:62: by Massimo Di Pierro with patches from others licensed ...
11 years, 7 months ago (2012-10-03 09:24:40 UTC) #4
Reimar Bauer
answered the license only 2 or only 3 is not what moin as license has ...
11 years, 7 months ago (2012-10-03 12:03:23 UTC) #5
remco.boerma
Fixed more code, it's quite pylint proof. please have a look again. i've tested it ...
11 years, 6 months ago (2012-10-04 07:38:26 UTC) #6
ThomasJWaldmann
http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py File MoinMoin/auth/cas.py (right): http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py#newcode62 MoinMoin/auth/cas.py:62: by Massimo Di Pierro with patches from others licensed ...
11 years, 6 months ago (2012-10-04 08:43:33 UTC) #7
remco.boerma
Patched as requested http://codereview.appspot.com/6593070/diff/9001/MoinMoin/auth/cas.py File MoinMoin/auth/cas.py (right): http://codereview.appspot.com/6593070/diff/9001/MoinMoin/auth/cas.py#newcode14 MoinMoin/auth/cas.py:14: Logging = log.getLogger(__name__) pylint was complaining, ...
11 years, 6 months ago (2012-10-04 09:30:05 UTC) #8
remco.boerma
License stuff. http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py File MoinMoin/auth/cas.py (right): http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py#newcode62 MoinMoin/auth/cas.py:62: by Massimo Di Pierro with patches from ...
11 years, 6 months ago (2012-10-04 09:32:21 UTC) #9
ThomasJWaldmann
http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py File MoinMoin/auth/cas.py (right): http://codereview.appspot.com/6593070/diff/4002/MoinMoin/auth/cas.py#newcode62 MoinMoin/auth/cas.py:62: by Massimo Di Pierro with patches from others licensed ...
11 years, 6 months ago (2012-10-04 10:39:42 UTC) #10
ThomasJWaldmann
http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py File MoinMoin/auth/cas.py (right): http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py#newcode57 MoinMoin/auth/cas.py:57: Part of this code if from is http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py#newcode77 MoinMoin/auth/cas.py:77: ...
11 years, 6 months ago (2012-10-05 08:38:09 UTC) #11
remco.boerma
11 years, 6 months ago (2012-10-08 08:01:21 UTC) #12
Done as requested and more questions on the logout mech

http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py
File MoinMoin/auth/cas.py (right):

http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py#newcode57
MoinMoin/auth/cas.py:57: Part of this code if from
On 2012/10/05 08:38:10, ThomasJWaldmann wrote:
> is

Done.

http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py#newcode77
MoinMoin/auth/cas.py:77: if len(envelope) > 0:
I'm not sure if that is supported. See
http://docs.python.org/library/xml.dom.html#nodelist-objects 

On 2012/10/05 08:38:10, ThomasJWaldmann wrote:
> btw, if envelope is a simple list, you can also just say:
> 
> if envelope:

http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py#newcode160
MoinMoin/auth/cas.py:160: logging.info('new cas user has id: %s '%
(moin_user.id))
If you want it that way, sure :)
On 2012/10/04 10:39:42, ThomasJWaldmann wrote:
> as a general hint (please review similar cases):
> if you start a tuple (or list, or dict) and it yet has only one element, do it
> like this:
> 
> (x, )
> 
> It's more regular then.

Done

http://codereview.appspot.com/6593070/diff/3/MoinMoin/auth/cas.py#newcode195
MoinMoin/auth/cas.py:195: # TODO: fix above or mention in docs
On 2012/10/04 10:39:42, ThomasJWaldmann wrote:
> hmm, I am not sure, so take this just as a wild guess:
> 
> iirc openid is in a somehow similar situation, maybe have a look there how it
> handles such stuff.

From what i can find, it doesn't handle logouts at all. Not in the 1.9.5
codebase anyway. 
Say i want to remove the session. How would that be done?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b