From 26826585072fcd4d7a6736c0ffd91327e246599c Mon Sep 17 00:00:00 2001 From: Ludwig Lehnert Date: Sat, 3 Oct 2026 18:59:48 +0000 Subject: [PATCH] allowing upn login --- app/web_ui.py | 35 ++++++++++++++++++------- dev/ad-entrypoint.sh | 17 ++++++++++++ dev/e2e.py | 11 +++++++- dev/seed-files.sh | 30 +++++++++++++++++++++ tests/document_live_smoke.mjs | 31 ++++++++++++++++++++-- tests/test_web_ui.py | 49 ++++++++++++++++++++++++++++++----- 6 files changed, 155 insertions(+), 18 deletions(-) diff --git a/app/web_ui.py b/app/web_ui.py index b10e2e8..c39f0dc 100644 --- a/app/web_ui.py +++ b/app/web_ui.py @@ -332,19 +332,24 @@ def authenticate_user(username: str, password: str) -> Optional[Dict[str, str]]: workgroup = os.environ["WORKGROUP"] realm = os.environ["REALM"] + is_upn = "\\" not in qualified if "\\" in qualified: domain_name, account = qualified.split("\\", 1) if domain_name.casefold() != workgroup.casefold(): return None + principal = f"{account}@{realm}" else: - account, principal_realm = qualified.rsplit("@", 1) - domain_names = {realm.casefold(), workgroup.casefold(), os.getenv("DOMAIN", realm).casefold()} - if principal_realm.casefold() not in domain_names: + if qualified.count("@") != 1: return None - if not account or is_excluded_user(account): + account, principal_realm = qualified.rsplit("@", 1) + # A UPN suffix belongs to AD, including alternate DNS suffixes. DOMAIN + # names the domain controller; WORKGROUP is only for DOMAIN\user logins. + if not principal_realm or principal_realm.casefold() == workgroup.casefold() or any(char.isspace() for char in principal_realm): + return None + principal = qualified + if not account or any(char in account for char in "@\\/") or is_excluded_user(account): return None canonical_name = f"{workgroup}\\{account}" - principal = f"{account}@{realm}" cache_fd = -1 cache_path = "" @@ -356,8 +361,9 @@ def authenticate_user(username: str, password: str) -> Optional[Dict[str, str]]: cache_fd = -1 command_env = os.environ.copy() command_env["KRB5CCNAME"] = f"FILE:{cache_path}" + command_env["LC_ALL"] = "C" auth_result = subprocess.run( - ["kinit", principal], + ["kinit", *(["-C", "-E"] if is_upn else []), "--", principal], input=f"{password}\n", capture_output=True, text=True, @@ -365,6 +371,20 @@ def authenticate_user(username: str, password: str) -> Optional[Dict[str, str]]: timeout=15, check=False, ) + if auth_result.returncode != 0: + return None + if is_upn: + # Use the KDC-confirmed account, never assume the UPN prefix is its + # sAMAccountName. Keep the ticket private and remove it below. + ticket = subprocess.run(["klist", "-c", cache_path], capture_output=True, text=True, + env=command_env, timeout=15, check=False) + matched = re.search(r"^Default principal:\s*([^\s@\\/]+)@([^\s]+)\s*$", ticket.stdout, re.MULTILINE) + if ticket.returncode != 0 or not matched or matched.group(2).casefold() != realm.casefold(): + return None + account = matched.group(1) + if is_excluded_user(account): + return None + canonical_name = f"{workgroup}\\{account}" except (OSError, subprocess.TimeoutExpired): return None finally: @@ -375,9 +395,6 @@ def authenticate_user(username: str, password: str) -> Optional[Dict[str, str]]: os.remove(cache_path) except OSError: pass - if auth_result.returncode != 0: - return None - try: sid_result = subprocess.run( ["wbinfo", "--name-to-sid", canonical_name], diff --git a/dev/ad-entrypoint.sh b/dev/ad-entrypoint.sh index d43d0ac..3f675cd 100755 --- a/dev/ad-entrypoint.sh +++ b/dev/ad-entrypoint.sh @@ -120,6 +120,23 @@ ensure_user "$AD_WEB_ADMIN_USER" "$AD_WEB_ADMIN_PASSWORD" Preview Administrator ensure_user report_svc "$AD_USER_PASSWORD" Report Service ensure_user MSOL_sync "$AD_USER_PASSWORD" Directory Sync +# Windows' post-2000 logon name can differ from the pre-2000 account name, +# including a DNS suffix that differs from the domain's Kerberos realm. +for user in dave frank; do + user_dn=$(samba-tool user show "$user" | sed -n 's/^dn: //p') + if [[ $user == dave ]]; then + user_upn="david.davis@${AD_DNS_DOMAIN}" + else + user_upn="frank.foster@people.${AD_DNS_DOMAIN}" + fi + ldbmodify -H /var/lib/samba/private/sam.ldb < int: ) check(non_admin.status == 200 and non_admin.json().get("role") == "user", "valid non-admin domain user cannot sign in") user_token = non_admin.json()["token"] - for username in (f"{WORKGROUP}\\alice", f"alice@{DNS_DOMAIN}", f"alice@{WORKGROUP}"): + for username in (f"{WORKGROUP}\\alice", f"alice@{DNS_DOMAIN}", f"alice@{DNS_DOMAIN.upper()}"): formatted = http("/api/login", method="POST", value={"username": username, "password": USER_PASSWORD}) check(formatted.status == 200, f"qualified user login failed for {username}") check(formatted.json().get("sid") == non_admin.json()["sid"] and formatted.json().get("role") == "user", "login format changes identity or grants administration") + check(http("/api/login", method="POST", value={"username": f"alice@{WORKGROUP}", "password": USER_PASSWORD}).status == 401, + "UPN login accepts a NetBIOS suffix") + for account, upn in (("dave", f"david.davis@{DNS_DOMAIN}"), ("frank", f"frank.foster@people.{DNS_DOMAIN}")): + legacy = http("/api/login", method="POST", value={"username": f"{WORKGROUP}\\{account}", "password": USER_PASSWORD}) + modern = http("/api/login", method="POST", value={"username": upn, "password": USER_PASSWORD}) + check(legacy.status == modern.status == 200, f"UPN alias authentication failed for {upn}") + check(modern.json()["user"] == f"{WORKGROUP}\\{account}" and modern.json()["sid"] == legacy.json()["sid"], + "UPN prefix is confused with another account") + check(modern.json()["role"] == "user", "UPN alias receives unexpected admin access") for endpoint in ("/api/overview", "/api/access", "/api/storage", "/api/trash", "/api/report", "/api/system"): check(http(endpoint, token=user_token).status == 403, f"non-admin can read {endpoint}") for endpoint in ("/api/access", "/api/actions/backup", "/api/actions/reconciliation", "/api/trash/restore"): diff --git a/dev/seed-files.sh b/dev/seed-files.sh index cca327d..9e8f605 100755 --- a/dev/seed-files.sh +++ b/dev/seed-files.sh @@ -50,6 +50,36 @@ font=next(Path('/usr/share/fonts').rglob('NimbusSans-Regular.otf')) draw=ImageDraw.Draw(image) draw.text((120,200),'INVOICE SCAN\nCustomer reference SCANNEDUNIQUE742\nInvoice total 1500 EUR\nPayment due 30 October 2026',fill='black',font=ImageFont.truetype(str(font),44),spacing=40) image.save(root/'scanned-invoice.pdf','PDF',resolution=150) + +# Large portrait/landscape images and a small image for responsive preview checks. +# Generate them locally so the preview needs no external downloads or image assets. +def preview_image(path, width, height): + path.parent.mkdir(parents=True, exist_ok=True) + image = Image.new('RGB', (width, height), '#dceaf2') + draw = ImageDraw.Draw(image) + unit = min(width, height) + sun = unit // 9 + center = (width * 3 // 4, height // 4) + draw.ellipse((center[0] - sun, center[1] - sun, + center[0] + sun, center[1] + sun), fill='#e7bb57') + draw.polygon([(0, height * 3 // 4), (width // 3, height // 3), + (width * 3 // 4, height * 4 // 5), (width, height // 2), + (width, height), (0, height)], fill='#7e9a82') + draw.polygon([(0, height * 9 // 10), (width // 2, height * 3 // 5), + (width, height * 4 // 5), (width, height), (0, height)], fill='#59796a') + margin = unit // 20 + label_font = ImageFont.truetype(str(font), max(12, unit // 24)) + draw.text((margin, margin), f'{width} × {height} px', fill='#304a60', font=label_font) + draw.text((margin, height - margin - unit // 20), 'IMAGEPREVIEW742', fill='white', font=label_font) + draw.rectangle((0, 0, width - 1, height - 1), outline='#304a60', width=max(2, unit // 100)) + image.save(path) + +photos = Path('/data/groups/data/Finance/Photos') +preview_image(photos/'portrait-4000x6000.jpg', 4000, 6000) +preview_image(photos/'landscape-6000x4000.png', 6000, 4000) +preview_image(photos/'small-320x240.png', 320, 240) +preview_image(Path('/data/private/alice/private-portrait.jpg'), 2400, 3600) + for folder in (root,Path('/data/private/alice')): for name in ('Thumbs.db','THUMBS.DB','~$Locked.docx','~$Locked.pptx'): (folder/name).write_text('IGNOREDARTIFACT742') diff --git a/tests/document_live_smoke.mjs b/tests/document_live_smoke.mjs index ef3f2b3..59c08ea 100644 --- a/tests/document_live_smoke.mjs +++ b/tests/document_live_smoke.mjs @@ -45,7 +45,34 @@ try{ await page.setViewportSize({width:390,height:844}); assert.deepEqual(await page.locator('#document-dialog').boundingBox(),{x:0,y:0,width:390,height:844}); await page.screenshot({path:out+'/02c-live-mobile-pdf-viewer.png',fullPage:false}); - await page.locator('#document-close').click();await page.locator('#reset-filters').click(); + await page.locator('#document-close').click(); + // Real seeded JPEG/PNG files exercise the image parser and responsive dialog together. + for(const name of ['portrait-4000x6000.jpg','landscape-6000x4000.png','small-320x240.png','private-portrait.jpg']){ + await page.setViewportSize({width:1500,height:1050}); + await page.locator('#search-query').fill(name); + const card=page.locator('.document-card').filter({hasText:name}); + await card.waitFor();await card.locator('[data-document]').click(); + await page.locator('#document-preview img').waitFor(); + await page.waitForFunction(()=>{const img=document.querySelector('#document-preview img');return img?.complete&&img.naturalWidth>0;}); + for(const [device,viewport] of [['desktop',{width:1500,height:1050}],['mobile',{width:390,height:844}]]){ + await page.setViewportSize(viewport); + const metrics=await page.evaluate(()=>{ + const dialog=document.querySelector('#document-dialog'),preview=document.querySelector('#document-preview'),img=preview.querySelector('img'); + const bounds=img.getBoundingClientRect(),area=preview.getBoundingClientRect(); + return {ratio:bounds.width/bounds.height,naturalRatio:img.naturalWidth/img.naturalHeight, + fits:bounds.width>0&&bounds.height>0&&bounds.left>=area.left&&bounds.top>=area.top&&bounds.right<=area.right+1&&bounds.bottom<=area.bottom+1, + scrolls:[dialog,preview].some(element=>element.scrollHeight>element.clientHeight+1||element.scrollWidth>element.clientWidth+1)}; + }); + assert.ok(metrics.fits,`${name} must fit the ${device} preview`); + assert.ok(Math.abs(metrics.ratio-metrics.naturalRatio)<0.01,'Image must preserve its aspect ratio'); + assert.equal(metrics.scrolls,false,'Image dialog must not require scrolling'); + await page.screenshot({path:`${out}/02d-image-${name}-${device}.png`,fullPage:false}); + } + await page.locator('#document-close').click(); + } + const imageText=await page.evaluate(async()=>{const response=await fetch('/api/documents?q=IMAGEPREVIEW742&scope=content');return response.json();}); + assert.equal(imageText.total,0,'Image fixtures must not be OCRed or content-indexed'); + await page.locator('#reset-filters').click(); await page.waitForFunction(()=>document.querySelectorAll('.document-card').length>1); assert.equal(await page.evaluate(()=>document.documentElement.scrollWidth>innerWidth),false); await page.screenshot({path:out+'/03-live-mobile-files.png',fullPage:true}); @@ -74,6 +101,6 @@ try{ await admin.goto(origin+'/');await admin.locator('.document-card').first().waitFor(); assert.equal(await admin.locator('#admin-link').isVisible(),true); assert.deepEqual(errors,[]); - console.log('PASS: live user/admin login, OCR search/preview, original PDF download, mobile layout, /admin boundary, index monitoring and pause/resume.'); + console.log('PASS: live user/admin login, OCR search/preview, original PDF download, shared/private JPEG/PNG previews, mobile layout, /admin boundary, index monitoring and pause/resume.'); console.log('Screenshots: '+out); }finally{await browser.close();} diff --git a/tests/test_web_ui.py b/tests/test_web_ui.py index 4567495..538e863 100644 --- a/tests/test_web_ui.py +++ b/tests/test_web_ui.py @@ -297,7 +297,7 @@ class DomainAuthenticationTests(unittest.TestCase): result = web_ui.authenticate_domain_admin("alice", "p@ss word") self.assertEqual(result, "EXAMPLE\\alice") - self.assertEqual(run.call_args_list[0].args[0], ["kinit", "alice@EXAMPLE.COM"]) + self.assertEqual(run.call_args_list[0].args[0], ["kinit", "--", "alice@EXAMPLE.COM"]) self.assertNotIn("p@ss word", run.call_args_list[0].args[0]) @mock.patch.dict(os.environ, {'WORKGROUP':'EXAMPLE','REALM':'EXAMPLE.COM','DOMAIN_ADMINS_SID':'S-1-5-21-1-2-3-512'}) @@ -314,24 +314,61 @@ class DomainAuthenticationTests(unittest.TestCase): @mock.patch.dict(os.environ, {'WORKGROUP':'EXAMPLE','REALM':'EXAMPLE.COM','DOMAIN':'example.com','DOMAIN_ADMINS_SID':'S-1-5-21-1-2-3-512'}) @mock.patch('app.web_ui.subprocess.run') def test_login_formats_resolve_the_same_identity_and_role(self, run): - for name in ('alice','EXAMPLE\\alice','example\\alice','alice@example.com','alice@EXAMPLE.COM','alice@EXAMPLE'): + for name in ('alice','EXAMPLE\\alice','example\\alice','alice@example.com','alice@EXAMPLE.COM'): with self.subTest(name=name): run.reset_mock() - run.side_effect = [mock.Mock(returncode=0,stdout=''), + responses = [mock.Mock(returncode=0,stdout=''), mock.Mock(returncode=0,stdout='S-1-5-21-1-2-3-1100 SID_USER (1)'), mock.Mock(returncode=0,stdout='S-1-5-21-1-2-3-513'), mock.Mock(returncode=0,stdout='EXAMPLE\\alice 1'), mock.Mock(returncode=0,stdout='11100')] + if '@' in name: + responses.insert(1,mock.Mock(returncode=0,stdout='Default principal: alice@EXAMPLE.COM\n')) + run.side_effect = responses value = web_ui.authenticate_user(name,'password') self.assertEqual(value['sub'],'EXAMPLE\\alice') self.assertEqual(value['role'],'user') - self.assertEqual(run.call_args_list[0].args[0],['kinit','alice@EXAMPLE.COM']) - self.assertEqual(run.call_args_list[1].args[0],['wbinfo','--name-to-sid','EXAMPLE\\alice']) + self.assertEqual(run.call_args_list[0].args[0],['kinit','-C','-E','--',name] if '@' in name else ['kinit','--','alice@EXAMPLE.COM']) + self.assertEqual(run.call_args_list[2 if '@' in name else 1].args[0],['wbinfo','--name-to-sid','EXAMPLE\\alice']) run.reset_mock() - for name in ('alice@other.example','OTHER\\alice','MSOL_sync@example.com','krbtgt@example.com'): + for name in ('alice@EXAMPLE','OTHER\\alice','MSOL_sync@example.com','krbtgt@example.com','alice@','@example.com','alice@@example.com','EXAMPLE\\alice@example.com'): self.assertIsNone(web_ui.authenticate_user(name,'password')) run.assert_not_called() + @mock.patch.dict(os.environ, {'WORKGROUP':'EXAMPLE','REALM':'EXAMPLE.COM','DOMAIN':'dc.example.com','DOMAIN_ADMINS_SID':'S-1-5-21-1-2-3-512'}) + @mock.patch('app.web_ui.subprocess.run') + def test_upn_uses_kdc_account_with_different_prefix_and_alternate_dns_suffix(self, run): + for upn in ('alice.smith@example.com','alice.smith@people.example.net'): + with self.subTest(upn=upn): + run.reset_mock() + run.side_effect = [mock.Mock(returncode=0,stdout=''), + mock.Mock(returncode=0,stdout='Default principal: alice@EXAMPLE.COM\n'), + mock.Mock(returncode=0,stdout='S-1-5-21-1-2-3-1100 SID_USER (1)'), + mock.Mock(returncode=0,stdout='S-1-5-21-1-2-3-513'), + mock.Mock(returncode=0,stdout='EXAMPLE\\alice 1'), + mock.Mock(returncode=0,stdout='11100')] + result=web_ui.authenticate_user(upn,'password') + self.assertEqual(result['sub'],'EXAMPLE\\alice') + self.assertEqual(result['role'],'user') + self.assertEqual(run.call_args_list[0].args[0],['kinit','-C','-E','--',upn]) + self.assertEqual(run.call_args_list[2].args[0],['wbinfo','--name-to-sid','EXAMPLE\\alice']) + self.assertEqual(run.call_args_list[1].kwargs['env']['LC_ALL'],'C') + self.assertFalse(os.path.exists(run.call_args_list[1].args[0][2])) + + @mock.patch.dict(os.environ, {'WORKGROUP':'EXAMPLE','REALM':'EXAMPLE.COM'}) + @mock.patch('app.web_ui.subprocess.run') + def test_upn_rejects_failed_password_foreign_ticket_and_system_account_alias(self, run): + for code, ticket in ((1,''),(0,'Default principal: alice@OTHER.EXAMPLE\n'), + (0,'Default principal: MSOL_sync@EXAMPLE.COM\n'), + (0,'Default principal: krbtgt@EXAMPLE.COM\n'),(0,'unreadable ticket')): + with self.subTest(code=code,ticket=ticket): + run.reset_mock() + run.side_effect=[mock.Mock(returncode=code,stdout=''),mock.Mock(returncode=0,stdout=ticket)] + self.assertIsNone(web_ui.authenticate_user('alias@example.com','password')) + self.assertFalse(any(call.args[0][0]=='wbinfo' for call in run.call_args_list)) + cache=run.call_args_list[0].kwargs['env']['KRB5CCNAME'].removeprefix('FILE:') + self.assertFalse(os.path.exists(cache)) + @mock.patch.dict(os.environ, {'WORKGROUP':'EXAMPLE','REALM':'EXAMPLE.COM','DOMAIN_ADMINS_SID':'S-1-5-21-1-2-3-512'}) @mock.patch('app.web_ui.subprocess.run') def test_system_accounts_rejected_even_when_resolved_from_an_alias(self, run):