OpenID login #716
OpenID login #716
Conversation
janno42
force-pushed
the
openid
branch
3 times, most recently
from
409f3d9
to
d9a9d18
Compare
| </a> | ||
| {% endbuttons %} | ||
| {% else %} | ||
| <form action="{% url 'login' %}" method="post" class="form-horizontal" role="form"> |
There was a problem hiding this comment.
as this posts to the plain login route, the user is shown the openid login screen instead of a form with errors after entering wrong user credentials
There was a problem hiding this comment.
yes, I can confirm that problem.
There was a problem hiding this comment.
i can think of only one use case where this might be a problem: when the admin user wants to login on production and provides wrong credentials. all other users will use open id on production, and in development open id will be deactivated.
so i don't think we need to add additional code for this.
There was a problem hiding this comment.
you might be right, but this is still some kind of a bug and it should be handled correctly. That means that even an admin user should see the login form and the system should tell him that has been a problem logging him in, because username and password did not match
There was a problem hiding this comment.
if anyone wants to look into this in the next days - feel welcome. it would require some logic to know which login was used and errors then need to be handled accordingly.
as this pr is blocking our much needed production update and i currently don't want to invest more time into the project than absolutely necessary, i won't implement it myself.
There was a problem hiding this comment.
see #721 for this issue
There was a problem hiding this comment.
Works quite well ❤️
Please have a look at my comments and please, also add oidc to the forbidden URL list in settings.py.
It would also make sense to add some tests that test the following:
- If OpenID login is not enabled. The user should see the local login page and be able to log in through that page.
- If OpenID login is available. The user should be able to login via OpenID (only add this test if it is possible to somehow mock the OpenID procedure.
- If OpenID login is available and the user wants to login using a local user, he should see the local login page and if he makes a mistake during login, he should still the page, but with an error message instead of the openID login page.
| </a> | ||
| {% endbuttons %} | ||
| {% else %} | ||
| <form action="{% url 'login' %}" method="post" class="form-horizontal" role="form"> |
There was a problem hiding this comment.
yes, I can confirm that problem.
|
about the tests:
|
|
Yes, right 😅 the local login is already tested. |
see e-valuation/EvaP#1366 for reference