diff --git a/teknologr/api/bill.py b/teknologr/api/bill.py index 39b4a1c7..82e92a3b 100644 --- a/teknologr/api/bill.py +++ b/teknologr/api/bill.py @@ -1,5 +1,6 @@ import requests import re +import json from getenv import env @@ -8,6 +9,7 @@ class BILLException(Exception): class BILLAccountManager: + ERROR_ACCOUNT_DOES_NOT_EXIST = "BILL account does not exist" def __init__(self): self.api_url = env("BILL_API_URL") @@ -19,70 +21,72 @@ def admin_url(self, bill_code): return '' return f'{"/".join(self.api_url.split("/")[:-2])}/admin/userdata?id={bill_code}' - def create_bill_account(self, username): - if not re.search(r'^[A-Za-z0-9]+$', username): - raise BILLException("Can not create a BILL account using an LDAP username containing anything other than letters and numbers") - + def __request(self, path): try: - r = requests.post(self.api_url + "add?type=user&id=%s" % username, auth=(self.user, self.password)) + r = requests.post(self.api_url + path, auth=(self.user, self.password)) except: raise BILLException("Could not connect to BILL server") + if r.status_code != 200: - raise BILLException("BILL returned status: %d" % r.status_code) + raise BILLException(f"BILL returned status code {r.status_code}") + + number = 0 try: number = int(r.text) except ValueError: - # Returned value not a BILL code or error code - raise BILLException("BILL returned error: " + r.text) + # Not a number, return as text + return r.text + + # A negative number means an error code + if number == -3: + raise BILLException(BILLAccountManager.ERROR_ACCOUNT_DOES_NOT_EXIST) if number < 0: - raise BILLException("BILL returned error code: " + r.text) + raise BILLException(f"BILL returned error code: {number}") + return number + def create_bill_account(self, username): + if not re.search(r'^[A-Za-z0-9]+$', username): + raise BILLException("Can not create a BILL account using an LDAP username containing anything other than letters and numbers") + + result = self.__request(f"add?type=user&id={username}") + if type(result) == int: + return result + raise BILLException(f"BILL returned error: {result}") + def delete_bill_account(self, bill_code): - try: - r = requests.post(self.api_url + "del?type=user&acc=%s" % bill_code, auth=(self.user, self.password)) - except: - raise BILLException("Could not connect to BILL server") - if r.status_code != 200: - raise BILLException("BILL returned status: %d" % r.status_code) - try: - number = int(r.text) - except ValueError: - # Returned value not a number, unknown error occurred - raise BILLException("BILL returned error: " + r.text) - if number == 0: - pass # All is good - else: - raise BILLException("BILL returned error code: %d" % number) + info = self.get_bill_info(bill_code) + error = info.get('error') + if error: + raise BILLException(error) - def get_bill_info(self, bill_code): - import json - try: - r = requests.get(self.api_url + "get?type=user&acc=%s" % bill_code, auth=(self.user, self.password)) - except: - raise BILLException("Could not connect to BILL server") - if r.status_code != 200: - raise BILLException("BILL returned status: %d" % r.status_code) - # BILL API does not use proper http status codes - try: - error = int(r.text) - except ValueError: - # The returned string is not an integer, so presumably we have the json we want - return json.loads(r.text) - raise BILLException("BILL returned error code: " + r.text) + # If the BILL account does not exist all is ok + if info.get('exists') is False: + return + + result = self.__request(f"del?type=user&acc={bill_code}") + + if result != 0: + raise BILLException(f"BILL returned error: {result}") def find_bill_code(self, username): - import json - try: - r = requests.get(self.api_url + "get?type=user&id=%s" % username, auth=(self.user, self.password)) - except: - raise BILLException("Could not connect to BILL server") - if r.status_code != 200: - raise BILLException("BILL returned status: %d" % r.status_code) - # BILL API does not use proper http status codes + result = self.__request(f"get?type=user&id={username}") + return json.loads(result)["acc"] + + def get_bill_info(self, bill_code): + ''' + Get the info for a certain BILL account. Never throws. + ''' + if not bill_code: + return {'acc': None, 'exists': False} try: - error = int(r.text) - except ValueError: - # The returned string is not an integer, so presumably we have the json we want - return json.loads(r.text)["acc"] - raise BILLException("BILL returned error code: " + r.text + " for username: " + username) + result = self.__request(f"get?type=user&acc={bill_code}") + return { + **json.loads(result), + 'exists': True, + } + except BILLException as e: + s = str(e) + if s == BILLAccountManager.ERROR_ACCOUNT_DOES_NOT_EXIST: + return {'acc': bill_code, 'exists': False} + return {'acc': bill_code, 'error': s} diff --git a/teknologr/api/ldap.py b/teknologr/api/ldap.py index f7ebf136..d1c5a646 100644 --- a/teknologr/api/ldap.py +++ b/teknologr/api/ldap.py @@ -10,12 +10,26 @@ def LDAPError_to_string(e): if not isinstance(e, ldap.LDAPError): return str(e) data = e.args[0] - s = f"{data.get('desc' , '')} [{data.get('result')}]" + s = f"{data.get('desc' , '')} [LDAP result code {data.get('result')}]" info = data.get('info') if info: s += f' ({info})' return s +def get_ldap_account(username): + ''' + Get the info for a certain LDAP account. Never throws. + ''' + if not username: + return {'username': None, 'exists': False, 'groups': []} + try: + with LDAPAccountManager() as lm: + exists = lm.check_account(username) + groups = lm.get_ldap_groups(username) + return {'username': username, 'exists': exists, 'groups': sorted(groups)} + except ldap.LDAPError as e: + return {'username': username, 'error': LDAPError_to_string(e)} + class LDAPAccountManager: def __init__(self): # Don't require certificates @@ -94,14 +108,29 @@ def get_next_uidnumber(self): last = uid return last + 1 + def check_account(self, username): + ''' + Check if a certain LDAP account exists. + ''' + try: + dn = env("LDAP_USER_DN_TEMPLATE") % {'user': username} + self.ldap.search_s(dn, ldap.SCOPE_BASE) + return True + except ldap.LDAPError as e: + # Result code 32 = noSuchObject + if e.args[0].get('result') == 32: + return False + raise e + def delete_account(self, username): # Remove user from members group group_dn = env("LDAP_MEMBER_GROUP_DN") self.ldap.modify_s(group_dn, [(ldap.MOD_DELETE, 'memberUid', username.encode('utf-8'))]) - # Remove user - dn = env("LDAP_USER_DN_TEMPLATE") % {'user': username} - self.ldap.delete_s(dn) + # Remove user, if it exists + if self.check_account(username): + dn = env("LDAP_USER_DN_TEMPLATE") % {'user': username} + self.ldap.delete_s(dn) def change_password(self, username, password): # Changes both the user password and the samba password diff --git a/teknologr/api/views.py b/teknologr/api/views.py index 81319be2..68b3100c 100644 --- a/teknologr/api/views.py +++ b/teknologr/api/views.py @@ -2,6 +2,7 @@ from django.db import connection from django.db.models import Q from django.db.utils import IntegrityError +from django.http import HttpResponse from django_filters import rest_framework as filters from rest_framework import viewsets, permissions from rest_framework.views import APIView @@ -13,7 +14,7 @@ from datetime import datetime from api.serializers import * from api.filters import * -from api.ldap import LDAPAccountManager, LDAPError_to_string +from api.ldap import LDAPAccountManager, LDAPError_to_string, get_ldap_account from api.bill import BILLAccountManager, BILLException from api.utils import assert_public_member_fields from api.mailutils import mailNewPassword, mailNewAccount @@ -150,7 +151,7 @@ def multi_group_memberships_save(request): # get_or_create is used to ignore duplicates GroupMembership.objects.get_or_create(member_id=mid, group_id=int(gid)) - return Response(status=200) + return HttpResponse(status=200) @api_view(['POST']) @@ -170,7 +171,7 @@ def multi_functionaries_save(request): begin_date=begin_date ) - return Response(status=200) + return HttpResponse(status=200) @api_view(['POST']) @@ -184,7 +185,7 @@ def multi_decoration_ownerships_save(request): # get_or_create is used to ignore duplicates DecorationOwnership.objects.get_or_create(member_id=mid, decoration_id=int(did), acquired=acquired) - return Response(status=200) + return HttpResponse(status=200) # FunctionaryTypes and Functionaries @@ -259,14 +260,7 @@ def get_serializer(self, *args, **kwargs): class LDAPAccountView(APIView): def get(self, request, member_id): member = get_object_or_404(Member, id=member_id) - result = {} - try: - with LDAPAccountManager() as lm: - result = {'username': member.username, 'groups': lm.get_ldap_groups(member.username)} - except LDAPError as e: - return Response(LDAPError_to_string(e), status=400) - - return Response(result, status=200) + return Response(get_ldap_account(member.username), status=200) def post(self, request, member_id): # Create LDAP account for given user @@ -275,19 +269,19 @@ def post(self, request, member_id): password = request.data.get('password') mailToUser = request.data.get('mail_to_user') if not username or not password: - return Response('Username or password field missing', status=400) + return HttpResponse('Username or password field missing', status=400) if member.username: - return Response('Member already has an LDAP account', status=400) + return HttpResponse('Member already has an LDAP account', status=400) if Member.objects.filter(username=username).exists(): - return Response(f'Username "{username}" is already taken', status=400) + return HttpResponse(f'Username "{username}" is already taken', status=400) try: with LDAPAccountManager() as lm: lm.add_account(member, username, password) except LDAPError as e: - return Response(LDAPError_to_string(e), status=400) + return HttpResponse(LDAPError_to_string(e), status=400) # Store account details member.username = username @@ -296,30 +290,30 @@ def post(self, request, member_id): if mailToUser: status = mailNewAccount(member, password) if not status: - return Response(f'Account created, failed to send mail to {member}', status=500) + return HttpResponse(f'Account created, failed to send mail to {member}', status=500) - return Response(status=200) + return HttpResponse(status=200) def delete(self, request, member_id): # Delete LDAP account for a given user member = get_object_or_404(Member, id=member_id) if not member.username: - return Response('Member has no LDAP account', status=400) + return HttpResponse('Member has no LDAP account', status=400) if member.bill_code: - return Response('BILL account must be deleted first', status=400) + return HttpResponse('BILL account must be deleted first', status=400) try: with LDAPAccountManager() as lm: lm.delete_account(member.username) except LDAPError as e: - return Response(LDAPError_to_string(e), status=400) + return HttpResponse(LDAPError_to_string(e), status=400) # Delete account information from user in db member.username = None member.save() - return Response(status=200) + return HttpResponse(status=200) @api_view(['POST']) @@ -328,7 +322,7 @@ def change_ldap_password(request, member_id): password = request.data.get('password') mailToUser = request.data.get('mail_to_user') if not password: - return Response('Password field missing', status=400) + return HttpResponse('Password field missing', status=400) try: with LDAPAccountManager() as lm: @@ -336,35 +330,25 @@ def change_ldap_password(request, member_id): if mailToUser: status = mailNewPassword(member, password) if not status: - return Response('Password changed, but failed to send mail', status=500) + return HttpResponse('Password changed, but failed to send mail', status=500) except LDAPError as e: - return Response(LDAPError_to_string(e), status=400) + return HttpResponse(LDAPError_to_string(e), status=400) - return Response(status=200) + return HttpResponse(status=200) class BILLAccountView(APIView): def get(self, request, member_id): member = get_object_or_404(Member, id=member_id) - - if not member.bill_code: - return Response('Member has no BILL account', status=400) - - bm = BILLAccountManager() - try: - result = bm.get_bill_info(member.bill_code) - except BILLException as e: - return Response(str(e), status=400) - - return Response(result, status=200) + return Response(BILLAccountManager().get_bill_info(member.bill_code), status=200) def post(self, request, member_id): member = get_object_or_404(Member, id=member_id) if member.bill_code: - return Response('Member already has a BILL account', status=400) + return HttpResponse('Member already has a BILL account', status=400) if not member.username: - return Response('LDAP account missing', status=400) + return HttpResponse('LDAP account missing', status=400) bm = BILLAccountManager() bill_code = None @@ -380,29 +364,29 @@ def post(self, request, member_id): try: bill_code = bm.create_bill_account(member.username) except BILLException as e: - return Response(str(e), status=400) + return HttpResponse(str(e), status=400) member.bill_code = bill_code member.save() - return Response(status=200) + return HttpResponse(status=200) def delete(self, request, member_id): member = get_object_or_404(Member, id=member_id) if not member.bill_code: - return Response('Member has no BILL account', status=400) + return HttpResponse('Member has no BILL account', status=400) bm = BILLAccountManager() try: bm.delete_bill_account(member.bill_code) except BILLException as e: - return Response(str(e), status=400) + return HttpResponse(str(e), status=400) member.bill_code = None member.save() - return Response(status=200) + return HttpResponse(status=200) # Registration/Applicant @@ -464,7 +448,7 @@ def post(self, request, applicant_id): new_member.save() applicant.delete() except IntegrityError as err: - return Response(f'Error accepting application with id {applicant_id}: {str(err)}', status=400) + return HttpResponse(f'Error accepting application with id {applicant_id}: {str(err)}', status=400) # Add MemberTypes if set membership_date = request.data.get('membership_date') @@ -489,18 +473,18 @@ def post(self, request, applicant_id): status = mailNewAccount(new_member, password) if not status: - return Response(f'LDAP account created, failed to send mail to {new_member}', status=400) + return HttpResponse(f'LDAP account created, failed to send mail to {new_member}', status=400) # LDAP account creation failed (e.g. if the account already exists) except LDAPError as e: - return Response(f'Error creating LDAP account for {new_member}: {LDAPError_to_string(e)}', status=400) + return HttpResponse(f'Error creating LDAP account for {new_member}: {LDAPError_to_string(e)}', status=400) # Updating the username field failed, remove the created LDAP account # as it is not currently referenced by any member. except IntegrityError as e: lm.delete_account(username) - return Response(f'Error creating LDAP account for {new_member}: {str(e)}', status=400) + return HttpResponse(f'Error creating LDAP account for {new_member}: {str(e)}', status=400) - return Response(status=200) + return HttpResponse(status=200) def _create_member_type(self, member, date, type): try: @@ -525,9 +509,9 @@ def multi_applicant_submissions(request): errors.append(response.data) if len(errors) > 0: - return Response(f'{len(errors)} error(s) occured when accepting submissions: {" ".join(errors)}', status=400) + return HttpResponse(f'{len(errors)} error(s) occured when accepting submissions: {" ".join(errors)}', status=400) - return Response(status=200) + return HttpResponse(status=200) # JSON API:s diff --git a/teknologr/members/models.py b/teknologr/members/models.py index e8a04d60..1302f628 100644 --- a/teknologr/members/models.py +++ b/teknologr/members/models.py @@ -267,7 +267,9 @@ def save(self, *args, **kwargs): from ldap import LDAPError try: with LDAPAccountManager() as lm: - lm.change_email(self.username, self.email) + # Skip syncing email to LDAP if the LDAP account does not exist + if lm.check_account(self.username): + lm.change_email(self.username, self.email) except LDAPError as e: # Could not update LDAP, save other fields but keep original email self.email = self._original_email diff --git a/teknologr/members/static/js/base.js b/teknologr/members/static/js/base.js index 81892e9b..3eabceb3 100644 --- a/teknologr/members/static/js/base.js +++ b/teknologr/members/static/js/base.js @@ -52,8 +52,8 @@ const add_request_listener = ({ selector, method, url, data, confirmMessage, new else location.reload(); }); // XXX: The error message is not very helpful for the user, so should probably change this to something else - request.fail((jqHXR, textStatus) => { - alert(`Request failed (${textStatus}): ${jqHXR.responseText}`); + request.fail((jqHXR, _textStatus) => { + alert(`Request failed with status ${jqHXR.status} (${jqHXR.statusText}): ${jqHXR.responseText}`); }); }); }); diff --git a/teknologr/members/static/js/ldap.js b/teknologr/members/static/js/ldap.js index e0fc74bb..d6c8ad21 100644 --- a/teknologr/members/static/js/ldap.js +++ b/teknologr/members/static/js/ldap.js @@ -27,7 +27,7 @@ $(document).ready(function() { selector: "#delete-ldap-button", method: "DELETE", url: element => `/api/accounts/ldap/${element.data('id')}/`, - confirmMessage: "Vill du ta bort detta LDAP konto?", + confirmMessage: "Vill du ta bort detta LDAP-konto?", }); // Change the LDAP password for the selected user add_request_listener({ @@ -48,6 +48,6 @@ $(document).ready(function() { selector: "#delete-bill-button", method: "DELETE", url: element => `/api/accounts/bill/${element.data('id')}/`, - confirmMessage: "Vill du ta bort detta BILL konto?", + confirmMessage: "Vill du ta bort detta BILL-konto?", }); }); diff --git a/teknologr/members/templates/member.html b/teknologr/members/templates/member.html index 3f0ddeff..c548845d 100644 --- a/teknologr/members/templates/member.html +++ b/teknologr/members/templates/member.html @@ -185,45 +185,81 @@

Användarkonton

LDAP
{% if member.username %} - Användarnamn: {{ member.username }} -
{% if LDAP.error %} -
{{ LDAP.error }}
- {% else %} +
{{ LDAP.error }}
+ {% elif not LDAP.exists %} +
LDAP-konto "{{ member.username }}" existerar inte
+ {% endif %} + Användarnamn: {{ member.username }} +
Grupper: - + {% include "modals/ldap_changepw.html" with modalname="changepw_modal" title="Ändra LDAP lösenord" member_id=member.id only %} - - {% endif %} + {% else %} - - {% include "modals/ldap_add.html" with modalname="addldap_modal" title="Skapa LDAP konto" member_id=member.id only %} + + + {% include "modals/ldap_add.html" with modalname="addldap_modal" title="Skapa LDAP-konto" member_id=member.id only %} + {% endif %}
BILL
{% if member.bill_code %} + + {% if BILL.error %} +
{{ BILL.error }}
+ {% elif not BILL.exists %} +
BILL-konto "{{ member.bill_code }}" existerar inte
+ {% endif %} + Konto: {{ member.bill_code }}
- {% if BILL.error %} -
{{ BILL.error }}
- {% else %} + {% if BILL.exists %} Saldo: {{ BILL.balance }}€
- {% endif %} + + {% else %} - + {% endif %}
diff --git a/teknologr/members/tests_models.py b/teknologr/members/tests_models.py index d0429748..5715fb75 100644 --- a/teknologr/members/tests_models.py +++ b/teknologr/members/tests_models.py @@ -217,20 +217,6 @@ def test_member_type(self): self.assertEquals('StÄlM', member.current_member_type) self.assertFalse(member.shouldBeStalm()) - def test_saving(self): - member = Member( - given_names='Svatta', - surname='Teknolog', - ) - member.save() - - # try to cover lines missed by conditions. - member.username = 'teknosv1' - member.student_id = '123456' - member.email = 'svatta@teknolog.fi' - with self.assertRaises(LDAPError): - member.save() - class MemberOrderTest(BaseTest): def test_order_by(self): diff --git a/teknologr/members/views.py b/teknologr/members/views.py index ac093167..e59dcc79 100644 --- a/teknologr/members/views.py +++ b/teknologr/members/views.py @@ -6,6 +6,8 @@ from members.programmes import DEGREE_PROGRAMME_CHOICES from registration.models import Applicant from registration.forms import RegistrationForm +from api.ldap import get_ldap_account +from api.bill import BILLAccountManager from getenv import env from locale import strxfrm @@ -75,10 +77,11 @@ def member(request, member_id): form = MemberForm(request.POST, instance=member) if form.is_valid(): from ldap import LDAPError + from api.ldap import LDAPError_to_string try: form.save() - except LDAPError: - form.add_error("email", "Could not sync to LDAP") + except LDAPError as e: + form.add_error('email', f'Could not sync to LDAP: {LDAPError_to_string(e)}') context['result'] = 'success' else: context['result'] = 'failure' @@ -113,26 +116,15 @@ def member(request, member_id): # Get user account info if member.username: - from api.ldap import LDAPAccountManager, LDAPError_to_string - try: - with LDAPAccountManager() as lm: - context['LDAP'] = {'groups': lm.get_ldap_groups(member.username)} - except Exception as e: - context['LDAP'] = {'error': LDAPError_to_string(e)} + context['LDAP'] = get_ldap_account(member.username) if member.bill_code: - from api.bill import BILLAccountManager, BILLException bm = BILLAccountManager() - try: - context['bill_admin_url'] = bm.admin_url(member.bill_code) - context['BILL'] = bm.get_bill_info(member.bill_code) - - # Check that the username stored by BILL is the same as our - username = context['BILL']['id'] - if member.username != username: - context['BILL']['error'] = f'LDAP användarnamnen här ({member.username}) och i BILL ({username}) matchar inte' - except BILLException as e: - context['BILL'] = {'error': e} + context['bill_admin_url'] = bm.admin_url(member.bill_code) + context['BILL'] = bm.get_bill_info(member.bill_code) + username = context['BILL'].get('id') + if username and member.username != username: + context['BILL']['error'] = f'LDAP användarnamnen här ({member.username}) och i BILL ({username}) matchar inte' # load side list items set_side_context(context, 'members', member)