PR#6: Return auth errors specfically to catch expired passwords - ipsilon

ipsilon

#6 Return auth errors specfically to catch expired passwords

Merged 8 years ago by puiterwijk. Opened 8 years ago by rcritten.

rcritten/ipsilon auth_error into master

Return PAM errors from mod_intercept_form_submit

Rob Crittenden • 8 years ago

da1c054

ipsilon/login/authform.py

file modified

+16 -6

		`@@ -9,6 +9,15 @@`
		`import cherrypy`
		`import subprocess`

		`+ # Translate PAM errors into more human-digestible values and eventually`
		`+ # other languages.`
		`+ PAM_AUTH_ERRORS = {`
		`+ "Authentication token is no longer valid; new one required":`
		`+ "Password is expired",`
		`+ "Authentication failure":`
		`+ "Authentication failure",`
		`+ }`
		`+`

		`class Form(LoginFormBase):`

		`@@ -19,12 +28,13 @@`
		`if not user.is_anonymous:`
		`return self.lm.auth_successful(self.trans, user.name, 'password')`
		`else:`
		`- try:`
		`- error = cherrypy.request.headers['EXTERNAL_AUTH_ERROR']`
		`- except KeyError:`
		`- error = "Unknown error using external authentication"`
		`- cherrypy.log.error("Error: %s" % error)`
		`- return self.lm.auth_failed(self.trans)`
		`+ error = cherrypy.request.wsgi_environ.get(`
		`+ 'EXTERNAL_AUTH_ERROR',`
		`+ 'Unknown error using external authentication'`
		`+ )`
		`+ error = PAM_AUTH_ERRORS.get(error, error)`
		`+ cherrypy.log.error("Error: %s" % error)`
		`+ return self.lm.auth_failed(self.trans, error)`


		`class LoginManager(LoginManagerBase):`

ipsilon/login/common.py

file modified

+2 -2

		`@@ -85,7 +85,7 @@`
		`trans.wipe()`
		`raise cherrypy.HTTPRedirect(redirect)`

		`- def auth_failed(self, trans):`
		`+ def auth_failed(self, trans, message=None):`
		`# try with next module`
		`next_login = self.next_login()`
		`if next_login:`
		`@@ -104,7 +104,7 @@`
		`# destroy session and return error`
		`if 'login_return' not in transdata:`
		`session.logout(None)`
		`- raise cherrypy.HTTPError(401)`
		`+ raise cherrypy.HTTPError(401, message)`

		`raise cherrypy.HTTPRedirect(transdata['login_return'])`

no initial comment

puiterwijk commented on line 4 of ipsilon/login/authform.py 8 years ago

Would it perhaps be an idea of instead of try/catch do something like:

error = cherrypy.request.wsgi_environ.get('EXTERNAL_AUTH_ERROR', 'Unknown error using external authentication')

puiterwijk commented on line 12 of ipsilon/login/authform.py 8 years ago

Do we know the types of errors that might be returned?

I'm thinking these errors might be too technical for the average user: for example with your example "auth token is expired" (in ticket #69), I don't think an average user would think "Auth token is expired" would mean he has an expired password.

So we might want to catch the ones we know, and revert to "Unknown error" for the rest.

(Note: this would also help internationalization if we would want to at some point)

rcritten commented 8 years ago

I thought about the dict get with a default rather than try/except but it seemed to me just as readable with try/except without having to span multiple lines. I'm not committed to this format though.

So while we can try to pick and choose error strings it would require substring matches which would be vulnerable to upstream changes (unlikely) and general ugliness. "Authentication token is no longer valid; new one required" and "Authentication failure" are the only messages I was able to generate.