diff --git a/.github/conformance/browser-runner.py b/.github/conformance/browser-runner.py new file mode 100644 index 00000000..571051a9 --- /dev/null +++ b/.github/conformance/browser-runner.py @@ -0,0 +1,471 @@ +#!/usr/bin/env python3 +""" +Drive OIDC conformance front-channel URLs with Selenium/Chromium. + +The OpenID conformance suite's built-in BrowserControl uses HtmlUnit. For +Nextcloud, the login form is rendered by modern JavaScript, so the suite is +configured without browser commands and this worker handles exposed browser URLs +through the suite's existing runner API. +""" + +import base64 +import os +import sys +import time +import urllib.parse + +import httpx +from selenium import webdriver +from selenium.common.exceptions import NoSuchElementException +from selenium.common.exceptions import StaleElementReferenceException +from selenium.webdriver import ChromeOptions +from selenium.webdriver.common.by import By + + +CONFORMANCE_SERVER = os.environ.get("CONFORMANCE_SERVER", "https://nginx:8443/").rstrip("/") +SELENIUM_REMOTE_URL = os.environ.get("SELENIUM_REMOTE_URL") or os.environ.get("SELENIUM_HUB_HOST") or "http://chrome:4444/wd/hub" +OIDC_TEST_USER = os.environ["OIDC_TEST_USER"] +OIDC_TEST_PASSWORD = os.environ["OIDC_TEST_PASSWORD"] +POLL_SECONDS = float(os.environ.get("CONFORMANCE_BROWSER_POLL_SECONDS", "1")) +VISIT_TIMEOUT_SECONDS = int(os.environ.get("CONFORMANCE_BROWSER_VISIT_TIMEOUT", "90")) +LOGIN_REDIRECT_TIMEOUT_SECONDS = int(os.environ.get("CONFORMANCE_BROWSER_LOGIN_REDIRECT_TIMEOUT", "15")) +PLACEHOLDER_CHECK_SECONDS = float(os.environ.get("CONFORMANCE_BROWSER_PLACEHOLDER_CHECK_SECONDS", "2")) +SCREENSHOT_STABILITY_SECONDS = float(os.environ.get("CONFORMANCE_BROWSER_SCREENSHOT_STABILITY_SECONDS", "2")) + + +def log(message): + print(f"[conformance-browser] {message}", flush=True) + + +def new_driver(): + options = ChromeOptions() + options.set_capability("acceptInsecureCerts", True) + for argument in ( + "--headless=new", + "--no-sandbox", + "--disable-dev-shm-usage", + "--ignore-certificate-errors", + "--window-size=1280,1000", + ): + options.add_argument(argument) + return webdriver.Remote(command_executor=SELENIUM_REMOTE_URL, options=options) + + +def first_present(driver, selectors, timeout=5): + end = time.monotonic() + timeout + while time.monotonic() < end: + for by, value in selectors: + try: + element = driver.find_element(by, value) + if element.is_displayed(): + return element + except (NoSuchElementException, StaleElementReferenceException): + pass + time.sleep(0.2) + raise NoSuchElementException(f"None of the selectors were found: {selectors}") + + +def first_clickable(driver, selectors, timeout=5): + end = time.monotonic() + timeout + while time.monotonic() < end: + for by, value in selectors: + try: + element = driver.find_element(by, value) + if element.is_displayed() and element.is_enabled(): + return element + except (NoSuchElementException, StaleElementReferenceException): + pass + time.sleep(0.2) + raise NoSuchElementException(f"None of the selectors were clickable: {selectors}") + + +def submit_post(driver, url): + parsed = urllib.parse.urlsplit(url) + action = urllib.parse.urlunsplit((parsed.scheme, parsed.netloc, parsed.path, "", "")) + params = urllib.parse.parse_qsl(parsed.query, keep_blank_values=True) + + inputs = "\n".join( + f'' + for name, value in params + ) + page = f""" + + + +
+ {inputs} +
+ + + +""" + driver.get("data:text/html;charset=utf-8," + urllib.parse.quote(page)) + + +def html_escape(value): + return ( + value.replace("&", "&") + .replace('"', """) + .replace("<", "<") + .replace(">", ">") + ) + + +def is_login_page(driver): + current_url = driver.current_url + if "/index.php/login" in current_url: + return True + try: + return bool(driver.find_elements(By.ID, "login")) + except StaleElementReferenceException: + return False + + +def login(driver): + login_url = driver.current_url + log(f"Logging in at {driver.current_url}") + user = first_present(driver, ((By.ID, "user"), (By.NAME, "user")), timeout=30) + password = first_present(driver, ((By.ID, "password"), (By.NAME, "password")), timeout=30) + user.clear() + user.send_keys(OIDC_TEST_USER) + password.clear() + password.send_keys(OIDC_TEST_PASSWORD) + submit = first_clickable( + driver, + ( + (By.ID, "submit-form"), + (By.CSS_SELECTOR, "button[type='submit']"), + (By.CSS_SELECTOR, "input[type='submit']"), + ), + timeout=10, + ) + submit.click() + wait_for_login_redirect(driver, login_url) + + +def wait_for_login_redirect(driver, login_url): + deadline = time.monotonic() + LOGIN_REDIRECT_TIMEOUT_SECONDS + while time.monotonic() < deadline: + current_url = driver.current_url + if current_url != login_url: + return + if not is_login_page(driver): + return + time.sleep(0.2) + + +def grant_consent_if_present(driver): + if "/apps/oidc/consent" not in driver.current_url and not driver.find_elements(By.ID, "oidc-consent"): + return False + + try: + allow = first_clickable( + driver, + ( + (By.XPATH, "//button[normalize-space()='Allow']"), + (By.XPATH, "//button[contains(normalize-space(.), 'Allow')]"), + ), + timeout=15, + ) + log("Granting consent") + allow.click() + return True + except NoSuchElementException: + return False + + +def is_conformance_callback(url): + parsed = urllib.parse.urlsplit(url) + return parsed.netloc == "nginx:8443" and ( + parsed.path.startswith("/test/a/") + or parsed.path.startswith("/test-mtls/a/") + ) + + +def page_diagnostics(driver): + try: + ready_state = driver.execute_script("return document.readyState") + except Exception: + ready_state = "unavailable" + try: + body = driver.execute_script("return document.body ? document.body.innerText : ''") + except Exception: + body = "" + body = " ".join((body or "").split()) + if len(body) > 1000: + body = body[:1000] + "..." + return { + "url": driver.current_url, + "title": driver.title, + "ready_state": ready_state, + "body": body, + } + + +def page_ready_for_screenshot(driver): + diag = page_diagnostics(driver) + if diag["ready_state"] != "complete": + return False + return bool(diag["title"] or diag["body"]) + + +def get_pending_upload_entry(client, test_id, uploaded_placeholders): + try: + response = client.get(f"{CONFORMANCE_SERVER}/api/log/{test_id}") + response.raise_for_status() + log_entries = response.json() + except Exception as exc: + log(f"Unable to read log placeholders for {test_id}: {exc}") + return None + + for entry in reversed(log_entries): + placeholder = entry.get("upload") + if ( + placeholder + and entry.get("result") == "REVIEW" + and (test_id, placeholder) not in uploaded_placeholders + ): + return entry + + return None + + +def screenshot_data_urls(driver): + try: + for quality in (80, 60, 40): + result = driver.execute_cdp_cmd( + "Page.captureScreenshot", + { + "format": "jpeg", + "quality": quality, + "captureBeyondViewport": False, + }, + ) + encoded = result.get("data") + if encoded: + yield f"jpeg q{quality}", f"data:image/jpeg;base64,{encoded}" + except Exception as exc: + log(f"Unable to capture JPEG screenshot through Chrome DevTools: {exc}") + + encoded = base64.b64encode(driver.get_screenshot_as_png()).decode("ascii") + yield "png", f"data:image/png;base64,{encoded}" + + +def encoded_data_size(data_url): + marker = "base64," + marker_index = data_url.find(marker) + if marker_index == -1: + return 0 + try: + return len(base64.b64decode(data_url[marker_index + len(marker):])) + except Exception: + return 0 + + +def upload_review_screenshot(client, test_id, placeholder, driver): + url = f"{CONFORMANCE_SERVER}/api/log/{test_id}/images/{placeholder}" + + for label, data_url in screenshot_data_urls(driver): + size = encoded_data_size(data_url) + log(f"Uploading {label} screenshot for placeholder {placeholder} ({size} bytes)") + try: + response = client.post( + url, + content=data_url, + headers={"Content-Type": "text/plain; charset=utf-8"}, + ) + except Exception as exc: + log(f"Unable to upload screenshot for {test_id}: {exc}") + return False + + if response.status_code == 200: + log(f"Uploaded screenshot for review placeholder {placeholder}") + return True + + body = " ".join(response.text.split()) + if len(body) > 300: + body = body[:300] + "..." + log(f"Screenshot upload failed with HTTP {response.status_code}: {body}") + + if response.status_code != 400: + return False + + return False + + +def is_second_login_placeholder(entry): + return ( + entry.get("src") == "ExpectSecondLoginPage" + or "login for a second time" in (entry.get("msg") or "").lower() + ) + + +def maybe_upload_pending_review_screenshot(client, test_id, uploaded_placeholders, driver, entry=None): + entry = entry or get_pending_upload_entry(client, test_id, uploaded_placeholders) + if not entry: + return False + + if not page_ready_for_screenshot(driver): + return None + + placeholder = entry.get("upload") + log(f"Review placeholder {placeholder} is pending at {driver.current_url}") + if upload_review_screenshot(client, test_id, placeholder, driver): + uploaded_placeholders.add((test_id, placeholder)) + return True + + return False + + +def drive_url(driver, client, test_id, uploaded_placeholders, method, url): + log(f"Visiting {method} {url}") + if method.upper() == "POST": + submit_post(driver, url) + else: + driver.get(url) + + deadline = time.monotonic() + VISIT_TIMEOUT_SECONDS + last_seen_url = None + last_url_changed_at = time.monotonic() + next_placeholder_check = 0 + while time.monotonic() < deadline: + now = time.monotonic() + current_url = driver.current_url + if current_url != last_seen_url: + last_seen_url = current_url + last_url_changed_at = now + log(f"Browser at {current_url}") + + if is_conformance_callback(current_url): + log(f"Reached conformance callback {current_url}") + return current_url + + if is_login_page(driver): + placeholder_entry = get_pending_upload_entry(client, test_id, uploaded_placeholders) + if placeholder_entry and is_second_login_placeholder(placeholder_entry): + upload_result = maybe_upload_pending_review_screenshot( + client, + test_id, + uploaded_placeholders, + driver, + placeholder_entry, + ) + if upload_result is None: + time.sleep(0.2) + continue + + login(driver) + continue + + if grant_consent_if_present(driver): + continue + + if ( + now >= next_placeholder_check + and now - last_url_changed_at >= SCREENSHOT_STABILITY_SECONDS + and page_ready_for_screenshot(driver) + ): + next_placeholder_check = now + PLACEHOLDER_CHECK_SECONDS + if maybe_upload_pending_review_screenshot(client, test_id, uploaded_placeholders, driver): + return current_url + + time.sleep(0.5) + + log(f"Timed out waiting for callback; current URL is {driver.current_url}") + diag = page_diagnostics(driver) + log(f"Timeout page title: {diag['title']}") + log(f"Timeout page readyState: {diag['ready_state']}") + log(f"Timeout page body: {diag['body']}") + return driver.current_url + + +def get_browser_items(status): + browser = status.get("browser") or {} + items = browser.get("urlsWithMethod") or [] + if items: + return [ + {"url": item["url"], "method": item.get("method") or "GET"} + for item in items + if item.get("url") + ] + return [{"url": url, "method": "GET"} for url in browser.get("urls", [])] + + +def get_driver(drivers, test_id): + driver = drivers.get(test_id) + if driver is None: + driver = new_driver() + drivers[test_id] = driver + return driver + + +def close_driver(drivers, test_id): + driver = drivers.pop(test_id, None) + if driver is None: + return + log(f"Closing browser session for {test_id}") + try: + driver.quit() + except Exception as exc: + log(f"Unable to close browser session for {test_id}: {exc}") + + +def main(): + drivers = {} + processed = set() + uploaded_placeholders = set() + active_test_id = None + + with httpx.Client(verify=False, timeout=15.0) as client: + while True: + try: + running = client.get(f"{CONFORMANCE_SERVER}/api/runner/running").json() + except Exception as exc: + log(f"Unable to read running tests: {exc}") + time.sleep(POLL_SECONDS) + continue + + for test_id in running: + try: + status = client.get(f"{CONFORMANCE_SERVER}/api/runner/{test_id}").json() + except Exception as exc: + log(f"Unable to read status for {test_id}: {exc}") + continue + + for item in get_browser_items(status): + url = item["url"] + method = item["method"].upper() + key = (test_id, method, url) + if key in processed: + continue + + if active_test_id is not None and active_test_id != test_id: + for old_test_id in list(drivers): + if old_test_id != test_id: + close_driver(drivers, old_test_id) + + active_test_id = test_id + driver = get_driver(drivers, test_id) + processed.add(key) + + try: + drive_url(driver, client, test_id, uploaded_placeholders, method, url) + except Exception as exc: + log(f"Browser visit failed for {test_id}: {exc}") + finally: + try: + client.post( + f"{CONFORMANCE_SERVER}/api/runner/browser/{test_id}/visit", + params={"url": url}, + ) + except Exception as exc: + log(f"Unable to mark URL visited for {test_id}: {exc}") + + time.sleep(POLL_SECONDS) + + +if __name__ == "__main__": + try: + main() + except KeyboardInterrupt: + sys.exit(0) diff --git a/.github/conformance/check-results.py b/.github/conformance/check-results.py new file mode 100644 index 00000000..356aa413 --- /dev/null +++ b/.github/conformance/check-results.py @@ -0,0 +1,110 @@ +#!/usr/bin/env python3 +"""Exit non-zero only when exported conformance results contain blocking failures.""" + +from __future__ import annotations + +import argparse +import json +import pathlib +import sys +import zipfile +from collections import Counter + + +BLOCKING_RESULTS = {"FAILED", "FAILURE", "ERROR", "INTERRUPTED", "UNKNOWN"} +NON_BLOCKING_RESULTS = {"PASSED", "SUCCESS", "SKIPPED", "WARNING", "REVIEW"} +FINISHED_STATUSES = {"FINISHED"} + + +def iter_export_logs(results_dir: pathlib.Path): + for zip_path in sorted(results_dir.glob("*.zip")): + with zipfile.ZipFile(zip_path) as export: + for name in sorted(export.namelist()): + if not name.endswith(".json"): + continue + with export.open(name) as handle: + yield zip_path.name, name, json.load(handle) + + for json_path in sorted(results_dir.glob("test-log-*.json")): + with json_path.open(encoding="utf-8") as handle: + yield json_path.name, json_path.name, json.load(handle) + + +def normalized_result(test_info: dict) -> str: + result = test_info.get("result") + if result: + return str(result).upper() + + status = test_info.get("status") + if status and status != "FINISHED": + return str(status).upper() + + return "UNKNOWN" + + +def is_blocking(test_info: dict) -> bool: + result = normalized_result(test_info) + status = str(test_info.get("status") or "").upper() + + if result in BLOCKING_RESULTS: + return True + + if result not in NON_BLOCKING_RESULTS: + return True + + return bool(status and status not in FINISHED_STATUSES and result != "SKIPPED") + + +def test_label(test_info: dict, log_name: str) -> str: + name = test_info.get("testName") or pathlib.Path(log_name).stem + test_id = test_info.get("testId") or test_info.get("_id") + if test_id: + return f"{name} ({test_id})" + return str(name) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--results-dir", default="conformance-results", type=pathlib.Path) + args = parser.parse_args() + + tests = [] + blocking = [] + counts = Counter() + + for export_name, log_name, data in iter_export_logs(args.results_dir): + test_info = data.get("testInfo", {}) + result = normalized_result(test_info) + counts[result] += 1 + item = { + "export": export_name, + "label": test_label(test_info, log_name), + "result": result, + "status": test_info.get("status") or "", + } + tests.append(item) + if is_blocking(test_info): + blocking.append(item) + + if not tests: + print(f"No conformance test logs found in {args.results_dir}", file=sys.stderr) + return 1 + + summary = ", ".join(f"{result}={count}" for result, count in sorted(counts.items())) + print(f"Conformance result summary: {summary}") + + if not blocking: + print("No blocking conformance failures found.") + return 0 + + print("Blocking conformance failures found:", file=sys.stderr) + for item in blocking: + print( + f"- {item['label']}: result={item['result']} status={item['status']} export={item['export']}", + file=sys.stderr, + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/conformance/docker-compose.chrome.yml b/.github/conformance/docker-compose.chrome.yml new file mode 100644 index 00000000..3bd4ca49 --- /dev/null +++ b/.github/conformance/docker-compose.chrome.yml @@ -0,0 +1,19 @@ +services: + # Selenium Chrome service for conformance testing + # Provides WebDriver endpoint at http://chrome:4444/wd/hub + chrome: + image: selenium/standalone-chromium:latest + container_name: conformance-chrome + shm_size: 2gb + ports: + - "4444:4444" + - "7900:7900" # VNC port for debugging + environment: + - SE_NODE_MAX_SESSIONS=3 + - SE_SESSION_REQUEST_TIMEOUT=60 + - SE_SESSION_RETRY_INTERVAL=2 + - SE_BROWSER_ARGS_HEADLESS=--headless=new + - SE_BROWSER_ARGS_NO_SANDBOX=--no-sandbox + - SE_BROWSER_ARGS_DISABLE_DEV_SHM_USAGE=--disable-dev-shm-usage + extra_hosts: + - "host.docker.internal:host-gateway" diff --git a/.github/conformance/docker-compose.github-actions.yml b/.github/conformance/docker-compose.github-actions.yml new file mode 100644 index 00000000..c0c48733 --- /dev/null +++ b/.github/conformance/docker-compose.github-actions.yml @@ -0,0 +1,12 @@ +services: + nginx: + extra_hosts: + - "host.docker.internal:host-gateway" + + server: + extra_hosts: + - "host.docker.internal:host-gateway" + + test: + extra_hosts: + - "host.docker.internal:host-gateway" diff --git a/.github/conformance/generate-report.py b/.github/conformance/generate-report.py new file mode 100644 index 00000000..c04dc19f --- /dev/null +++ b/.github/conformance/generate-report.py @@ -0,0 +1,193 @@ +#!/usr/bin/env python3 +"""Create a Markdown summary from OpenID conformance-suite exports.""" + +from __future__ import annotations + +import argparse +import datetime as dt +import json +import pathlib +import re +import zipfile +from collections import Counter + + +ISSUE_RESULTS = {"FAILURE", "ERROR", "WARNING"} +RESULT_ORDER = { + "FAILED": 0, + "FAILURE": 0, + "ERROR": 1, + "INTERRUPTED": 2, + "WARNING": 3, + "PASSED": 4, + "SUCCESS": 4, + "SKIPPED": 5, + "UNKNOWN": 6, +} + + +def compact(value: object) -> str: + return re.sub(r"\s+", " ", str(value or "")).strip() + + +def table_cell(value: object) -> str: + text = compact(value) + return text.replace("|", "\\|") + + +def truncate(value: str, max_length: int) -> str: + if len(value) <= max_length: + return value + return value[: max_length - 3].rstrip() + "..." + + +def iter_export_logs(results_dir: pathlib.Path): + for zip_path in sorted(results_dir.glob("*.zip")): + with zipfile.ZipFile(zip_path) as export: + for name in sorted(export.namelist()): + if not name.endswith(".json"): + continue + with export.open(name) as handle: + yield zip_path.name, name, json.load(handle) + + for json_path in sorted(results_dir.glob("test-log-*.json")): + with json_path.open(encoding="utf-8") as handle: + yield json_path.name, json_path.name, json.load(handle) + + +def normalized_result(test_info: dict) -> str: + result = test_info.get("result") + if result: + return str(result) + status = test_info.get("status") + if status and status != "FINISHED": + return str(status) + return "UNKNOWN" + + +def issue_summary(results: list[dict], max_issues: int) -> str: + issues = [ + f"{item.get('result')}: {compact(item.get('msg'))}" + for item in results + if item.get("result") in ISSUE_RESULTS and item.get("msg") + ] + if not issues: + return "" + extra = len(issues) - max_issues + selected = issues[:max_issues] + if extra > 0: + selected.append(f"+ {extra} more") + return "; ".join(selected) + + +def collect_tests(results_dir: pathlib.Path, max_issues: int) -> list[dict]: + tests = [] + for export_name, log_name, data in iter_export_logs(results_dir): + test_info = data.get("testInfo", {}) + test_results = data.get("results", []) + result = normalized_result(test_info) + tests.append( + { + "name": test_info.get("testName", pathlib.Path(log_name).stem), + "id": test_info.get("testId", test_info.get("_id", "")), + "status": test_info.get("status", ""), + "result": result, + "summary": compact(test_info.get("summary")), + "issues": issue_summary(test_results, max_issues), + "started": test_info.get("started", ""), + "export": export_name, + } + ) + return sorted( + tests, + key=lambda item: ( + RESULT_ORDER.get(item["result"], RESULT_ORDER["UNKNOWN"]), + item["name"], + item["id"], + ), + ) + + +def write_report(tests: list[dict], output: pathlib.Path, results_dir: pathlib.Path) -> None: + now = dt.datetime.now(dt.UTC).replace(microsecond=0).isoformat() + counts = Counter(test["result"] for test in tests) + + lines = [ + "# OIDC Conformance Report", + "", + f"Generated: {now}", + f"Results source: `{results_dir}`", + f"Executed tests: {len(tests)}", + "", + "## Result Summary", + "", + "| Result | Count |", + "| --- | ---: |", + ] + + if counts: + for result, count in sorted(counts.items(), key=lambda item: RESULT_ORDER.get(item[0], RESULT_ORDER["UNKNOWN"])): + lines.append(f"| {table_cell(result)} | {count} |") + else: + lines.append("| UNKNOWN | 0 |") + + lines.extend( + [ + "", + "## Executed Tests", + "", + "| Result | Status | Test | Description | Issues |", + "| --- | --- | --- | --- | --- |", + ] + ) + + if tests: + for test in tests: + test_name = test["name"] + if test["id"]: + test_name = f"{test_name} ({test['id']})" + lines.append( + "| " + + " | ".join( + [ + table_cell(test["result"]), + table_cell(test["status"]), + table_cell(test_name), + table_cell(truncate(test["summary"], 260)), + table_cell(truncate(test["issues"], 320)), + ] + ) + + " |" + ) + else: + lines.append("| UNKNOWN | UNKNOWN | No conformance test logs found | | |") + + lines.extend( + [ + "", + "## Notes", + "", + "- `WARNING` is reported by the conformance suite for behavior that may still complete the test module.", + "- `INTERRUPTED` means the test module did not reach a final pass/fail result in the exported run.", + "- Full logs and signed test exports are available in the workflow artifact.", + "", + ] + ) + + output.parent.mkdir(parents=True, exist_ok=True) + output.write_text("\n".join(lines), encoding="utf-8") + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--results-dir", default="conformance-results", type=pathlib.Path) + parser.add_argument("--output", default="CONFORMANCE.md", type=pathlib.Path) + parser.add_argument("--max-issues", default=3, type=int) + args = parser.parse_args() + + tests = collect_tests(args.results_dir, args.max_issues) + write_report(tests, args.output, args.results_dir) + + +if __name__ == "__main__": + main() diff --git a/.github/conformance/oidc-basic-config.json b/.github/conformance/oidc-basic-config.json new file mode 100644 index 00000000..cfc11cd3 --- /dev/null +++ b/.github/conformance/oidc-basic-config.json @@ -0,0 +1,20 @@ +{ + "alias": "${CONFORMANCE_ALIAS}", + "description": "Nextcloud OIDC GitHub Actions basic conformance", + "publish": "no", + "server": { + "discoveryUrl": "${NEXTCLOUD_BASE_URL}/index.php/apps/oidc/openid-configuration" + }, + "client": { + "client_id": "${OIDC_CLIENT_ID_1}", + "client_secret": "${OIDC_CLIENT_SECRET_1}" + }, + "client_secret_post": { + "client_id": "${OIDC_CLIENT_ID_1}", + "client_secret": "${OIDC_CLIENT_SECRET_1}" + }, + "client2": { + "client_id": "${OIDC_CLIENT_ID_2}", + "client_secret": "${OIDC_CLIENT_SECRET_2}" + } +} diff --git a/.github/conformance/write-config.py b/.github/conformance/write-config.py new file mode 100644 index 00000000..4af43772 --- /dev/null +++ b/.github/conformance/write-config.py @@ -0,0 +1,25 @@ +#!/usr/bin/env python3 + +import os +import string +import sys + + +def main() -> int: + if len(sys.argv) != 3: + print("Usage: write-config.py TEMPLATE OUTPUT", file=sys.stderr) + return 2 + + with open(sys.argv[1], encoding="utf-8") as template_file: + template = string.Template(template_file.read()) + + rendered = template.safe_substitute(os.environ) + + with open(sys.argv[2], "w", encoding="utf-8") as output_file: + output_file.write(rendered) + + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/workflows/build-test.yaml b/.github/workflows/build-test.yaml index 5c8bd6f3..bda2f700 100644 --- a/.github/workflows/build-test.yaml +++ b/.github/workflows/build-test.yaml @@ -13,8 +13,6 @@ on: branches: - "*" workflow_dispatch: - branches: - - "*" env: PHP_VERSION: 8.5 diff --git a/.github/workflows/oidc-conformance.yaml b/.github/workflows/oidc-conformance.yaml new file mode 100644 index 00000000..70715f6c --- /dev/null +++ b/.github/workflows/oidc-conformance.yaml @@ -0,0 +1,266 @@ +name: OIDC conformance + +on: + schedule: + - cron: "17 2 * * *" + workflow_dispatch: + +env: + PHP_VERSION: 8.5 + +jobs: + oidc_conformance: + runs-on: ubuntu-latest + if: github.event_name != 'schedule' || github.ref == 'refs/heads/master' + timeout-minutes: 90 + + env: + CONFORMANCE_ALIAS: nextcloud-${{ github.run_id }} + NEXTCLOUD_BASE_URL: http://host.docker.internal:8080 + OIDC_TEST_USER: oidc-test-user + OIDC_TEST_PASSWORD: oidc-test-password + OIDC_CLIENT_ID_1: nextcloud-conformance-client-00001 + OIDC_CLIENT_SECRET_1: nextcloud-conformance-secret-00001 + OIDC_CLIENT_ID_2: nextcloud-conformance-client-00002 + OIDC_CLIENT_SECRET_2: nextcloud-conformance-secret-00002 + + steps: + - name: Set app env + run: | + echo "APP_NAME=${GITHUB_REPOSITORY##*/}" >> $GITHUB_ENV + + - name: Checkout + uses: actions/checkout@v3 + with: + path: ${{ env.APP_NAME }} + + - name: Get appinfo data + id: appinfo + uses: mavrosxristoforos/get-xml-info@1.1.1 + with: + xml-file: ${{ env.APP_NAME }}/appinfo/info.xml + xpath: "//info//dependencies//nextcloud/@max-version" + + - name: Derive Nextcloud checkout ref + shell: bash + run: | + max_version="${{ steps.appinfo.outputs.info }}" + if [ -z "$max_version" ]; then + max_version="${{ steps.appinfo.outputs.value }}" + fi + if [ -z "$max_version" ]; then + echo "Error: could not read nextcloud max-version from appinfo" >&2 + exit 1 + fi + major="${max_version%%.*}" + echo "NEXTCLOUD_REF=stable$major" >> $GITHUB_ENV + + - name: Read package.json node and npm engines version + uses: skjnldsv/read-package-engines-version-actions@v2 + id: versions + continue-on-error: true + with: + path: ${{ env.APP_NAME }} + fallbackNode: "^22" + fallbackNpm: "^10" + + - name: Set up node ${{ steps.versions.outputs.nodeVersion }} + if: ${{ steps.versions.outputs.nodeVersion }} + uses: actions/setup-node@v3 + with: + node-version: ${{ steps.versions.outputs.nodeVersion }} + + - name: Set up npm ${{ steps.versions.outputs.npmVersion }} + if: ${{ steps.versions.outputs.npmVersion }} + run: npm i -g npm@"${{ steps.versions.outputs.npmVersion }}" + + - name: Set up php ${{ env.PHP_VERSION }} + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ env.PHP_VERSION }} + coverage: none + + - name: Set up Java + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: "21" + + - name: Install app dependencies and build assets + run: | + cd ${{ env.APP_NAME }} + composer install + npm ci + npm run build + + - name: Checkout Nextcloud server + uses: actions/checkout@v3 + with: + repository: nextcloud/server + path: nextcloud + submodules: recursive + ref: ${{ env.NEXTCLOUD_REF }} + + - name: Install Nextcloud dependencies + run: | + cd nextcloud + composer install + + - name: Install and configure Nextcloud OIDC provider + run: | + mv -v ${{ env.APP_NAME }} nextcloud/apps/ + cd nextcloud + php occ maintenance:install --admin-user admin --admin-pass admin + php occ app:enable ${{ env.APP_NAME }} + php occ config:system:set trusted_domains 1 --value=host.docker.internal + php occ config:system:set trusted_domains 2 --value=127.0.0.1 + php occ config:system:set overwrite.cli.url --value="${NEXTCLOUD_BASE_URL}" + php occ config:system:set overwritehost --value=host.docker.internal:8080 + php occ config:system:set overwriteprotocol --value=http + php occ config:app:set ${{ env.APP_NAME }} allow_user_settings --value no + OC_PASS="${OIDC_TEST_PASSWORD}" php occ user:add --password-from-env "${OIDC_TEST_USER}" + php occ user:setting "${OIDC_TEST_USER}" settings email oidc-test@example.test + callback="https://nginx:8443/test/a/${CONFORMANCE_ALIAS}/callback" + php occ oidc:create "OIDC Conformance Basic 1" "${callback}" \ + --client_id "${OIDC_CLIENT_ID_1}" \ + --client_secret "${OIDC_CLIENT_SECRET_1}" \ + --type confidential \ + --flow code \ + --allowed_scopes "openid profile email offline_access" + php occ oidc:create "OIDC Conformance Basic 2" "${callback}" \ + --client_id "${OIDC_CLIENT_ID_2}" \ + --client_secret "${OIDC_CLIENT_SECRET_2}" \ + --type confidential \ + --flow code \ + --allowed_scopes "openid profile email offline_access" + + - name: Start Nextcloud web server + run: | + cd nextcloud + php -S 0.0.0.0:8080 -t . > ../nextcloud-server.log 2>&1 & + echo $! > ../nextcloud-server.pid + for i in {1..30}; do + if curl --fail --silent http://127.0.0.1:8080/index.php/apps/oidc/openid-configuration > /dev/null; then + exit 0 + fi + sleep 2 + done + cat ../nextcloud-server.log + exit 1 + + - name: Checkout OpenID conformance suite + run: git clone --depth 1 https://gitlab.com/openid/conformance-suite.git conformance-suite + + - name: Build and start OpenID conformance suite + run: | + sudo sh -c 'echo "127.0.0.1 nginx" >> /etc/hosts' + sudo sh -c 'echo "127.0.0.1 oidcc-provider" >> /etc/hosts' + sudo sh -c 'echo "127.0.0.1 chrome" >> /etc/hosts' + cd conformance-suite + mvn -B -DskipTests package + docker compose \ + -f docker-compose-localtest.yml \ + -f ../nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/docker-compose.github-actions.yml \ + -f ../nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/docker-compose.chrome.yml \ + up -d mongodb nginx server chrome + # Wait for Chrome/Selenium service to be ready + for i in {1..60}; do + if curl --fail --silent http://chrome:4444/status > /dev/null 2>&1; then + echo "Chrome/Selenium service is ready" + break + fi + sleep 2 + done + # Wait for API runner + for i in {1..60}; do + if curl --insecure --fail --silent https://nginx:8443/api/runner/available > /dev/null; then + exit 0 + fi + sleep 5 + done + docker compose \ + -f docker-compose-localtest.yml \ + -f ../nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/docker-compose.github-actions.yml \ + -f ../nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/docker-compose.chrome.yml \ + logs server nginx chrome + exit 1 + + - name: Install conformance runner dependencies + run: python3 -m pip install --user httpx pyparsing selenium + + - name: Write conformance plan config + run: | + mkdir -p conformance-results conformance-config + python3 nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/write-config.py \ + nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/oidc-basic-config.json \ + conformance-config/oidc-basic-config.json + python3 -m json.tool conformance-config/oidc-basic-config.json > /dev/null + + - name: Run OIDC basic conformance plan + env: + CONFORMANCE_DEV_MODE: 1 + CONFORMANCE_SERVER: https://nginx:8443/ + CONFORMANCE_SERVER_MTLS: https://nginx:8444/ + SELENIUM_REMOTE_URL: http://chrome:4444/wd/hub + CONFORMANCE_BROWSER_VISIT_TIMEOUT: 90 + run: | + cd conformance-suite + : > ../conformance-browser.log + PYTHONUNBUFFERED=1 python3 ../nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/browser-runner.py \ + >> ../conformance-browser.log 2>&1 & + browser_runner_pid=$! + tail -n +1 -f ../conformance-browser.log & + browser_log_tail_pid=$! + cleanup_browser_runner() { + kill "${browser_runner_pid}" 2>/dev/null || true + kill "${browser_log_tail_pid}" 2>/dev/null || true + wait "${browser_runner_pid}" 2>/dev/null || true + wait "${browser_log_tail_pid}" 2>/dev/null || true + } + trap cleanup_browser_runner EXIT + set +e + python3 scripts/run-test-plan.py \ + --export-dir ../conformance-results \ + --verbose \ + "oidcc-basic-certification-test-plan[server_metadata=discovery][client_registration=static_client]" \ + ../conformance-config/oidc-basic-config.json + test_plan_status=$? + set -e + cleanup_browser_runner + trap - EXIT + echo "::group::conformance-browser.log" + cat ../conformance-browser.log || true + echo "::endgroup::" + set +e + python3 ../nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/check-results.py \ + --results-dir ../conformance-results + blocking_status=$? + set -e + if [ "${blocking_status}" -ne 0 ]; then + if [ "${test_plan_status}" -ne 0 ]; then + exit "${test_plan_status}" + fi + exit "${blocking_status}" + fi + if [ "${test_plan_status}" -ne 0 ]; then + echo "Conformance suite exited with ${test_plan_status}, but exported results contain only non-blocking WARNING/REVIEW outcomes." + fi + + - name: Generate conformance report + if: always() + run: | + python3 nextcloud/apps/${{ env.APP_NAME }}/.github/conformance/generate-report.py \ + --results-dir conformance-results \ + --output CONFORMANCE.md + cat CONFORMANCE.md + + - name: Upload conformance results + if: always() + uses: actions/upload-artifact@v4 + with: + name: oidc-conformance-results + path: | + CONFORMANCE.md + conformance-results + conformance-browser.log + nextcloud-server.log diff --git a/appinfo/routes.php b/appinfo/routes.php index 52a48fc0..fd337ced 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -184,6 +184,11 @@ 'url' => '/authorize', 'verb' => 'GET', ], + [ + 'name' => 'LoginRedirector#authorizePost', + 'url' => '/authorize', + 'verb' => 'POST', + ], [ 'name' => 'Cors#authorizeCorsResponse', 'url' => '/authorize', diff --git a/lib/Controller/ConsentController.php b/lib/Controller/ConsentController.php index 67c2c98d..808079fa 100644 --- a/lib/Controller/ConsentController.php +++ b/lib/Controller/ConsentController.php @@ -216,7 +216,11 @@ public function grant(): RedirectResponse { 'resource' => $this->session->get('oidc_resource'), 'code_challenge' => $this->session->get('oidc_code_challenge'), 'code_challenge_method' => $this->session->get('oidc_code_challenge_method'), - ])); + 'prompt' => $this->session->get('oidc_prompt'), + 'max_age' => $this->session->get('oidc_max_age'), + ], static function ($value): bool { + return $value !== null && $value !== ''; + })); // IMPORTANT: Close the session to commit changes before redirecting // Without this, the authorize endpoint won't see the updated session values @@ -434,6 +438,8 @@ public function deny(): RedirectResponse { $this->session->remove('oidc_resource'); $this->session->remove('oidc_code_challenge'); $this->session->remove('oidc_code_challenge_method'); + $this->session->remove('oidc_prompt'); + $this->session->remove('oidc_max_age'); $this->session->remove('oidc_requested_scopes'); // Return error to client diff --git a/lib/Controller/LoginRedirectorController.php b/lib/Controller/LoginRedirectorController.php index b186691c..f3defe26 100644 --- a/lib/Controller/LoginRedirectorController.php +++ b/lib/Controller/LoginRedirectorController.php @@ -159,6 +159,59 @@ public function __construct( * @param string $resource * @param string $code_challenge * @param string $code_challenge_method + * @param string $prompt + * @param string $max_age + * @return Response + */ + #[BruteForceProtection(action: 'oidc_login')] + #[NoCSRFRequired] + #[UseSession] + #[PublicPage] + public function authorizePost( + $client_id, + $state, + $response_type, + $redirect_uri, + $scope, + $nonce, + $resource = null, + $code_challenge = null, + $code_challenge_method = null, + $prompt = null, + $max_age = null + ): Response + { + return $this->authorize( + $client_id, + $state, + $response_type, + $redirect_uri, + $scope, + $nonce, + $resource, + $code_challenge, + $code_challenge_method, + $prompt, + $max_age); + } + + /** + * @PublicPage + * @NoCSRFRequired + * @UseSession + * @BruteForceProtection(action=oidc_login) + * + * @param string $client_id + * @param string $state + * @param string $response_type + * @param string $redirect_uri + * @param string $scope + * @param string $nonce + * @param string $resource + * @param string $code_challenge + * @param string $code_challenge_method + * @param string $prompt + * @param string $max_age * @return Response */ #[BruteForceProtection(action: 'oidc_login')] @@ -174,46 +227,58 @@ public function authorize( $nonce, $resource = null, $code_challenge = null, - $code_challenge_method = null + $code_challenge_method = null, + $prompt = null, + $max_age = null ): Response { + $prompt = $prompt ?? $this->request->getParam('prompt'); + $max_age = $max_age ?? $this->request->getParam('max_age'); + + $unsupportedRequestParameterResponse = $this->rejectUnsupportedRequestParameters( + $client_id, + $redirect_uri, + $state + ); + if ($unsupportedRequestParameterResponse !== null) { + return $unsupportedRequestParameterResponse; + } + if (!$this->userSession->isLoggedIn()) { - // Not authenticated yet - // Store oidc attributes in user session to be available after login - $this->session->set('oidc_client_id', $client_id); - $this->session->set('oidc_state', $state); - $this->session->set('oidc_response_type', $response_type); - $this->session->set('oidc_redirect_uri', $redirect_uri); - $this->session->set('oidc_scope', $scope); - $this->session->set('oidc_nonce', $nonce); - $this->session->set('oidc_resource', $resource); - $this->session->set('oidc_code_challenge', $code_challenge); - $this->session->set('oidc_code_challenge_method', $code_challenge_method); - - $afterLoginRedirectUrl = $this->urlGenerator->linkToRoute('oidc.Page.index', array_filter([ - 'client_id' => $client_id, - 'state' => $state, - 'response_type' => $response_type, - 'redirect_uri' => $redirect_uri, - 'scope' => $scope, - 'nonce' => $nonce, - 'resource' => $resource, - 'code_challenge' => $code_challenge, - 'code_challenge_method' => $code_challenge_method, - ])); - - $loginUrl = $this->urlGenerator->linkToRoute( - 'core.login.showLoginForm', - [ - 'redirect_url' => $afterLoginRedirectUrl - ] - ); + if ($this->promptContains($prompt, 'none')) { + $clientOrResponse = $this->loadAuthorizationClient($client_id); + if ($clientOrResponse instanceof Response) { + return $clientOrResponse; + } - $this->session->close(); // Close session to prevent session locking issues during redirect + $redirectUriErrorResponse = $this->validateAuthorizationRedirectUri($clientOrResponse, $client_id, $redirect_uri); + if ($redirectUriErrorResponse !== null) { + return $redirectUriErrorResponse; + } - $this->logger->debug('Not authenticated yet for client ' . $client_id . '. Redirect to login.'); + $this->logger->debug('prompt=none requested without authenticated user for client ' . $client_id . '. Returning login_required.'); + return $this->createAuthorizationErrorRedirect( + (string)$redirect_uri, + 'login_required', + 'User is not logged in.', + $state + ); + } - return new RedirectResponse($loginUrl); + return $this->redirectToLoginAfterOidcAuthentication( + $client_id, + $state, + $response_type, + $redirect_uri, + $scope, + $nonce, + $resource, + $code_challenge, + $code_challenge_method, + $prompt, + $max_age, + 'Not authenticated yet for client ' . $client_id . '. Redirect to login.' + ); } // Debug: Log client id before and after fallback @@ -235,13 +300,18 @@ public function authorize( $resource = $this->session->get('oidc_resource'); $code_challenge = $this->session->get('oidc_code_challenge'); $code_challenge_method = $this->session->get('oidc_code_challenge_method'); + $prompt = $this->session->get('oidc_prompt'); + $max_age = $this->session->get('oidc_max_age'); } + $oidcLoginPending = $this->session->get('oidc_login_pending') === true; + $authTime = $this->getOidcAuthenticationTime(); + // Guard: if critical OAuth params are still missing after session fallback, // return a meaningful error instead of letting downstream code crash with a 500 // (e.g. matchRedirectUri(null, ...) or trim(null) in PHP 8.4). - // Note: state is not critical for processing the request and might also not be passed by the client, e.g Guacamole, so we only check response_type and redirect_uri here. - if (empty($response_type) || empty($redirect_uri)) { + // Note: state is not critical for processing the request and might also not be passed by the client, e.g Guacamole. + if (empty($redirect_uri)) { $this->logger->error('Missing critical OAuth params after session fallback: ' . 'response_type=' . var_export($response_type, true) . ', ' . 'redirect_uri=' . var_export($redirect_uri, true)); @@ -255,26 +325,11 @@ public function authorize( $scope = Application::DEFAULT_SCOPE; } - $this->clientMapper->cleanUp(); - - try { - $client = $this->clientMapper->getByIdentifier($client_id); - } catch (ClientNotFoundException $e) { - $params = [ - 'message' => $this->l->t('Your client is not authorized to connect. Please inform the administrator of your client.'), - ]; - $this->logger->notice('Client ' . $client_id . ' is not authorized to connect.'); - return new TemplateResponse('core', '403', $params, 'error'); - } - - // The client must not be expired - if ($client->isDcr() && $this->time->getTime() > ($client->getIssuedAt() + (int)$this->appConfig->getAppValueString(Application::APP_CONFIG_DEFAULT_CLIENT_EXPIRE_TIME, Application::DEFAULT_CLIENT_EXPIRE_TIME))) { - $this->logger->warning('Client expired. Client id was ' . $client_id . '.'); - $params = [ - 'message' => $this->l->t('Your client is expired. Please inform the administrator of your client.'), - ]; - return new TemplateResponse('core', '400', $params, 'error'); + $clientOrResponse = $this->loadAuthorizationClient($client_id); + if ($clientOrResponse instanceof Response) { + return $clientOrResponse; } + $client = $clientOrResponse; // Set default resource if resource is not set at all if (!isset($resource) || trim($resource)==='') { @@ -308,21 +363,19 @@ public function authorize( $scope = $newScope; $this->logger->debug('[SCOPE DEBUG] Scope after filtering: ' . $scope); - // Check if redirect URI is configured for client - $redirectUris = $this->redirectUriMapper->getByClientId($client->getId()); - $redirectUriFound = false; - foreach ($redirectUris as $redirectUri) { - if ($this->redirectUriService->matchRedirectUri($redirect_uri, $redirectUri->getRedirectUri())) { - $redirectUriFound = true; - break; - } + $redirectUriErrorResponse = $this->validateAuthorizationRedirectUri($client, $client_id, $redirect_uri); + if ($redirectUriErrorResponse !== null) { + return $redirectUriErrorResponse; } - if (!$redirectUriFound) { - $params = [ - 'message' => $this->l->t('The received redirect URI is not accepted to connect. Please inform the administrator of your client.'), - ]; - $this->logger->notice('Redirect URI ' . $redirect_uri . ' is not accepted for client ' . $client_id . '.'); - return new TemplateResponse('core', '403', $params, 'error'); + + if (empty($response_type)) { + $this->logger->notice('Missing response_type in request for client ' . $client_id . '.'); + $separator = str_contains($redirect_uri, '?') ? '&' : '?'; + $url = $redirect_uri . $separator + . 'error=unsupported_response_type' + . '&error_description=Missing%20response_type' + . '&state=' . urlencode((string)$state); + return new RedirectResponse($url); } // Check response type @@ -357,6 +410,54 @@ public function authorize( return new RedirectResponse($url); } + if ($this->promptContains($prompt, 'login') && !$oidcLoginPending) { + $this->logger->debug('prompt=login requested for client ' . $client_id . '. Forcing reauthentication.'); + $this->userSession->logout(); + return $this->redirectToLoginAfterOidcAuthentication( + $client_id, + $state, + $response_type, + $redirect_uri, + $scope, + $nonce, + $resource, + $code_challenge, + $code_challenge_method, + $prompt, + $max_age, + 'Redirect to login for prompt=login reauthentication for client ' . $client_id . '.' + ); + } + + if ($this->maxAgeExceeded($max_age, $authTime)) { + if ($this->promptContains($prompt, 'none')) { + $this->logger->debug('prompt=none requested but max_age is exceeded for client ' . $client_id . '. Returning login_required.'); + return $this->createAuthorizationErrorRedirect( + (string)$redirect_uri, + 'login_required', + 'User authentication is too old.', + $state + ); + } + + $this->logger->debug('max_age is exceeded for client ' . $client_id . '. Forcing reauthentication.'); + $this->userSession->logout(); + return $this->redirectToLoginAfterOidcAuthentication( + $client_id, + $state, + $response_type, + $redirect_uri, + $scope, + $nonce, + $resource, + $code_challenge, + $code_challenge_method, + $prompt, + $max_age, + 'Redirect to login for max_age reauthentication for client ' . $client_id . '.' + ); + } + // Check if user is in allowed groups for client $clientGroups = $this->groupMapper->getGroupsByClientId($client->getId()); $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); @@ -449,6 +550,8 @@ public function authorize( $this->session->set('oidc_resource', $resource); $this->session->set('oidc_code_challenge', $code_challenge); $this->session->set('oidc_code_challenge_method', $code_challenge_method); + $this->session->set('oidc_prompt', $prompt); + $this->session->set('oidc_max_age', $max_age); $this->session->close(); // Close session to prevent session locking issues during redirect @@ -501,7 +604,7 @@ public function authorize( } else { $accessToken->setResource(substr($resource, 0, 2000)); } - $accessToken->setCreated($this->time->getTime()); + $accessToken->setCreated($authTime); $accessToken->setRefreshed($this->time->getTime()); if (empty($nonce) || !isset($nonce)) { $nonce = ''; @@ -559,4 +662,269 @@ public function authorize( return new RedirectResponse($url); } + + private function rejectUnsupportedRequestParameters( + mixed $clientId, + mixed $redirectUri, + mixed $state + ): ?Response { + $requestObject = $this->request->getParam('request'); + $requestUri = $this->request->getParam('request_uri'); + + if ( + !$this->hasNonEmptyRequestParameter($requestObject) + && !$this->hasNonEmptyRequestParameter($requestUri) + ) { + return null; + } + + if (!$this->hasNonEmptyRequestParameter($redirectUri)) { + $this->logger->notice('Unsupported request parameter received without a redirect URI.'); + return new TemplateResponse('core', '400', [ + 'message' => $this->l->t('Authorization request is missing a redirect URI.'), + ], 'error'); + } + + $clientOrResponse = $this->loadAuthorizationClient($clientId); + if ($clientOrResponse instanceof Response) { + return $clientOrResponse; + } + + $redirectUriErrorResponse = $this->validateAuthorizationRedirectUri( + $clientOrResponse, + $clientId, + $redirectUri + ); + if ($redirectUriErrorResponse !== null) { + return $redirectUriErrorResponse; + } + + if ($this->hasNonEmptyRequestParameter($requestObject)) { + $this->logger->notice('Request object parameter is not supported for client ' . $clientId . '.'); + return $this->createAuthorizationErrorRedirect( + (string)$redirectUri, + 'request_not_supported', + 'Request object parameter is not supported.', + $state + ); + } + + $this->logger->notice('Request URI parameter is not supported for client ' . $clientId . '.'); + return $this->createAuthorizationErrorRedirect( + (string)$redirectUri, + 'request_uri_not_supported', + 'Request URI parameter is not supported.', + $state + ); + } + + private function loadAuthorizationClient(mixed $clientId): Client|Response + { + if (!is_string($clientId) || trim($clientId) === '') { + $params = [ + 'message' => $this->l->t('Your client is not authorized to connect. Please inform the administrator of your client.'), + ]; + $this->logger->notice('Client ' . var_export($clientId, true) . ' is not authorized to connect.'); + return new TemplateResponse('core', '403', $params, 'error'); + } + + $this->clientMapper->cleanUp(); + + try { + $client = $this->clientMapper->getByIdentifier($clientId); + } catch (ClientNotFoundException $e) { + $params = [ + 'message' => $this->l->t('Your client is not authorized to connect. Please inform the administrator of your client.'), + ]; + $this->logger->notice('Client ' . $clientId . ' is not authorized to connect.'); + return new TemplateResponse('core', '403', $params, 'error'); + } + + if ($client === null) { + $params = [ + 'message' => $this->l->t('Your client is not authorized to connect. Please inform the administrator of your client.'), + ]; + $this->logger->notice('Client ' . $clientId . ' is not authorized to connect.'); + return new TemplateResponse('core', '403', $params, 'error'); + } + + // The client must not be expired + if ($client->isDcr() && $this->time->getTime() > ($client->getIssuedAt() + (int)$this->appConfig->getAppValueString(Application::APP_CONFIG_DEFAULT_CLIENT_EXPIRE_TIME, Application::DEFAULT_CLIENT_EXPIRE_TIME))) { + $this->logger->warning('Client expired. Client id was ' . $clientId . '.'); + $params = [ + 'message' => $this->l->t('Your client is expired. Please inform the administrator of your client.'), + ]; + return new TemplateResponse('core', '400', $params, 'error'); + } + + return $client; + } + + private function validateAuthorizationRedirectUri( + Client $client, + mixed $clientId, + mixed $redirectUri + ): ?TemplateResponse { + if (!is_string($redirectUri) || trim($redirectUri) === '') { + $params = [ + 'message' => $this->l->t('The received redirect URI is not accepted to connect. Please inform the administrator of your client.'), + ]; + $this->logger->notice('Redirect URI ' . var_export($redirectUri, true) . ' is not accepted for client ' . $clientId . '.'); + return new TemplateResponse('core', '403', $params, 'error'); + } + + // Check if redirect URI is configured for client + $redirectUris = $this->redirectUriMapper->getByClientId($client->getId()); + foreach ($redirectUris as $registeredRedirectUri) { + if ($this->redirectUriService->matchRedirectUri($redirectUri, $registeredRedirectUri->getRedirectUri())) { + return null; + } + } + + $params = [ + 'message' => $this->l->t('The received redirect URI is not accepted to connect. Please inform the administrator of your client.'), + ]; + $this->logger->notice('Redirect URI ' . $redirectUri . ' is not accepted for client ' . $clientId . '.'); + return new TemplateResponse('core', '403', $params, 'error'); + } + + private function hasNonEmptyRequestParameter(mixed $value): bool + { + if (is_string($value)) { + return trim($value) !== ''; + } + + return !empty($value); + } + + private function redirectToLoginAfterOidcAuthentication( + mixed $clientId, + mixed $state, + mixed $responseType, + mixed $redirectUri, + mixed $scope, + mixed $nonce, + mixed $resource, + mixed $codeChallenge, + mixed $codeChallengeMethod, + mixed $prompt, + mixed $maxAge, + string $logMessage + ): RedirectResponse { + // Store OIDC attributes in the user session to be available after login. + $this->session->set('oidc_client_id', $clientId); + $this->session->set('oidc_state', $state); + $this->session->set('oidc_response_type', $responseType); + $this->session->set('oidc_redirect_uri', $redirectUri); + $this->session->set('oidc_scope', $scope); + $this->session->set('oidc_nonce', $nonce); + $this->session->set('oidc_resource', $resource); + $this->session->set('oidc_code_challenge', $codeChallenge); + $this->session->set('oidc_code_challenge_method', $codeChallengeMethod); + $this->session->set('oidc_prompt', $prompt); + $this->session->set('oidc_max_age', $maxAge); + $this->session->set('oidc_login_pending', true); + + $afterLoginRedirectUrl = $this->urlGenerator->linkToRoute('oidc.Page.index', array_filter([ + 'client_id' => $clientId, + 'state' => $state, + 'response_type' => $responseType, + 'redirect_uri' => $redirectUri, + 'scope' => $scope, + 'nonce' => $nonce, + 'resource' => $resource, + 'code_challenge' => $codeChallenge, + 'code_challenge_method' => $codeChallengeMethod, + 'prompt' => $prompt, + 'max_age' => $maxAge, + ], static function ($value): bool { + return $value !== null && $value !== ''; + })); + + $loginUrl = $this->urlGenerator->linkToRoute( + 'core.login.showLoginForm', + [ + 'redirect_url' => $afterLoginRedirectUrl + ] + ); + + $this->session->close(); // Close session to prevent session locking issues during redirect + + $this->logger->debug($logMessage); + + return new RedirectResponse($loginUrl); + } + + private function promptContains(mixed $prompt, string $expectedPrompt): bool + { + if (!is_string($prompt) || trim($prompt) === '') { + return false; + } + + $promptEntries = array_filter(array_map('trim', explode(' ', strtolower($prompt)))); + return in_array(strtolower($expectedPrompt), $promptEntries, true); + } + + private function getOidcAuthenticationTime(): int + { + $storedAuthTime = $this->session->get('oidc_auth_time'); + $loginPending = $this->session->get('oidc_login_pending'); + + if ($loginPending || !$this->isPositiveIntegerLike($storedAuthTime)) { + $storedAuthTime = $this->time->getTime(); + $this->session->set('oidc_auth_time', $storedAuthTime); + } + + if ($loginPending) { + $this->session->remove('oidc_login_pending'); + } + + return (int)$storedAuthTime; + } + + private function maxAgeExceeded(mixed $maxAge, int $authTime): bool + { + if (!$this->isNonNegativeIntegerLike($maxAge)) { + return false; + } + + return $this->time->getTime() > $authTime + (int)$maxAge; + } + + private function isPositiveIntegerLike(mixed $value): bool + { + return $this->isNonNegativeIntegerLike($value) && (int)$value > 0; + } + + private function isNonNegativeIntegerLike(mixed $value): bool + { + if (is_int($value)) { + return $value >= 0; + } + + if (!is_string($value)) { + return false; + } + + $value = trim($value); + return $value !== '' && ctype_digit($value); + } + + private function createAuthorizationErrorRedirect( + string $redirectUri, + string $error, + string $errorDescription, + mixed $state + ): RedirectResponse { + $params = [ + 'error' => $error, + 'error_description' => $errorDescription, + ]; + if ($state !== null && trim((string)$state) !== '') { + $params['state'] = (string)$state; + } + + $separator = str_contains($redirectUri, '?') ? '&' : '?'; + return new RedirectResponse($redirectUri . $separator . http_build_query($params, '', '&', PHP_QUERY_RFC3986)); + } } diff --git a/lib/Controller/OIDCApiController.php b/lib/Controller/OIDCApiController.php index a8189e05..9326ef15 100644 --- a/lib/Controller/OIDCApiController.php +++ b/lib/Controller/OIDCApiController.php @@ -190,23 +190,21 @@ public function getToken( $code = $refresh_token; } - try { - $accessToken = $this->accessTokenMapper->getByCode($code); - } catch (AccessTokenNotFoundException $e) { - $this->logger->info('Could not find access token for code or refresh_token for client id ' . $client_id . '.'); + if ($code === null || trim($code) === '') { + $this->logger->info('Missing code or refresh_token for client id ' . $client_id . '.'); return new JSONResponse([ 'error' => 'invalid_request', - 'error_description' => 'Could not find access token for code or refresh_token.', + 'error_description' => 'Missing code or refresh_token.', ], Http::STATUS_BAD_REQUEST); } try { - $client = $this->clientMapper->getByUid($accessToken->getClientId()); - } catch (ClientNotFoundException $e) { - $this->logger->error('Could not find client for access token. Client id was ' . $client_id . '.'); + $accessToken = $this->accessTokenMapper->getByCode($code); + } catch (AccessTokenNotFoundException $e) { + $this->logger->info('Could not find access token for code or refresh_token for client id ' . $client_id . '.'); return new JSONResponse([ - 'error' => 'invalid_request', - 'error_description' => 'Could not find client for access token.', + 'error' => 'invalid_grant', + 'error_description' => 'Could not find access token for code or refresh_token.', ], Http::STATUS_BAD_REQUEST); } @@ -214,7 +212,7 @@ public function getToken( $this->logger->debug('No client_id in request. Trying to fetch from Authorization Header.'); if (isset($this->request->server['PHP_AUTH_USER'])) { $client_id = $this->request->server['PHP_AUTH_USER']; - $client_secret = $this->request->server['PHP_AUTH_PW']; + $client_secret = $this->request->server['PHP_AUTH_PW'] ?? null; } if (!isset($client_id)) { $this->logger->debug('No client_id in PHP_AUTH_USER superglobal. Trying to fetch from Authorization Header directly.'); @@ -231,18 +229,37 @@ public function getToken( } } + if ($client_id === null || trim($client_id) === '') { + $this->logger->info('Missing client_id in token request.'); + return new JSONResponse([ + 'error' => 'invalid_client', + 'error_description' => 'Missing client_id.', + ], Http::STATUS_BAD_REQUEST); + } + + try { + $client = $this->clientMapper->getByIdentifier($client_id); + } catch (ClientNotFoundException $e) { + $this->logger->info('Client not found. Client id was ' . $client_id . '.'); + return new JSONResponse([ + 'error' => 'invalid_client', + 'error_description' => 'Client not found.', + ], Http::STATUS_BAD_REQUEST); + } + if ($client === null) { + $this->logger->info('Client not found. Client id was ' . $client_id . '.'); + return new JSONResponse([ + 'error' => 'invalid_client', + 'error_description' => 'Client not found.', + ], Http::STATUS_BAD_REQUEST); + } + if ($client->getType() === 'public') { - // Only the client id must match for a public client. Else we don't provide an access token! - if ($client->getClientIdentifier() !== $client_id) { - $this->logger->info('Client not found. Client id was ' . $client_id . '.'); - return new JSONResponse([ - 'error' => 'invalid_client', - 'error_description' => 'Client not found.', - ], Http::STATUS_BAD_REQUEST); - } + // Only the client id must be present for a public client. + $this->logger->debug('Authenticated public client. Client id was ' . $client_id . '.'); } else { // The client id and secret must match. Else we don't provide an access token! - if ($client->getClientIdentifier() !== $client_id || $client->getSecret() !== $client_secret) { + if ($client->getSecret() !== $client_secret) { $this->logger->error('Client authentication failed. Client id was ' . $client_id . '.'); return new JSONResponse([ 'error' => 'invalid_client', @@ -251,6 +268,14 @@ public function getToken( } } + if ($accessToken->getClientId() !== $client->getId()) { + $this->logger->info('Grant is not valid for client id ' . $client_id . '.'); + return new JSONResponse([ + 'error' => 'invalid_grant', + 'error_description' => 'Grant is not valid for this client.', + ], Http::STATUS_BAD_REQUEST); + } + // The client must not be expired if ($client->isDcr() && $this->time->getTime() > ($client->getIssuedAt() + (int)$this->appConfig->getAppValueString(Application::APP_CONFIG_DEFAULT_CLIENT_EXPIRE_TIME, Application::DEFAULT_CLIENT_EXPIRE_TIME))) { $this->logger->warning('Client expired. Client id was ' . $client_id . '.'); diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php index 7e5349d5..43fa0a2a 100644 --- a/lib/Controller/PageController.php +++ b/lib/Controller/PageController.php @@ -73,6 +73,8 @@ public function index() $resource = $this->request->getParam('resource'); $code_challenge = $this->request->getParam('code_challenge'); $code_challenge_method = $this->request->getParam('code_challenge_method'); + $prompt = $this->request->getParam('prompt'); + $max_age = $this->request->getParam('max_age'); } else { $client_id = $this->session->get('oidc_client_id'); $state = $this->session->get('oidc_state'); @@ -83,6 +85,8 @@ public function index() $resource = $this->session->get('oidc_resource'); $code_challenge = $this->session->get('oidc_code_challenge'); $code_challenge_method = $this->session->get('oidc_code_challenge_method'); + $prompt = $this->session->get('oidc_prompt'); + $max_age = $this->session->get('oidc_max_age'); } $parameters = [ @@ -94,7 +98,9 @@ public function index() 'code_challenge' => $code_challenge, 'code_challenge_method' => $code_challenge_method, 'scope' => $scope, - 'resource' => $resource + 'resource' => $resource, + 'prompt' => $prompt, + 'max_age' => $max_age ]; $response = new TemplateResponse('oidc', 'main', $parameters); diff --git a/lib/Controller/UserInfoController.php b/lib/Controller/UserInfoController.php index 1d9b1555..6c601d1f 100644 --- a/lib/Controller/UserInfoController.php +++ b/lib/Controller/UserInfoController.php @@ -265,10 +265,12 @@ public function getInfo(): JSONResponse $names = $this->converter->splitFullName($displayName); $profile = array_merge($profile, [ 'name' => $displayName, - 'family_name' => $names[0], - 'given_name' => $names[1], - 'middle_name' => $names[2] ]); + foreach (['family_name', 'given_name', 'middle_name'] as $index => $claimName) { + if (isset($names[$index]) && trim($names[$index]) !== '') { + $profile[$claimName] = $names[$index]; + } + } } else { $profile = array_merge($profile, ['name' => $user->getDisplayName()]); } diff --git a/lib/Util/DiscoveryGenerator.php b/lib/Util/DiscoveryGenerator.php index c7cc9270..5d2b5a74 100644 --- a/lib/Util/DiscoveryGenerator.php +++ b/lib/Util/DiscoveryGenerator.php @@ -125,6 +125,7 @@ public function generateDiscovery(IRequest $request): JSONResponse $grantTypesSupported = [ 'authorization_code', 'implicit', + 'refresh_token', ]; $acrValuesSupported = [ '0', @@ -217,8 +218,8 @@ public function generateDiscovery(IRequest $request): JSONResponse // 'claims_locales_supported' => , // 'ui_locales_supported' => , // 'claims_parameter_supported' => true, - // 'request_parameter_supported' => true, - // 'request_uri_parameter_supported' => false, + 'request_parameter_supported' => false, + 'request_uri_parameter_supported' => false, // 'require_request_uri_registration' => true, // 'op_policy_uri' => , // 'op_tos_uri' => , diff --git a/lib/Util/JwtGenerator.php b/lib/Util/JwtGenerator.php index e2a709f5..048523c6 100644 --- a/lib/Util/JwtGenerator.php +++ b/lib/Util/JwtGenerator.php @@ -228,10 +228,12 @@ public function generateIdToken(AccessToken $accessToken, Client $client, string $names = $this->converter->splitFullName($displayName); $profile = array_merge($profile, [ 'name' => $displayName, - 'family_name' => $names[0], - 'given_name' => $names[1], - 'middle_name' => $names[2] ]); + foreach (['family_name', 'given_name', 'middle_name'] as $index => $claimName) { + if (isset($names[$index]) && trim($names[$index]) !== '') { + $profile[$claimName] = $names[$index]; + } + } } else { $profile = array_merge($profile, ['name' => $user->getDisplayName()]); } @@ -427,10 +429,12 @@ public function generateAccessToken(AccessToken $accessToken, Client $client, st $names = $this->converter->splitFullName($displayName); $profile = array_merge($profile, [ 'name' => $displayName, - 'family_name' => $names[0], - 'given_name' => $names[1], - 'middle_name' => $names[2] ]); + foreach (['family_name', 'given_name', 'middle_name'] as $index => $claimName) { + if (isset($names[$index]) && trim($names[$index]) !== '') { + $profile[$claimName] = $names[$index]; + } + } } else { $profile = array_merge($profile, ['name' => $user->getDisplayName()]); } diff --git a/src/Redirect.vue b/src/Redirect.vue index 846d815c..b1074632 100644 --- a/src/Redirect.vue +++ b/src/Redirect.vue @@ -25,6 +25,7 @@ export default { const keys = [ 'client_id', 'state', 'response_type', 'redirect_uri', 'scope', 'nonce', 'resource', 'code_challenge', 'code_challenge_method', + 'prompt', 'max_age', ] keys.forEach(key => { const value = el?.getAttribute('data-' + key.replace(/_/g, '-')) diff --git a/templates/main.php b/templates/main.php index f5e5f763..8967b5df 100644 --- a/templates/main.php +++ b/templates/main.php @@ -13,6 +13,7 @@ data-="" diff --git a/tests/Integration/OIDCCodeFlowTest.php b/tests/Integration/OIDCCodeFlowTest.php index b9e4d7f8..ef5505ea 100644 --- a/tests/Integration/OIDCCodeFlowTest.php +++ b/tests/Integration/OIDCCodeFlowTest.php @@ -97,6 +97,12 @@ class OIDCCodeFlowTest extends \Test\TestCase /** @var string */ private $testClientSecret = 'test-secret'; + /** @var string */ + private $secondTestClientId = 'test-client-2'; + + /** @var string */ + private $secondTestClientSecret = 'test-secret-2'; + /** @var string */ private $testRedirectUri = 'https://client.example.com/callback'; @@ -244,14 +250,16 @@ protected function tearDown(): void private function cleanupTestData(): void { - try { - // Delete the test client (this will cascade and delete related redirect URIs and access tokens) - $client = $this->clientMapper->getByIdentifier($this->testClientId); - if ($client !== null) { - $this->clientMapper->delete($client); + foreach ([$this->testClientId, $this->secondTestClientId] as $clientId) { + try { + // Delete the test client (this will cascade and delete related redirect URIs and access tokens) + $client = $this->clientMapper->getByIdentifier($clientId); + if ($client !== null) { + $this->clientMapper->delete($client); + } + } catch (\Exception $e) { + // Ignore errors during cleanup } - } catch (\Exception $e) { - // Ignore errors during cleanup } try { @@ -265,11 +273,15 @@ private function cleanupTestData(): void } } - private function createTestClient(): Client + private function createTestClient( + string|null $clientId = null, + string|null $clientSecret = null, + string $name = 'Test Client' + ): Client { // Create client - the redirect URIs will be automatically created by ClientMapper $client = new Client( - 'Test Client', + $name, [$this->testRedirectUri], 'RS256', 'confidential', @@ -279,8 +291,8 @@ private function createTestClient(): Client '', false ); - $client->setClientIdentifier($this->testClientId); - $client->setSecret($this->testClientSecret); + $client->setClientIdentifier($clientId ?? $this->testClientId); + $client->setSecret($clientSecret ?? $this->testClientSecret); $insertedClient = $this->clientMapper->insert($client); @@ -437,6 +449,39 @@ public function testTokenExchangeWithInvalidClient(): void $this->assertEquals('invalid_client', $responseData['error'], 'Error should be invalid_client'); } + /** + * Test refresh token use with a different authenticated client + */ + public function testRefreshTokenForDifferentClientReturnsInvalidGrant(): void + { + $this->createTestClient(); + $secondClient = $this->createTestClient( + $this->secondTestClientId, + $this->secondTestClientSecret, + 'Second Test Client' + ); + $user = $this->createTestUser(); + + $tokenResult = $this->createAccessToken($secondClient, $user, 'openid offline_access'); + $refreshToken = $tokenResult['rawCode']; + + $response = $this->oidcApiController->getToken( + 'refresh_token', + null, + $refreshToken, + $this->testClientId, + $this->testClientSecret, + null + ); + + $this->assertInstanceOf(JSONResponse::class, $response, 'Response is not a JSONResponse'); + $this->assertEquals(400, $response->getStatus(), 'Token endpoint should reject refresh tokens from another client'); + + $responseData = $response->getData(); + $this->assertArrayHasKey('error', $responseData, 'Response missing error field'); + $this->assertEquals('invalid_grant', $responseData['error'], 'Error should be invalid_grant'); + } + /** * Test user info with invalid access token */ diff --git a/tests/Unit/Controller/DiscoveryControllerTest.php b/tests/Unit/Controller/DiscoveryControllerTest.php index f2adb004..110daafd 100644 --- a/tests/Unit/Controller/DiscoveryControllerTest.php +++ b/tests/Unit/Controller/DiscoveryControllerTest.php @@ -112,6 +112,7 @@ public function testDiscoveryResponse() { $grantTypesSupported = [ 'authorization_code', 'implicit', + 'refresh_token', ]; $acrValuesSupported = [ '0', @@ -190,6 +191,8 @@ public function testDiscoveryResponse() { $this->assertEquals($displayValuesSupported, $result->getData()['display_values_supported']); $this->assertEquals($claimTypesSupported, $result->getData()['claim_types_supported']); $this->assertEquals($claimsSupported, $result->getData()['claims_supported']); + $this->assertFalse($result->getData()['request_parameter_supported']); + $this->assertFalse($result->getData()['request_uri_parameter_supported']); } diff --git a/tests/Unit/Controller/LoginRedirectorControllerTest.php b/tests/Unit/Controller/LoginRedirectorControllerTest.php index 0ceaed9f..3f175a12 100644 --- a/tests/Unit/Controller/LoginRedirectorControllerTest.php +++ b/tests/Unit/Controller/LoginRedirectorControllerTest.php @@ -28,9 +28,11 @@ use OC\Authentication\Token\IProvider; use OC\Security\SecureRandom; +use OCA\OIDCIdentityProvider\AppInfo\Application; use OCA\OIDCIdentityProvider\Db\ClientMapper; use OCA\OIDCIdentityProvider\Db\AccessTokenMapper; use OCA\OIDCIdentityProvider\Controller\LoginRedirectorController; +use OCA\OIDCIdentityProvider\Db\AccessToken; use OCA\OIDCIdentityProvider\Db\Client; use OCA\OIDCIdentityProvider\Db\GroupMapper; use OCA\OIDCIdentityProvider\Db\RedirectUri; @@ -256,4 +258,509 @@ function ($arg1, $arg2) { $this->assertEquals('http://oidc.local/login-form', $result->getRedirectURL()); } + public function testAuthorizePromptNoneNotLoggedInReturnsLoginRequired() { + $clientId = 'client1'; + $state = 'state-1'; + $redirectUri = 'https://client.example.com/callback'; + + $client = new Client( + 'Test Client', + [$redirectUri], + 'RS256', + 'confidential', + 'code', + 'opaque', + 'openid', + '', + false + ); + $client->id = 1; + $client->setClientIdentifier($clientId); + + $registeredRedirectUri = new RedirectUri(); + $registeredRedirectUri->setClientId(1); + $registeredRedirectUri->setRedirectUri($redirectUri); + + $this->userSession + ->method('isLoggedIn') + ->willReturn(false); + $this->session + ->expects($this->never()) + ->method('set'); + $this->urlGenerator + ->expects($this->never()) + ->method('linkToRoute'); + $this->clientMapper + ->method('getByIdentifier') + ->with($clientId) + ->willReturn($client); + $this->redirectUriMapper + ->method('getByClientId') + ->with(1) + ->willReturn([$registeredRedirectUri]); + $this->accessTokenMapper + ->expects($this->never()) + ->method('insert'); + + $result = $this->controller->authorize( + $clientId, + $state, + 'code', + $redirectUri, + 'openid', + 'nonce-1', + null, + null, + null, + 'none' + ); + + $this->assertEquals(Http::STATUS_SEE_OTHER, $result->getStatus(), 'Status Code does not match!'); + $this->assertEquals( + $redirectUri . '?error=login_required&error_description=User%20is%20not%20logged%20in.&state=state-1', + $result->getRedirectURL() + ); + } + + public function testAuthorizeUsesStoredOidcAuthenticationTimeForAccessToken() { + $clientId = 'client1'; + $state = 'state-1'; + $redirectUri = 'https://client.example.com/callback'; + $authTime = 1234567890; + + $client = new Client( + 'Test Client', + [$redirectUri], + 'RS256', + 'confidential', + 'code', + 'opaque', + 'openid', + '', + false + ); + $client->id = 1; + $client->setClientIdentifier($clientId); + + $registeredRedirectUri = new RedirectUri(); + $registeredRedirectUri->setClientId(1); + $registeredRedirectUri->setRedirectUri($redirectUri); + + $user = $this->createMock(\OCP\IUser::class); + $user + ->method('getUID') + ->willReturn('testuser'); + + $jwtGenerator = $this->createMock(JwtGenerator::class); + $jwtGenerator + ->method('generateAccessToken') + ->willReturn('access-token'); + + $controller = new LoginRedirectorController( + 'oidc', + $this->request, + $this->urlGenerator, + $this->clientMapper, + $this->groupMapper, + $this->secureRandom, + $this->session, + $this->l, + $this->time, + $this->userSession, + $this->groupManager, + $this->accessTokenMapper, + $this->redirectUriMapper, + $this->userConsentMapper, + $this->appConfig, + $jwtGenerator, + $this->redirectUriService, + $this->logger + ); + + $this->userSession + ->method('isLoggedIn') + ->willReturn(true); + $this->userSession + ->method('getUser') + ->willReturn($user); + $this->session + ->method('get') + ->willReturnCallback(function ($key) use ($authTime) { + $values = [ + 'oidc_auth_time' => $authTime, + 'oidc_login_pending' => false, + ]; + return $values[$key] ?? null; + }); + $this->request + ->method('getServerProtocol') + ->willReturn('https'); + $this->request + ->method('getServerHost') + ->willReturn('server.example.com'); + $this->clientMapper + ->method('getByIdentifier') + ->with($clientId) + ->willReturn($client); + $this->redirectUriMapper + ->method('getByClientId') + ->with(1) + ->willReturn([$registeredRedirectUri]); + $this->groupMapper + ->method('getGroupsByClientId') + ->with(1) + ->willReturn([]); + $this->groupManager + ->method('getUserGroups') + ->with($user) + ->willReturn([]); + $this->userConsentMapper + ->method('findByUserAndClient') + ->with('testuser', 1) + ->willReturn(null); + $this->appConfig + ->method('getAppValueString') + ->willReturnCallback(function ($key, $default = '') { + if ($key === Application::APP_CONFIG_ALLOW_USER_SETTINGS) { + return 'no'; + } + return $default; + }); + $this->accessTokenMapper + ->expects($this->once()) + ->method('insert') + ->with($this->callback(function (AccessToken $accessToken) use ($authTime) { + return $accessToken->getCreated() === $authTime; + })); + + $result = $controller->authorize( + $clientId, + $state, + 'code', + $redirectUri, + 'openid', + 'nonce-1', + null, + null, + null, + null, + null + ); + + $this->assertEquals(Http::STATUS_SEE_OTHER, $result->getStatus(), 'Status Code does not match!'); + $this->assertStringStartsWith($redirectUri . '?state=state-1&code=', $result->getRedirectURL()); + } + + public function testAuthorizeMaxAgeExceededForcesReauthentication() { + $clientId = 'client1'; + $state = 'state-1'; + $redirectUri = 'https://client.example.com/callback'; + + $client = new Client( + 'Test Client', + [$redirectUri], + 'RS256', + 'confidential', + 'code', + 'opaque', + 'openid', + '', + false + ); + $client->id = 1; + $client->setClientIdentifier($clientId); + + $registeredRedirectUri = new RedirectUri(); + $registeredRedirectUri->setClientId(1); + $registeredRedirectUri->setRedirectUri($redirectUri); + + $time = $this->createMock(ITimeFactory::class); + $time + ->method('getTime') + ->willReturn(2000); + + $controller = new LoginRedirectorController( + 'oidc', + $this->request, + $this->urlGenerator, + $this->clientMapper, + $this->groupMapper, + $this->secureRandom, + $this->session, + $this->l, + $time, + $this->userSession, + $this->groupManager, + $this->accessTokenMapper, + $this->redirectUriMapper, + $this->userConsentMapper, + $this->appConfig, + $this->jwtGenerator, + $this->redirectUriService, + $this->logger + ); + + $this->userSession + ->method('isLoggedIn') + ->willReturn(true); + $this->userSession + ->expects($this->once()) + ->method('logout'); + $this->session + ->method('get') + ->willReturnCallback(function ($key) { + $values = [ + 'oidc_auth_time' => 1000, + 'oidc_login_pending' => false, + ]; + return $values[$key] ?? null; + }); + $this->session + ->expects($this->atLeastOnce()) + ->method('set'); + $this->clientMapper + ->method('getByIdentifier') + ->with($clientId) + ->willReturn($client); + $this->redirectUriMapper + ->method('getByClientId') + ->with(1) + ->willReturn([$registeredRedirectUri]); + $this->urlGenerator + ->method('linkToRoute') + ->willReturnCallback(function ($route) { + if ($route === 'oidc.Page.index') { + return '/index.php/apps/oidc/redirect?client_id=client1'; + } + if ($route === 'core.login.showLoginForm') { + return '/index.php/login?redirect_url=/index.php/apps/oidc/redirect?client_id=client1'; + } + return '/unexpected'; + }); + $this->accessTokenMapper + ->expects($this->never()) + ->method('insert'); + + $result = $controller->authorize( + $clientId, + $state, + 'code', + $redirectUri, + 'openid', + 'nonce-1', + null, + null, + null, + null, + '1' + ); + + $this->assertEquals(Http::STATUS_SEE_OTHER, $result->getStatus(), 'Status Code does not match!'); + $this->assertEquals( + '/index.php/login?redirect_url=/index.php/apps/oidc/redirect?client_id=client1', + $result->getRedirectURL() + ); + } + + public function testAuthorizePromptLoginForcesReauthentication() { + $clientId = 'client1'; + $state = 'state-1'; + $redirectUri = 'https://client.example.com/callback'; + + $client = new Client( + 'Test Client', + [$redirectUri], + 'RS256', + 'confidential', + 'code', + 'opaque', + 'openid', + '', + false + ); + $client->id = 1; + $client->setClientIdentifier($clientId); + + $registeredRedirectUri = new RedirectUri(); + $registeredRedirectUri->setClientId(1); + $registeredRedirectUri->setRedirectUri($redirectUri); + + $this->userSession + ->method('isLoggedIn') + ->willReturn(true); + $this->userSession + ->expects($this->once()) + ->method('logout'); + $this->session + ->method('get') + ->willReturnCallback(function ($key) { + $values = [ + 'oidc_auth_time' => 2000, + 'oidc_login_pending' => false, + ]; + return $values[$key] ?? null; + }); + $this->session + ->expects($this->atLeastOnce()) + ->method('set'); + $this->clientMapper + ->method('getByIdentifier') + ->with($clientId) + ->willReturn($client); + $this->redirectUriMapper + ->method('getByClientId') + ->with(1) + ->willReturn([$registeredRedirectUri]); + $this->urlGenerator + ->method('linkToRoute') + ->willReturnCallback(function ($route) { + if ($route === 'oidc.Page.index') { + return '/index.php/apps/oidc/redirect?client_id=client1&prompt=login'; + } + if ($route === 'core.login.showLoginForm') { + return '/index.php/login?redirect_url=/index.php/apps/oidc/redirect?client_id=client1&prompt=login'; + } + return '/unexpected'; + }); + $this->accessTokenMapper + ->expects($this->never()) + ->method('insert'); + + $result = $this->controller->authorize( + $clientId, + $state, + 'code', + $redirectUri, + 'openid', + 'nonce-1', + null, + null, + null, + 'login' + ); + + $this->assertEquals(Http::STATUS_SEE_OTHER, $result->getStatus(), 'Status Code does not match!'); + $this->assertEquals( + '/index.php/login?redirect_url=/index.php/apps/oidc/redirect?client_id=client1&prompt=login', + $result->getRedirectURL() + ); + } + + public function testAuthorizeRejectsUnsupportedRequestObject() { + $clientId = 'client1'; + $state = 'state-1'; + $redirectUri = 'https://client.example.com/callback'; + + $client = new Client( + 'Test Client', + [$redirectUri], + 'RS256', + 'confidential', + 'code', + 'opaque', + 'openid', + '', + false + ); + $client->id = 1; + $client->setClientIdentifier($clientId); + + $registeredRedirectUri = new RedirectUri(); + $registeredRedirectUri->setClientId(1); + $registeredRedirectUri->setRedirectUri($redirectUri); + + $this->userSession + ->method('isLoggedIn') + ->willReturn(true); + $this->clientMapper + ->method('getByIdentifier') + ->with($clientId) + ->willReturn($client); + $this->redirectUriMapper + ->method('getByClientId') + ->with(1) + ->willReturn([$registeredRedirectUri]); + $this->request + ->method('getParam') + ->willReturnCallback(function ($key) { + return $key === 'request' ? 'eyJhbGciOiJub25lIn0.e30.' : null; + }); + + $result = $this->controller->authorize( + $clientId, + $state, + 'code', + $redirectUri, + 'openid', + 'nonce-1' + ); + + $this->assertEquals(Http::STATUS_SEE_OTHER, $result->getStatus(), 'Status Code does not match!'); + $this->assertEquals( + $redirectUri . '?error=request_not_supported&error_description=Request%20object%20parameter%20is%20not%20supported.&state=state-1', + $result->getRedirectURL() + ); + } + + public function testAuthorizeRejectsUnsupportedRequestObjectBeforeLogin() { + $clientId = 'client1'; + $redirectUri = 'https://client.example.com/callback'; + + $client = new Client( + 'Test Client', + [$redirectUri], + 'RS256', + 'confidential', + 'code', + 'opaque', + 'openid', + '', + false + ); + $client->id = 1; + $client->setClientIdentifier($clientId); + + $registeredRedirectUri = new RedirectUri(); + $registeredRedirectUri->setClientId(1); + $registeredRedirectUri->setRedirectUri($redirectUri); + + $this->userSession + ->expects($this->never()) + ->method('isLoggedIn'); + $this->session + ->expects($this->never()) + ->method('set'); + $this->urlGenerator + ->expects($this->never()) + ->method('linkToRoute'); + $this->clientMapper + ->method('getByIdentifier') + ->with($clientId) + ->willReturn($client); + $this->redirectUriMapper + ->method('getByClientId') + ->with(1) + ->willReturn([$registeredRedirectUri]); + $this->request + ->method('getParam') + ->willReturnCallback(function ($key) { + return $key === 'request' ? 'eyJhbGciOiJub25lIn0.e30.' : null; + }); + + $result = $this->controller->authorize( + $clientId, + null, + 'code', + $redirectUri, + 'openid', + null + ); + + $this->assertEquals(Http::STATUS_SEE_OTHER, $result->getStatus(), 'Status Code does not match!'); + $this->assertEquals( + $redirectUri . '?error=request_not_supported&error_description=Request%20object%20parameter%20is%20not%20supported.', + $result->getRedirectURL() + ); + } + } diff --git a/tests/Unit/Controller/UserInfoControllerTest.php b/tests/Unit/Controller/UserInfoControllerTest.php index 05b494fe..405c3f3a 100644 --- a/tests/Unit/Controller/UserInfoControllerTest.php +++ b/tests/Unit/Controller/UserInfoControllerTest.php @@ -390,6 +390,8 @@ public function testGetInfoSuccess() { $this->assertEquals('user1', $data['preferred_username']); $this->assertArrayHasKey('scope', $data); $this->assertEquals('openid profile email', $data['scope']); + $this->assertEquals('Test User', $data['name']); + $this->assertArrayNotHasKey('middle_name', $data); } public function testGetInfoPostSuccess() { diff --git a/tests/Unit/Util/DiscoveryGeneratorTest.php b/tests/Unit/Util/DiscoveryGeneratorTest.php index 02224731..3436afc1 100644 --- a/tests/Unit/Util/DiscoveryGeneratorTest.php +++ b/tests/Unit/Util/DiscoveryGeneratorTest.php @@ -155,7 +155,7 @@ public function testGenerateDiscoveryHasGrantTypesSupported() { $this->assertArrayHasKey('grant_types_supported', $data); $grantTypes = $data['grant_types_supported']; - $expectedTypes = ['authorization_code', 'implicit']; + $expectedTypes = ['authorization_code', 'implicit', 'refresh_token']; foreach ($expectedTypes as $type) { $this->assertContains($type, $grantTypes, "Missing grant type: $type"); } @@ -208,6 +208,16 @@ public function testGenerateDiscoveryHasPkceSupport() { $this->assertContains('plain', $methods); } + public function testGenerateDiscoveryDoesNotAdvertiseRequestObjectSupport() { + $result = $this->generator->generateDiscovery($this->request); + $data = $result->getData(); + + $this->assertArrayHasKey('request_parameter_supported', $data); + $this->assertFalse($data['request_parameter_supported']); + $this->assertArrayHasKey('request_uri_parameter_supported', $data); + $this->assertFalse($data['request_uri_parameter_supported']); + } + public function testGenerateDiscoveryHasIntrospectionEndpoint() { $result = $this->generator->generateDiscovery($this->request); $data = $result->getData(); diff --git a/tests/Unit/Util/JwtGeneratorTest.php b/tests/Unit/Util/JwtGeneratorTest.php index 1288bbf6..405777b7 100644 --- a/tests/Unit/Util/JwtGeneratorTest.php +++ b/tests/Unit/Util/JwtGeneratorTest.php @@ -217,13 +217,19 @@ public function testGenerateIdToken() { $mockAccountProperty = $this->createMock(IAccountProperty::class); $mockAccountProperty->method('getValue')->willReturn(''); + $mockDisplayNameProperty = $this->createMock(IAccountProperty::class); + $mockDisplayNameProperty->method('getValue')->willReturn('Test User'); + // Special handling for email property $mockEmailProperty = $this->createMock(IAccountProperty::class); $mockEmailProperty->method('getValue')->willReturn($testEmail); $mockAccount ->method('getProperty') - ->willReturnCallback(function($prop) use ($mockEmailProperty, $mockAccountProperty) { + ->willReturnCallback(function($prop) use ($mockDisplayNameProperty, $mockEmailProperty, $mockAccountProperty) { + if ($prop === IAccountManager::PROPERTY_DISPLAYNAME) { + return $mockDisplayNameProperty; + } if ($prop === IAccountManager::PROPERTY_EMAIL) { return $mockEmailProperty; } @@ -287,6 +293,8 @@ public function testGenerateIdToken() { $this->assertEquals($client->getClientIdentifier(), $decodedJwt['aud']); $this->assertEquals($scope, $decodedJwt['scope']); $this->assertEquals($client->getClientIdentifier(), $decodedJwt['azp']); + $this->assertEquals('Test User', $decodedJwt['name']); + $this->assertArrayNotHasKey('middle_name', $decodedJwt); $this->assertArrayHasKey('email', $decodedJwt); $this->assertEquals('testuser@example.com', $decodedJwt['email']); $this->assertArrayHasKey('nonce', $decodedJwt);