PR#57: add the ability to interact with fasjson - fedora-infra/toddlers

fedora-infra / toddlers

#57 add the ability to interact with fasjson

Merged 3 years ago by nphilipp. Opened 3 years ago by scoady.

fedora-infra/ scoady/toddlers add-fasjson into master

add the ability to interact with fasjson

Stephen Coady • 3 years ago

0192006

requirements.txt

file modified

		`@@ -1,6 +1,7 @@`
		`beanbag`
		`bs4`
		`defusedxml`
		`+ fasjson-client`
		`fedora-messaging`
		`koji`
		`requests`

tests/plugins/test_distgit_bugzilla_sync.py

file modified

		`@@ -27,6 +27,7 @@`
		`# distgit_bugzilla_sync config values`
		`"user_notifications": False,`
		`"ignorable_accounts": [],`
		`+ "fasjson": False,`
		`"default_qa_contact": "nurgle@fedoraproject.org",`
		`"notify_admins": ["root@localhost.localdomain"],`
		`"pdc_types": {`

tests/utils/test_fedora_account.py

file modified

+10

		`@@ -141,3 +141,13 @@`
		`"toddlers-sig", {"foo@lists.bar.com": "foo@baz.com"}`
		`)`
		`assert output == "foo@baz.com"`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fas")`
		`+ def test_get_user_by_email(self, mock_fas):`
		`+ user = {"username": "scoady", "emails": ["scoady@fp.o"]}`
		`+ server = Mock()`
		`+ server.people_query.return_value = user`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_user_by_email("scoady@fp.o")`
		`+ assert output == {"username": "scoady", "emails": ["scoady@fp.o"]}`

tests/utils/test_fedora_account_fasjson.py

file added

+185

		`@@ -0,0 +1,185 @@`
		`+ from unittest.mock import Mock, patch`
		`+`
		`+ from fasjson_client.errors import ClientError`
		`+ import pytest`
		`+`
		`+ import toddlers.utils.fedora_account`
		`+`
		`+`
		`+ class TestFedoraAccountFASJSON:`
		`+ def test_set_fas_no_fas_url(self):`
		`+ with pytest.raises(`
		`+ ValueError, match=r"No fas_url found in the configuration file"`
		`+ ):`
		`+ toddlers.utils.fedora_account.set_fasjson({})`
		`+`
		`+ @patch("toddlers.utils.fedora_account.Client")`
		`+ def test_set_fas_json(self, mock_fas):`
		`+ mock_fas.return_value = "fas_object"`
		`+ config = {`
		`+ "fas_url": "https:fas.example.com",`
		`+ "fasjson": True,`
		`+ }`
		`+ output = toddlers.utils.fedora_account.set_fasjson(config)`
		`+ mock_fas.assert_called_with(url="https:fas.example.com")`
		`+ assert output == "fas_object"`
		`+`
		`+ def test_set_fas_json_error(self):`
		`+ with patch(`
		`+ "toddlers.utils.fedora_account.Client",`
		`+ side_effect=ClientError(`
		`+ message="Error communicating with fasjson to instantiate the client",`
		`+ code="404",`
		`+ ),`
		`+ ):`
		`+ with pytest.raises(`
		`+ ClientError,`
		`+ match=r"Error communicating with fasjson to instantiate the client",`
		`+ ):`
		`+ config = {`
		`+ "fas_url": "https:fas.example.com",`
		`+ "fasjson": True,`
		`+ }`
		`+ toddlers.utils.fedora_account.set_fasjson(config)`
		`+`
		`+ @patch("toddlers.utils.fedora_account._FASJSON", new=None)`
		`+ def test_get_fasjson_not_set(self):`
		`+ with pytest.raises(`
		`+ ValueError, match=r"No FASJSON client instantiated, call set_fas first"`
		`+ ):`
		`+ toddlers.utils.fedora_account.get_fasjson()`
		`+`
		`+ def test_get_fasjson(self):`
		`+ output = toddlers.utils.fedora_account.get_fasjson()`
		`+ assert output == "fas_object"`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_group_member(self, mock_fas):`
		`+ members = []`
		`+ for name in ["pingou", "ralph", "kevin", "nils"]:`
		`+ member = Mock()`
		`+ member.role_type = "administrator"`
		`+ member.username = name`
		`+ members.append(member)`
		`+ result = {"result": members}`
		`+ server = Mock()`
		`+ server.list_group_members.return_value = result`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_group_member("sysadmin")`
		`+ assert output == {"kevin", "nils", "pingou", "ralph"}`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_group_member_error(self, mock_fas):`
		`+ server = Mock()`
		`+ server.list_group_members.side_effect = ClientError(`
		`+ message="Error getting list of members",`
		`+ code="500",`
		`+ )`
		`+ mock_fas.return_value = server`
		`+ output = toddlers.utils.fedora_account.get_group_member("sysadmin")`
		`+ assert output == set()`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_group_member_empty(self, mock_fas):`
		`+ members = []`
		`+ server = Mock()`
		`+ result = {"result": members}`
		`+ server.list_group_members.return_value = result`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_group_member("sysadmin")`
		`+ assert output == set()`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_bz_email_user_no_bugzilla_email(self, mock_fas):`
		`+ server = Mock()`
		`+ server.get_user.return_value = {}`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_bz_email_user("pingou", {})`
		`+ assert output is None`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_bz_email_user(self, mock_fas):`
		`+ server = Mock()`
		`+ server.get_user.return_value = {"result": {"emails": ["foo@bar.com"]}}`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_bz_email_user("pingou", {})`
		`+ assert output == "foo@bar.com"`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_bz_email_user_error(self, mock_fas):`
		`+ server = Mock()`
		`+ server.get_user.side_effect = ClientError(`
		`+ message="Error getting bz_email",`
		`+ code="500",`
		`+ )`
		`+ mock_fas.return_value = server`
		`+ output = toddlers.utils.fedora_account.get_bz_email_user("pingou", {})`
		`+ assert output is None`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_bz_email_group_no_bugzilla_email(self, mock_fas):`
		`+ server = Mock()`
		`+ server.get_group.return_value = {}`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_bz_email_group("toddlers-sig", {})`
		`+ assert output is None`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_bz_email_group(self, mock_fas):`
		`+ server = Mock()`
		`+ server.get_group.return_value = {`
		`+ "result": {"mailing_list": "foo@lists.bar.com"}`
		`+ }`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_bz_email_group("toddlers-sig", {})`
		`+ assert output == "foo@lists.bar.com"`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_bz_email_group_error(self, mock_fas):`
		`+ server = Mock()`
		`+ server.get_group.side_effect = ClientError(`
		`+ message="Error getting bz_email for group",`
		`+ code="500",`
		`+ )`
		`+ mock_fas.return_value = server`
		`+ output = toddlers.utils.fedora_account.get_bz_email_group("sysadmin", {})`
		`+ assert output is None`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_user_by_email(self, mock_fas):`
		`+ user = [{"username": "scoady", "emails": ["scoady@fp.o"]}]`
		`+ result = {"result": user}`
		`+ server = Mock()`
		`+ server.search.return_value = result`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_user_by_email("scoady@fp.o")`
		`+ assert output == {"username": "scoady", "emails": ["scoady@fp.o"]}`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_user_by_email_empty(self, mock_fas):`
		`+ user = []`
		`+ result = {"result": user}`
		`+ server = Mock()`
		`+ server.search.return_value = result`
		`+ mock_fas.return_value = server`
		`+`
		`+ output = toddlers.utils.fedora_account.get_user_by_email("scoady@fp.o")`
		`+ assert output is None`
		`+`
		`+ @patch("toddlers.utils.fedora_account.get_fasjson")`
		`+ def test_get_user_by_email_error(self, mock_fas):`
		`+ server = Mock()`
		`+ server.search.side_effect = ClientError(`
		`+ message="Error getting bz_email for group",`
		`+ code="500",`
		`+ )`
		`+ mock_fas.return_value = server`
		`+ output = toddlers.utils.fedora_account.get_user_by_email("scoady@fp.o")`
		`+ assert output is None`

toddlers.toml.example

file modified

		`@@ -69,6 +69,9 @@`
		`mail_server = "bastion.fedoraproject.org"`
		`admin_email = "admin@fedoraproject.org"`

		`+ # Whether or not to use fasjson`
		`+ fasjson = false`
		`+`
		`# Account to use to connect to FAS`
		`fas_url = "https://admin.fedoraproject.org/accounts"`
		`fas_username = "FAS username"`
		`@@ -127,6 +130,8 @@`
		`notify_admins = [`
		`"root@localhost.localdomain",`
		`]`
		`+ # Use fasjson instead of FAS`
		`+ fasjson = true`

		`# PDC types mapping for distgit_bugzilla_sync toddler`
		`[consumer_config.distgit_bugzilla_sync.pdc_types]`

toddlers/plugins/distgit_bugzilla_sync.py

file modified

+4 -1

		`@@ -123,7 +123,10 @@`
		`times["data structure end"] = time.time()`

		`_log.info("Setting up connection to FAS")`
		`- fedora_account.set_fas(config)`
		`+ if config["fasjson"]:`
		`+ fedora_account.set_fasjson(config)`
		`+ else:`
		`+ fedora_account.set_fas(config)`

		`_log.info("Build bugzilla mail cache for users")`
		`bz_mail_to_fas = {}`

toddlers/utils/fedora_account.py

file modified

+87 -15

		`@@ -1,11 +1,14 @@`
		`from typing import Any`
		`from typing import Mapping`

		`+ from fasjson_client import Client`
		`+ from fasjson_client.errors import ClientError`
		`from fedora.client.fas2 import AccountSystem`


		`# Have a global connection to FAS open.`
		`_FAS = None`
		`+ _FASJSON = None`


		`def set_fas(conf: Mapping[str, str]) -> AccountSystem:`
		`@@ -28,10 +31,25 @@`
		`_FAS = AccountSystem(`
		`fas_url, username=fas_user, password=fas_pass, cache_session=False`
		`)`
		`-`
		`return _FAS`


		`+ def set_fasjson(conf: Mapping[str, str]) -> Client:`
		`+ """Set the connection to the Fedora Account System through Fasjson."""`
		`+ global _FASJSON`
		`+`
		`+ fas_url = conf.get("fas_url")`
		`+ if not fas_url:`
		`+ raise ValueError("No fas_url found in the configuration file")`
		`+`
		`+ # create a fasjson client`
		`+ try:`
		`+ _FASJSON = Client(url=conf.get("fas_url"))`
		`+ except ClientError as e:`
		`+ raise ClientError(message="{0!s}".format(e), code=e.code)`
		`+ return _FASJSON`
		`+`
		`+`
		`def get_fas() -> AccountSystem:`
		`"""Retrieve a connection to the Fedora Account System."""`
		`global _FAS`
		`@@ -41,28 +59,58 @@`
		`return _FAS`


		`+ def get_fasjson() -> Client:`
		`+ """Retrieve an instantiated FASJSON Client."""`
		`+ global _FASJSON`
		`+ if _FASJSON is None:`
		`+ raise ValueError("No FASJSON client instantiated, call set_fas first")`
		`+`
		`+ return _FASJSON`
		`+`
		`+`
		`def __get_fas_grp_member(group: str = "packager") -> Mapping[str, Mapping[str, Any]]:`
		`"""Retrieve from FAS the list of users in the packager group."""`
		`- fas = get_fas()`
		`+ global _FASJSON`
		`+`
		`+ if _FASJSON is None:`
		`+ fas = get_fas()`

		`- return fas.group_members(group)`
		`+ return fas.group_members(group)`
		`+ else:`
		`+ fasjson = get_fasjson()`
		`+ try:`
		`+ return fasjson.list_group_members(groupname=group).get("result", [])`
		`+ except ClientError:`
		`+ return {}`


		`def get_group_member(group_name: str) -> set:`
		`""" Return a list containing the name the members of the given group. """`
		`output = set()`
		`for user in __get_fas_grp_member(group_name):`
		`- if user.role_type in ("user", "sponsor", "administrator"):`
		`- output.add(user.username)`
		`+ output.add(user.username)`
		`return output`


		`def get_bz_email_user(username, email_overrides):`
		`"""Retrieve the bugzilla email associated to the provided username."""`
		`- fas = get_fas()`
		`-`
		`- user_info = fas.person_by_username(username)`
		`- bz_email = user_info.get("email", None)`
		`+ global _FASJSON`
		`+`
		`+ if _FASJSON is None:`
		`+ fas = get_fas()`
		`+ user_info = fas.person_by_username(username)`
		`+ bz_email = user_info.get("email", None)`
		`+ else:`
		`+ fasjson = get_fasjson()`
		`+ try:`
		`+ user_info = fasjson.get_user(username=username).get("result", {})`
		`+ emails = user_info.get("emails", [])`
		`+ if not emails:`
		`+ return None`
		`+ else:`
		`+ bz_email = emails[0]`
		`+ except ClientError:`
		`+ return None`
		`if bz_email is None:`
		`return`

		`@@ -73,12 +121,23 @@`

		`def get_bz_email_group(groupname, email_overrides):`
		`"""Retrieve the bugzilla email associated to the provided group name."""`
		`- fas = get_fas()`

		`- group = fas.group_by_name(groupname)`
		`+ global _FASJSON`
		`+`
		`+ if _FASJSON is None:`
		`+ fas = get_fas()`
		`+ group = fas.group_by_name(groupname)`
		`+`
		`+ else:`
		`+ fasjson = get_fasjson()`
		`+ try:`
		`+ group = fasjson.get_group(groupname=groupname).get("result", {})`
		`+ except ClientError:`
		`+ return None`
		`+`
		`bz_email = group.get("mailing_list")`
		`if bz_email is None:`
		`- return`
		`+ return None`

		`bz_email = bz_email.lower().strip()`
		`bz_email = email_overrides.get(bz_email, bz_email)`
		`@@ -87,8 +146,21 @@`

		`def get_user_by_email(email: str) -> dict:`
		`"""Returns the user found in FAS for that email address."""`
		`- fas = get_fas()`
		`-`
		`- user = fas.people_query(constraints={"email": email})`
		`+ global _FASJSON`
		`+`
		`+ if _FASJSON is None:`
		`+ fas = get_fas()`
		`+`
		`+ user = fas.people_query(constraints={"email": email})`
		`+ else:`
		`+ fasjson = get_fasjson()`
		`+ try:`
		`+ result = fasjson.search(email=email).get("result", [])`
		`+ if not result:`
		`+ return None`
		`+ else:`
		`+ user = result[0]`
		`+ except ClientError:`
		`+ return None`

		`return user`

scoady commented 3 years ago

Signed-off-by: Stephen Coady scoady@redhat.com

zuul commented 3 years ago

Build succeeded.

tox : SUCCESS in 7m 29s

1 new commit added

add testing for fasjson. lots of black and flake errors fixed

3 years ago

zuul commented 3 years ago

Build failed.

tox : FAILURE in 7m 42s

2 new commits added

added tests
add the ability to interact with fasjson

3 years ago

scoady commented 3 years ago

@nphilipp I cleaned up the formatting, hope this is OK now.

zuul commented 3 years ago

Build failed.

tox : FAILURE in 6m 15s

abompard commented on line 69 of toddlers/utils/fedora_account.py 3 years ago

The default here and below should probably be a list, not a dict.

abompard commented on line 106 of toddlers/utils/fedora_account.py 3 years ago

This should be None, I think.

1 new commit added

fixed some return types. re-ordered broken imports. fixed one file black had a problem with.

3 years ago

scoady commented 3 years ago

thanks @abompard. I've fixed those. I've had to leave some as default returns of dicts because I access them with .get in the same method. it should be ok though because that return doesn't leave the method and is checked before anything does.

zuul commented 3 years ago

Build succeeded.

tox : SUCCESS in 6m 10s

Metadata Update from @nphilipp:
- Request assigned

3 years ago

nphilipp commented 3 years ago

NB: Before we can merge this, the changes in this PR need to be squashed or rearranged into topical commits.

As I understand it, this PR removes the ability to operate against a classic FAS instance, right? Not sure if this is desirable until we move to Noggin/AAA in prod. Also, non-functional FAS-specific code (e.g. set_fas(), get_fas()) remains, which is also still used from toddlers and tests, but doesn't really do anything anymore.

I think (@pingou, @abompard, please correct me if I'm wrong) that we want to retain the ability to talk to plain FAS directly, at least until we've migrated in prod as well. For this case I suggest to move the code specific to talking to either FAS or FASJSON into classes which implement an interface common to both, and we just instantiate the one or the other depending on the environment. I'm willing to do this, if we agree that this is what we want and need.

abompard commented 3 years ago

Yeah we need to be able to keep talking to FAS until we switch a variable in the configuration and talk to fasjson.

scoady commented 3 years ago

@nphilipp no problem, I can squash.

No, it doesn't remove the ability to talk to FAS. I've set it up so that its configurable. If 'fasjson' in the toddlers.toml file is true then it will use fasjson. If it is false then it will continue to use FAS and nothing should change. This is similar to what I have done in all the other apps.

nphilipp commented 3 years ago

Ahh, misunderstood the logic. Let's make the dependency fasjson-client (with a dash), just as in that other PR, and it's good to squash & merge from my POV.

rebased onto 0192006

3 years ago

Pull-Request has been merged by nphilipp

3 years ago

zuul commented 3 years ago

Build succeeded.