From 05f397026a062d385e65c87904a948a052b2ddaf Mon Sep 17 00:00:00 2001
From: Emmanuel Ormancey <emmanuel.ormancey@cern.ch>
Date: Tue, 14 Jun 2022 13:24:33 +0200
Subject: [PATCH] [#72] Improve Auth API call for group members
---
Makefile | 24 ++-
.../authorization_service.py | 67 +++++--
notifications_routing/config.py | 3 +-
.../postgres/postgres_data_source.py | 13 +-
tests/unit/test_authorization_service.py | 178 ++++++++++++++++--
tests/unit/test_postgres_data_source.py | 14 +-
6 files changed, 234 insertions(+), 65 deletions(-)
diff --git a/Makefile b/Makefile
index 99e7563..7649784 100644
--- a/Makefile
+++ b/Makefile
@@ -33,27 +33,27 @@ ci-test: docker-build-test pytest
test: stop-test docker-build-test pytest
.PHONY: test
-docker-build-env:
+build-env:
docker-compose -f docker-compose.full.yml up --remove-orphans
-.PHONY: docker-build-env
+.PHONY: build-env
-docker-rebuild-env:
+rebuild-env:
docker-compose -f docker-compose.full.yml build --no-cache --parallel
-.PHONY: docker-build-env
+.PHONY: rebuild-env
-docker-build-env-local:
+build-env-local:
docker-compose -f docker-compose.local.yml build
docker-compose -f docker-compose.local.yml up --remove-orphans
-.PHONY: docker-build-env-local
+.PHONY: build-env-local
-docker-shell-env:
+shell-env:
docker-compose -f docker-compose.full.yml exec notifications-routing /bin/bash
-.PHONY: docker-shell-env
+.PHONY: shell-env
-docker-stop:
+stop:
docker-compose -f docker-compose.full.yml down --volumes
docker-compose -f docker-compose.full.yml rm -f
-.PHONY: docker-stop
+.PHONY: stop
stop-test:
docker-compose -f docker-compose.test.yml down --volumes
@@ -66,3 +66,7 @@ job:
shell-job:
docker-compose -f docker-compose.job.yml run notifications-job-daily-feed /bin/bash
.PHONY: shell-job
+
+logs:
+ docker-compose -f docker-compose.full.yml logs -f
+.PHONY: logs
diff --git a/notifications_routing/authorization_service.py b/notifications_routing/authorization_service.py
index e54a482..f6f4bde 100644
--- a/notifications_routing/authorization_service.py
+++ b/notifications_routing/authorization_service.py
@@ -1,9 +1,12 @@
"""Authorization Service handling."""
import json
+import logging
+from typing import Dict, Optional
import requests
from notifications_routing.config import Config
+from notifications_routing.data_source.data_source import DataSource
from .exceptions import BadResponseCodeError
@@ -20,27 +23,63 @@ def get_auth_access_token():
return token
+def _get_group_users(group_id: str, headers: Dict[str, str], url: str = None):
+ if not url:
+ url = f"{Config.CERN_GROUP_URL}/{group_id}/{Config.CERN_GROUP_QUERY}"
+
+ url = f"{Config.CERN_AUTH_SERVICE_URL}{url}"
+ r = requests.get(url, headers=headers)
+ if r.status_code != requests.codes.ok:
+ raise BadResponseCodeError(url, status_code=r.status_code)
+
+ return json.loads(r.content)
+
+
def get_group_users_api(group_id: str):
"""Get members of a group."""
token = get_auth_access_token()
headers = {"Authorization": "Bearer {}".format(token)}
- def _get_group_users(url: str = None):
- if not url:
- url = f"{Config.CERN_GROUP_URL}/{group_id}/{Config.CERN_GROUP_QUERY}"
-
- url = f"{Config.CERN_AUTH_SERVICE_URL}{url}"
- r = requests.get(url, headers=headers)
- if r.status_code != requests.codes.ok:
- raise BadResponseCodeError(url, status_code=r.status_code)
-
- return json.loads(r.content)
-
- response = _get_group_users()
+ response = _get_group_users(group_id, headers)
data = response["data"]
while response.get("pagination").get("next"):
- response = _get_group_users(response["pagination"]["next"])
+ response = _get_group_users(group_id, headers, response["pagination"]["next"])
data.extend(response["data"])
- return data
+ group_users = []
+ for member in data:
+ user = _prepare_user(member)
+
+ # Should Audit failures to failed_targets
+ if user:
+ group_users.append(user)
+
+ return group_users
+
+
+def _prepare_user(member: Dict[str, str]) -> Optional[Dict[str, str]]:
+ """Prepare user data (username and email) according to type and available fields."""
+ if member["type"] == "Application":
+ return None
+
+ if member["unconfirmed"]:
+ return {DataSource.USERNAME: member["unconfirmedEmail"], DataSource.EMAIL: member["unconfirmedEmail"]}
+
+ # always respect "activeUser"
+ # activeUser field is omitted in Application and Service types
+ if "activeUser" in member and member["activeUser"] is False:
+ if member["externalEmail"]:
+ return {DataSource.USERNAME: member["upn"], DataSource.EMAIL: member["externalEmail"]}
+ # member is not active and we don't active an externalEmail
+ return None
+
+ if member["primaryAccountEmail"]:
+ return {DataSource.USERNAME: member["upn"], DataSource.EMAIL: member["primaryAccountEmail"]}
+
+ # some secondary accounts don't have primaryAccountEmail
+ if member["type"] == "Secondary":
+ return {DataSource.USERNAME: member["upn"], DataSource.EMAIL: member["upn"] + "@cern.ch"}
+
+ logging.error("Could not extract user info: %s", member)
+ return None
diff --git a/notifications_routing/config.py b/notifications_routing/config.py
index dc52ba5..086da2d 100644
--- a/notifications_routing/config.py
+++ b/notifications_routing/config.py
@@ -33,7 +33,8 @@ class Config:
CERN_GROUP_URL = os.getenv("CERN_GROUP_URL", "/api/v1.0/Group")
CERN_GROUP_QUERY = os.getenv(
"CERN_GROUP_QUERY",
- "memberidentities/precomputed?field=upn&field=primaryAccountEmail" "&field=unconfirmed&field=unconfirmedEmail",
+ "memberidentities/precomputed?field=upn&field=primaryAccountEmail"
+ "&field=unconfirmed&field=unconfirmedEmail&field=type&field=externalEmail&field=activeUser",
)
# DB
diff --git a/notifications_routing/data_source/postgres/postgres_data_source.py b/notifications_routing/data_source/postgres/postgres_data_source.py
index 14f9264..6a06435 100644
--- a/notifications_routing/data_source/postgres/postgres_data_source.py
+++ b/notifications_routing/data_source/postgres/postgres_data_source.py
@@ -169,18 +169,7 @@ class PostgresDataSource(DataSource):
:return: A list of user data in the format of dicts containing username and email
:raises: BadResponseCodeError
"""
- group_users = []
-
- members = get_group_users_api(group_id)
- for member in members:
- if member["unconfirmed"]:
- user = {DataSource.USERNAME: member["unconfirmedEmail"], DataSource.EMAIL: member["unconfirmedEmail"]}
- else:
- user = {DataSource.USERNAME: member["upn"], DataSource.EMAIL: member["primaryAccountEmail"]}
-
- group_users.append(user)
-
- return group_users
+ return get_group_users_api(group_id)
def get_user_preferences(self, user_id: str, channel_id: str, **kwargs) -> Dict[str, List["Preference"]]:
"""Return the list of user preferences.
diff --git a/tests/unit/test_authorization_service.py b/tests/unit/test_authorization_service.py
index f6118ce..ab3e245 100644
--- a/tests/unit/test_authorization_service.py
+++ b/tests/unit/test_authorization_service.py
@@ -1,5 +1,6 @@
"""Unit Tests for the Auth service."""
import json
+import logging
from unittest import mock
from unittest.mock import call
@@ -31,7 +32,9 @@ def auth_user_1():
"upn": "user1",
"unconfirmed": False,
"unconfirmedEmail": None,
- "id": "dcb3e437-a27e-4193-a2b9-ba4eea55f315",
+ "externalEmail": None,
+ "type": "Person",
+ "activeUser": True,
}
@@ -43,42 +46,185 @@ def auth_user_2():
"upn": "user2",
"unconfirmed": False,
"unconfirmedEmail": None,
- "id": "dcb3e437-a27e-4193-a2b9-ba4eea55f315",
+ "externalEmail": None,
+ "type": "Person",
+ "activeUser": True,
}
@pytest.fixture(scope="function")
-def auth_user_3():
+def auth_user_data_inconsistent():
"""Auth user dict."""
return {
- "primaryAccountEmail": "user3@cern.ch",
- "upn": "user3",
+ "primaryAccountEmail": "inconsistent@cern.ch",
+ "upn": "inconsistent",
+ "unconfirmed": False,
+ "unconfirmedEmail": None,
+ "type": "Person",
+ "externalEmail": None,
+ # some times data is not consistent and activeUser is not filled
+ # "activeUser": True
+ }
+
+
+@pytest.fixture(scope="function")
+def auth_user_data_inconsistent_error():
+ """Auth user dict."""
+ return {
+ "primaryAccountEmail": None,
+ "upn": "missing email",
+ "unconfirmed": False,
+ "unconfirmedEmail": None,
+ "type": "Person",
+ "externalEmail": None,
+ "activeUser": True,
+ }
+
+
+@pytest.fixture(scope="function")
+def auth_group_user_unconfirmed():
+ """Auth user dict."""
+ return {
+ "externalEmail": None,
+ "primaryAccountEmail": None,
+ "type": "Person",
+ "upn": None,
+ "unconfirmed": True,
+ "unconfirmedEmail": "unconfirmed@hotmail.fr",
+ }
+
+
+@pytest.fixture(scope="function")
+def auth_group_user_application():
+ """Auth user dict."""
+ return {
+ "primaryAccountEmail": None,
+ "upn": "application-test",
+ "unconfirmed": False,
+ "unconfirmedEmail": None,
+ "type": "Application",
+ "externalEmail": None,
+ }
+
+
+@pytest.fixture(scope="function")
+def auth_group_user_service():
+ """Auth user dict."""
+ return {
+ "primaryAccountEmail": "service-email@cern.ch",
+ "upn": "service",
+ "unconfirmed": False,
+ "unconfirmedEmail": None,
+ "type": "Service",
+ "externalEmail": None,
+ }
+
+
+@pytest.fixture(scope="function")
+def auth_group_user_inactive_1():
+ """Auth user dict."""
+ return {
+ "externalEmail": "external@aol.com",
+ "primaryAccountEmail": "inactive1@cern.ch",
+ "type": "Person",
+ "upn": "inactive1",
+ "unconfirmed": False,
+ "unconfirmedEmail": None,
+ "activeUser": False,
+ }
+
+
+@pytest.fixture(scope="function")
+def auth_group_user_inactive_2():
+ """Auth user dict."""
+ return {
+ "externalEmail": None,
+ "primaryAccountEmail": "inactive2@cern.ch",
+ "type": "Person",
+ "upn": "inactive2",
+ "unconfirmed": False,
+ "unconfirmedEmail": None,
+ "activeUser": False,
+ }
+
+
+@pytest.fixture(scope="function")
+def auth_group_user_secondary():
+ """Auth user dict."""
+ return {
+ "externalEmail": None,
+ "primaryAccountEmail": "secondary@cern.ch",
+ "type": "Secondary",
+ "upn": "secondary",
"unconfirmed": False,
"unconfirmedEmail": None,
- "id": "dcb3e437-a27e-4193-a2b9-ba4eea55f315",
}
@mock.patch("notifications_routing.authorization_service.get_auth_access_token")
@mock.patch.object(requests, "get")
-def test_process_users(mock_get, mock_get_token, appctx, auth_user_1, auth_user_2, auth_user_3):
+@mock.patch.object(logging, "error")
+def test_process_users(
+ mock_logging_error,
+ mock_get,
+ mock_get_token,
+ appctx,
+ auth_user_1,
+ auth_user_2,
+ auth_user_data_inconsistent,
+ auth_group_user_inactive_1,
+ auth_group_user_inactive_2,
+ auth_group_user_service,
+ auth_group_user_application,
+ auth_group_user_secondary,
+ auth_group_user_unconfirmed,
+ auth_user_data_inconsistent_error,
+):
"""Test process users for normal."""
mock_get_token.return_value = "jwt"
mock_get.side_effect = [
- MockResponse({"pagination": {"next": "/next/url/page"}, "data": [auth_user_1, auth_user_2]}, 200),
- MockResponse({"pagination": {"next": None}, "data": [auth_user_3]}, 200),
+ MockResponse(
+ {
+ "pagination": {"next": "/next/url/page"},
+ "data": [
+ auth_user_1,
+ auth_user_2,
+ auth_user_data_inconsistent,
+ auth_group_user_inactive_1,
+ auth_group_user_inactive_2, # should be ignored, email is not extractable
+ auth_group_user_service,
+ auth_group_user_application, # should be ignore
+ auth_group_user_secondary,
+ auth_user_data_inconsistent_error,
+ ],
+ },
+ 200,
+ ),
+ MockResponse({"pagination": {"next": None}, "data": [auth_group_user_unconfirmed]}, 200),
]
result = get_group_users_api("group_id")
- assert result == [auth_user_1, auth_user_2, auth_user_3]
+ assert result == [
+ {"username": "user1", "email": "user1@cern.ch"},
+ {"username": "user2", "email": "user2@cern.ch"},
+ {"username": "inconsistent", "email": "inconsistent@cern.ch"},
+ {"username": "inactive1", "email": "external@aol.com"},
+ {"username": "service", "email": "service-email@cern.ch"},
+ {"username": "secondary", "email": "secondary@cern.ch"},
+ {"username": "unconfirmed@hotmail.fr", "email": "unconfirmed@hotmail.fr"},
+ ]
+
+ mock_logging_error.assert_called_once()
mock_get_token.assert_called_once()
expected_first_call_url = (
"https://authorization-service-api.web.cern.ch"
- "/api/v1.0/Group/group_id/memberidentities/precomputed?field=upn&field"
- "=primaryAccountEmail&field=unconfirmed&field=unconfirmedEmail"
+ "/api/v1.0/Group/group_id/memberidentities/precomputed?field=upn&field=primaryAccountEmail"
+ "&field=unconfirmed&field=unconfirmedEmail&field=type&field=externalEmail&field=activeUser"
)
expected_headers = {"Authorization": "Bearer jwt"}
- mock_get.assert_has_calls([
- call(expected_first_call_url, headers=expected_headers),
- call("https://authorization-service-api.web.cern.ch/next/url/page", headers=expected_headers)
- ])
+ mock_get.assert_has_calls(
+ [
+ call(expected_first_call_url, headers=expected_headers),
+ call("https://authorization-service-api.web.cern.ch/next/url/page", headers=expected_headers),
+ ]
+ )
diff --git a/tests/unit/test_postgres_data_source.py b/tests/unit/test_postgres_data_source.py
index c7f70fd..2e3f6ff 100644
--- a/tests/unit/test_postgres_data_source.py
+++ b/tests/unit/test_postgres_data_source.py
@@ -136,18 +136,8 @@ def test_get_channel_unsubscribed_users_(mock_get_scalar, db_mock, channel):
def test_get_group_users(mock_get_group_users_api, db_mock):
"""Test get group users."""
mock_get_group_users_api.return_value = [
- {
- "upn": "testuser",
- "primaryAccountEmail": "testuser@cern.ch",
- "unconfirmed": False,
- "unconfirmedEmail": None,
- },
- {
- "upn": None,
- "primaryAccountEmail": None,
- "unconfirmed": True,
- "unconfirmedEmail": "unconfirmedEmail@mail.ch",
- },
+ {"username": "testuser", "email": "testuser@cern.ch"},
+ {"username": "unconfirmedEmail@mail.ch", "email": "unconfirmedEmail@mail.ch"},
]
assert db_mock.get_group_users("186d8dfc-2774-43a8-91b5-a887fcb6ba4a") == [
--
GitLab