refactor: change OAuth scope separator from colon to dot for IDP compatibility
Many identity providers (AWS Cognito, Okta, Azure AD) reject or mishandle colons in OAuth scope names. This migrates all custom scopes from `resource:action` to `resource.action` format (e.g., `notes:read` → `notes.read`), which is universally accepted and aligns with industry conventions (Microsoft, Google). Includes Alembic migration 004 for stored scope strings and ADR-024 documenting the rationale and RFC references. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
899b9c7191
commit
29fd0486c9
@@ -100,7 +100,7 @@ class TestGetUserAccess:
|
||||
await temp_storage.store_app_password_with_scopes(
|
||||
user_id="alice",
|
||||
app_password="test-app-pw",
|
||||
scopes=["notes:read", "calendar:write"],
|
||||
scopes=["notes.read", "calendar.write"],
|
||||
username="alice_nc",
|
||||
)
|
||||
|
||||
@@ -115,7 +115,7 @@ class TestGetUserAccess:
|
||||
data = resp.json()
|
||||
assert data["success"] is True
|
||||
assert data["provisioned"] is True
|
||||
assert set(data["scopes"]) == {"notes:read", "calendar:write"}
|
||||
assert set(data["scopes"]) == {"notes.read", "calendar.write"}
|
||||
assert data["username"] == "alice_nc"
|
||||
|
||||
async def test_missing_auth_header(self, temp_storage):
|
||||
@@ -146,7 +146,7 @@ class TestUpdateUserScopes:
|
||||
await temp_storage.store_app_password_with_scopes(
|
||||
user_id="alice",
|
||||
app_password="test-app-pw",
|
||||
scopes=["notes:read"],
|
||||
scopes=["notes.read"],
|
||||
username="alice_nc",
|
||||
)
|
||||
|
||||
@@ -156,19 +156,19 @@ class TestUpdateUserScopes:
|
||||
resp = client.patch(
|
||||
"/api/v1/users/alice/scopes",
|
||||
headers={"Authorization": create_basic_auth_header("alice", "pw")},
|
||||
json={"scopes": ["notes:read", "notes:write", "calendar:read"]},
|
||||
json={"scopes": ["notes.read", "notes.write", "calendar.read"]},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
data = resp.json()
|
||||
assert data["success"] is True
|
||||
assert set(data["scopes"]) == {"notes:read", "notes:write", "calendar:read"}
|
||||
assert set(data["scopes"]) == {"notes.read", "notes.write", "calendar.read"}
|
||||
|
||||
async def test_invalid_scopes(self, temp_storage):
|
||||
"""Returns 400 for invalid scope names."""
|
||||
await temp_storage.store_app_password_with_scopes(
|
||||
user_id="alice",
|
||||
app_password="test-app-pw",
|
||||
scopes=["notes:read"],
|
||||
scopes=["notes.read"],
|
||||
)
|
||||
|
||||
app = create_test_app(temp_storage)
|
||||
@@ -177,7 +177,7 @@ class TestUpdateUserScopes:
|
||||
resp = client.patch(
|
||||
"/api/v1/users/alice/scopes",
|
||||
headers={"Authorization": create_basic_auth_header("alice", "pw")},
|
||||
json={"scopes": ["notes:read", "invalid:scope"]},
|
||||
json={"scopes": ["notes.read", "invalid:scope"]},
|
||||
)
|
||||
assert resp.status_code == 400
|
||||
data = resp.json()
|
||||
@@ -192,7 +192,7 @@ class TestUpdateUserScopes:
|
||||
resp = client.patch(
|
||||
"/api/v1/users/alice/scopes",
|
||||
headers={"Authorization": create_basic_auth_header("alice", "pw")},
|
||||
json={"scopes": ["notes:read"]},
|
||||
json={"scopes": ["notes.read"]},
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
data = resp.json()
|
||||
|
||||
@@ -38,14 +38,14 @@ async def test_store_app_password_with_scopes(temp_storage):
|
||||
await temp_storage.store_app_password_with_scopes(
|
||||
user_id="alice",
|
||||
app_password="aaaaa-bbbbb-ccccc-ddddd-eeeee",
|
||||
scopes=["notes:read", "notes:write"],
|
||||
scopes=["notes.read", "notes.write"],
|
||||
username="alice_nc",
|
||||
)
|
||||
|
||||
data = await temp_storage.get_app_password_with_scopes("alice")
|
||||
assert data is not None
|
||||
assert data["app_password"] == "aaaaa-bbbbb-ccccc-ddddd-eeeee"
|
||||
assert data["scopes"] == ["notes:read", "notes:write"]
|
||||
assert data["scopes"] == ["notes.read", "notes.write"]
|
||||
assert data["username"] == "alice_nc"
|
||||
assert data["created_at"] is not None
|
||||
assert data["updated_at"] is not None
|
||||
@@ -70,18 +70,18 @@ async def test_store_app_password_with_scopes_replaces(temp_storage):
|
||||
await temp_storage.store_app_password_with_scopes(
|
||||
user_id="alice",
|
||||
app_password="aaaaa-bbbbb-ccccc-ddddd-eeeee",
|
||||
scopes=["notes:read"],
|
||||
scopes=["notes.read"],
|
||||
)
|
||||
await temp_storage.store_app_password_with_scopes(
|
||||
user_id="alice",
|
||||
app_password="xxxxx-yyyyy-zzzzz-aaaaa-bbbbb",
|
||||
scopes=["notes:read", "calendar:read"],
|
||||
scopes=["notes.read", "calendar.read"],
|
||||
username="alice_nc",
|
||||
)
|
||||
|
||||
data = await temp_storage.get_app_password_with_scopes("alice")
|
||||
assert data["app_password"] == "xxxxx-yyyyy-zzzzz-aaaaa-bbbbb"
|
||||
assert data["scopes"] == ["notes:read", "calendar:read"]
|
||||
assert data["scopes"] == ["notes.read", "calendar.read"]
|
||||
|
||||
|
||||
async def test_get_app_password_with_scopes_nonexistent(temp_storage):
|
||||
@@ -99,14 +99,14 @@ async def test_store_and_get_login_flow_session(temp_storage):
|
||||
user_id="alice",
|
||||
poll_token="secret-poll-token",
|
||||
poll_endpoint="https://cloud.example.com/login/v2/poll",
|
||||
requested_scopes=["notes:read", "notes:write"],
|
||||
requested_scopes=["notes.read", "notes.write"],
|
||||
)
|
||||
|
||||
session = await temp_storage.get_login_flow_session("alice")
|
||||
assert session is not None
|
||||
assert session["poll_token"] == "secret-poll-token"
|
||||
assert session["poll_endpoint"] == "https://cloud.example.com/login/v2/poll"
|
||||
assert session["requested_scopes"] == ["notes:read", "notes:write"]
|
||||
assert session["requested_scopes"] == ["notes.read", "notes.write"]
|
||||
assert session["created_at"] is not None
|
||||
assert session["expires_at"] is not None
|
||||
|
||||
@@ -187,11 +187,11 @@ async def test_delete_expired_login_flow_sessions(temp_storage):
|
||||
|
||||
def test_all_supported_scopes():
|
||||
"""Test that ALL_SUPPORTED_SCOPES contains expected scopes."""
|
||||
assert "notes:read" in ALL_SUPPORTED_SCOPES
|
||||
assert "notes:write" in ALL_SUPPORTED_SCOPES
|
||||
assert "calendar:read" in ALL_SUPPORTED_SCOPES
|
||||
assert "files:read" in ALL_SUPPORTED_SCOPES
|
||||
assert "deck:read" in ALL_SUPPORTED_SCOPES
|
||||
assert "notes.read" in ALL_SUPPORTED_SCOPES
|
||||
assert "notes.write" in ALL_SUPPORTED_SCOPES
|
||||
assert "calendar.read" in ALL_SUPPORTED_SCOPES
|
||||
assert "files.read" in ALL_SUPPORTED_SCOPES
|
||||
assert "deck.read" in ALL_SUPPORTED_SCOPES
|
||||
# Scopes should be in pairs (read/write)
|
||||
read_scopes = [s for s in ALL_SUPPORTED_SCOPES if s.endswith(":read")]
|
||||
write_scopes = [s for s in ALL_SUPPORTED_SCOPES if s.endswith(":write")]
|
||||
|
||||
@@ -29,7 +29,7 @@ async def test_get_stored_scopes_with_scopes():
|
||||
mock_storage = AsyncMock()
|
||||
mock_storage.get_app_password_with_scopes.return_value = {
|
||||
"app_password": "xxxxx",
|
||||
"scopes": ["notes:read", "calendar:read"],
|
||||
"scopes": ["notes.read", "calendar.read"],
|
||||
"username": "alice",
|
||||
"created_at": 1000,
|
||||
"updated_at": 1000,
|
||||
@@ -41,7 +41,7 @@ async def test_get_stored_scopes_with_scopes():
|
||||
):
|
||||
result = await _get_stored_scopes("alice")
|
||||
|
||||
assert result == ["notes:read", "calendar:read"]
|
||||
assert result == ["notes.read", "calendar.read"]
|
||||
|
||||
|
||||
async def test_get_stored_scopes_null_scopes():
|
||||
|
||||
@@ -12,24 +12,24 @@ from nextcloud_mcp_server.auth.scope_authorization import (
|
||||
def test_scope_decorator_stores_metadata():
|
||||
"""Test that @require_scopes decorator stores scope requirements as function metadata."""
|
||||
|
||||
@require_scopes("notes:read", "notes:write")
|
||||
@require_scopes("notes.read", "notes.write")
|
||||
async def example_function():
|
||||
pass
|
||||
|
||||
# Verify metadata is stored
|
||||
assert hasattr(example_function, "_required_scopes")
|
||||
assert example_function._required_scopes == ["notes:read", "notes:write"]
|
||||
assert example_function._required_scopes == ["notes.read", "notes.write"]
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_scope_decorator_with_single_scope():
|
||||
"""Test decorator with a single scope requirement."""
|
||||
|
||||
@require_scopes("calendar:read")
|
||||
@require_scopes("calendar.read")
|
||||
async def example_function():
|
||||
pass
|
||||
|
||||
assert example_function._required_scopes == ["calendar:read"]
|
||||
assert example_function._required_scopes == ["calendar.read"]
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -46,18 +46,18 @@ def test_scope_decorator_with_no_scopes():
|
||||
@pytest.mark.unit
|
||||
def test_insufficient_scope_error():
|
||||
"""Test InsufficientScopeError exception structure."""
|
||||
missing = ["notes:write", "calendar:write"]
|
||||
missing = ["notes.write", "calendar.write"]
|
||||
error = InsufficientScopeError(missing)
|
||||
|
||||
assert error.missing_scopes == missing
|
||||
assert "notes:write" in str(error)
|
||||
assert "calendar:write" in str(error)
|
||||
assert "notes.write" in str(error)
|
||||
assert "calendar.write" in str(error)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_insufficient_scope_error_with_custom_message():
|
||||
"""Test InsufficientScopeError with custom message."""
|
||||
missing = ["files:write"]
|
||||
missing = ["files.write"]
|
||||
custom_msg = "You need more permissions"
|
||||
error = InsufficientScopeError(missing, custom_msg)
|
||||
|
||||
|
||||
@@ -379,7 +379,7 @@ class TestRefreshTokenRotation:
|
||||
expires_in,
|
||||
) = await broker._refresh_access_token_with_scopes(
|
||||
refresh_token="old_refresh_token_123",
|
||||
required_scopes=["notes:read"],
|
||||
required_scopes=["notes.read"],
|
||||
user_id="admin",
|
||||
)
|
||||
|
||||
@@ -424,7 +424,7 @@ class TestRefreshTokenRotation:
|
||||
):
|
||||
await broker._refresh_access_token_with_scopes(
|
||||
refresh_token="same_refresh_token",
|
||||
required_scopes=["notes:read"],
|
||||
required_scopes=["notes.read"],
|
||||
user_id="admin",
|
||||
)
|
||||
|
||||
@@ -460,7 +460,7 @@ class TestRefreshTokenRotation:
|
||||
):
|
||||
await broker._refresh_access_token_with_scopes(
|
||||
refresh_token="old_token",
|
||||
required_scopes=["notes:read"],
|
||||
required_scopes=["notes.read"],
|
||||
user_id=None, # No user_id
|
||||
)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user