#206 Remove user from packager-related group when not a packager
Merged 8 months ago by lenkaseg. Opened a year ago by lenkaseg.
fedora-infra/ lenkaseg/toddlers cleaning-packager-groups  into  main

file modified
+174 -38
@@ -1,4 +1,4 @@ 

- # This file is automatically @generated by Poetry 1.8.3 and should not be changed by hand.

+ # This file is automatically @generated by Poetry 1.8.5 and should not be changed by hand.

  

  [[package]]

  name = "arrow"
@@ -532,51 +532,51 @@ 

  

  [[package]]

  name = "cryptography"

- version = "44.0.0"

+ version = "43.0.3"

  description = "cryptography is a package which provides cryptographic recipes and primitives to Python developers."

  optional = false

- python-versions = "!=3.9.0,!=3.9.1,>=3.7"

- files = [

-     {file = "cryptography-44.0.0-cp37-abi3-macosx_10_9_universal2.whl", hash = "sha256:84111ad4ff3f6253820e6d3e58be2cc2a00adb29335d4cacb5ab4d4d34f2a123"},

-     {file = "cryptography-44.0.0-cp37-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:b15492a11f9e1b62ba9d73c210e2416724633167de94607ec6069ef724fad092"},

-     {file = "cryptography-44.0.0-cp37-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:831c3c4d0774e488fdc83a1923b49b9957d33287de923d58ebd3cec47a0ae43f"},

-     {file = "cryptography-44.0.0-cp37-abi3-manylinux_2_28_aarch64.whl", hash = "sha256:761817a3377ef15ac23cd7834715081791d4ec77f9297ee694ca1ee9c2c7e5eb"},

-     {file = "cryptography-44.0.0-cp37-abi3-manylinux_2_28_x86_64.whl", hash = "sha256:3c672a53c0fb4725a29c303be906d3c1fa99c32f58abe008a82705f9ee96f40b"},

-     {file = "cryptography-44.0.0-cp37-abi3-manylinux_2_34_aarch64.whl", hash = "sha256:4ac4c9f37eba52cb6fbeaf5b59c152ea976726b865bd4cf87883a7e7006cc543"},

-     {file = "cryptography-44.0.0-cp37-abi3-musllinux_1_2_aarch64.whl", hash = "sha256:ed3534eb1090483c96178fcb0f8893719d96d5274dfde98aa6add34614e97c8e"},

-     {file = "cryptography-44.0.0-cp37-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:f3f6fdfa89ee2d9d496e2c087cebef9d4fcbb0ad63c40e821b39f74bf48d9c5e"},

-     {file = "cryptography-44.0.0-cp37-abi3-win32.whl", hash = "sha256:eb33480f1bad5b78233b0ad3e1b0be21e8ef1da745d8d2aecbb20671658b9053"},

-     {file = "cryptography-44.0.0-cp37-abi3-win_amd64.whl", hash = "sha256:abc998e0c0eee3c8a1904221d3f67dcfa76422b23620173e28c11d3e626c21bd"},

-     {file = "cryptography-44.0.0-cp39-abi3-macosx_10_9_universal2.whl", hash = "sha256:660cb7312a08bc38be15b696462fa7cc7cd85c3ed9c576e81f4dc4d8b2b31591"},

-     {file = "cryptography-44.0.0-cp39-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:1923cb251c04be85eec9fda837661c67c1049063305d6be5721643c22dd4e2b7"},

-     {file = "cryptography-44.0.0-cp39-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:404fdc66ee5f83a1388be54300ae978b2efd538018de18556dde92575e05defc"},

-     {file = "cryptography-44.0.0-cp39-abi3-manylinux_2_28_aarch64.whl", hash = "sha256:c5eb858beed7835e5ad1faba59e865109f3e52b3783b9ac21e7e47dc5554e289"},

-     {file = "cryptography-44.0.0-cp39-abi3-manylinux_2_28_x86_64.whl", hash = "sha256:f53c2c87e0fb4b0c00fa9571082a057e37690a8f12233306161c8f4b819960b7"},

-     {file = "cryptography-44.0.0-cp39-abi3-manylinux_2_34_aarch64.whl", hash = "sha256:9e6fc8a08e116fb7c7dd1f040074c9d7b51d74a8ea40d4df2fc7aa08b76b9e6c"},

-     {file = "cryptography-44.0.0-cp39-abi3-musllinux_1_2_aarch64.whl", hash = "sha256:d2436114e46b36d00f8b72ff57e598978b37399d2786fd39793c36c6d5cb1c64"},

-     {file = "cryptography-44.0.0-cp39-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:a01956ddfa0a6790d594f5b34fc1bfa6098aca434696a03cfdbe469b8ed79285"},

-     {file = "cryptography-44.0.0-cp39-abi3-win32.whl", hash = "sha256:eca27345e1214d1b9f9490d200f9db5a874479be914199194e746c893788d417"},

-     {file = "cryptography-44.0.0-cp39-abi3-win_amd64.whl", hash = "sha256:708ee5f1bafe76d041b53a4f95eb28cdeb8d18da17e597d46d7833ee59b97ede"},

-     {file = "cryptography-44.0.0-pp310-pypy310_pp73-macosx_10_9_x86_64.whl", hash = "sha256:37d76e6863da3774cd9db5b409a9ecfd2c71c981c38788d3fcfaf177f447b731"},

-     {file = "cryptography-44.0.0-pp310-pypy310_pp73-manylinux_2_28_aarch64.whl", hash = "sha256:f677e1268c4e23420c3acade68fac427fffcb8d19d7df95ed7ad17cdef8404f4"},

-     {file = "cryptography-44.0.0-pp310-pypy310_pp73-manylinux_2_28_x86_64.whl", hash = "sha256:f5e7cb1e5e56ca0933b4873c0220a78b773b24d40d186b6738080b73d3d0a756"},

-     {file = "cryptography-44.0.0-pp310-pypy310_pp73-manylinux_2_34_aarch64.whl", hash = "sha256:8b3e6eae66cf54701ee7d9c83c30ac0a1e3fa17be486033000f2a73a12ab507c"},

-     {file = "cryptography-44.0.0-pp310-pypy310_pp73-manylinux_2_34_x86_64.whl", hash = "sha256:be4ce505894d15d5c5037167ffb7f0ae90b7be6f2a98f9a5c3442395501c32fa"},

-     {file = "cryptography-44.0.0-pp310-pypy310_pp73-win_amd64.whl", hash = "sha256:62901fb618f74d7d81bf408c8719e9ec14d863086efe4185afd07c352aee1d2c"},

-     {file = "cryptography-44.0.0.tar.gz", hash = "sha256:cd4e834f340b4293430701e772ec543b0fbe6c2dea510a5286fe0acabe153a02"},

+ python-versions = ">=3.7"

+ files = [

+     {file = "cryptography-43.0.3-cp37-abi3-macosx_10_9_universal2.whl", hash = "sha256:bf7a1932ac4176486eab36a19ed4c0492da5d97123f1406cf15e41b05e787d2e"},

+     {file = "cryptography-43.0.3-cp37-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:63efa177ff54aec6e1c0aefaa1a241232dcd37413835a9b674b6e3f0ae2bfd3e"},

+     {file = "cryptography-43.0.3-cp37-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:7e1ce50266f4f70bf41a2c6dc4358afadae90e2a1e5342d3c08883df1675374f"},

+     {file = "cryptography-43.0.3-cp37-abi3-manylinux_2_28_aarch64.whl", hash = "sha256:443c4a81bb10daed9a8f334365fe52542771f25aedaf889fd323a853ce7377d6"},

+     {file = "cryptography-43.0.3-cp37-abi3-manylinux_2_28_x86_64.whl", hash = "sha256:74f57f24754fe349223792466a709f8e0c093205ff0dca557af51072ff47ab18"},

+     {file = "cryptography-43.0.3-cp37-abi3-musllinux_1_2_aarch64.whl", hash = "sha256:9762ea51a8fc2a88b70cf2995e5675b38d93bf36bd67d91721c309df184f49bd"},

+     {file = "cryptography-43.0.3-cp37-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:81ef806b1fef6b06dcebad789f988d3b37ccaee225695cf3e07648eee0fc6b73"},

+     {file = "cryptography-43.0.3-cp37-abi3-win32.whl", hash = "sha256:cbeb489927bd7af4aa98d4b261af9a5bc025bd87f0e3547e11584be9e9427be2"},

+     {file = "cryptography-43.0.3-cp37-abi3-win_amd64.whl", hash = "sha256:f46304d6f0c6ab8e52770addfa2fc41e6629495548862279641972b6215451cd"},

+     {file = "cryptography-43.0.3-cp39-abi3-macosx_10_9_universal2.whl", hash = "sha256:8ac43ae87929a5982f5948ceda07001ee5e83227fd69cf55b109144938d96984"},

+     {file = "cryptography-43.0.3-cp39-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:846da004a5804145a5f441b8530b4bf35afbf7da70f82409f151695b127213d5"},

+     {file = "cryptography-43.0.3-cp39-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:0f996e7268af62598f2fc1204afa98a3b5712313a55c4c9d434aef49cadc91d4"},

+     {file = "cryptography-43.0.3-cp39-abi3-manylinux_2_28_aarch64.whl", hash = "sha256:f7b178f11ed3664fd0e995a47ed2b5ff0a12d893e41dd0494f406d1cf555cab7"},

+     {file = "cryptography-43.0.3-cp39-abi3-manylinux_2_28_x86_64.whl", hash = "sha256:c2e6fc39c4ab499049df3bdf567f768a723a5e8464816e8f009f121a5a9f4405"},

+     {file = "cryptography-43.0.3-cp39-abi3-musllinux_1_2_aarch64.whl", hash = "sha256:e1be4655c7ef6e1bbe6b5d0403526601323420bcf414598955968c9ef3eb7d16"},

+     {file = "cryptography-43.0.3-cp39-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:df6b6c6d742395dd77a23ea3728ab62f98379eff8fb61be2744d4679ab678f73"},

+     {file = "cryptography-43.0.3-cp39-abi3-win32.whl", hash = "sha256:d56e96520b1020449bbace2b78b603442e7e378a9b3bd68de65c782db1507995"},

+     {file = "cryptography-43.0.3-cp39-abi3-win_amd64.whl", hash = "sha256:0c580952eef9bf68c4747774cde7ec1d85a6e61de97281f2dba83c7d2c806362"},

+     {file = "cryptography-43.0.3-pp310-pypy310_pp73-macosx_10_9_x86_64.whl", hash = "sha256:d03b5621a135bffecad2c73e9f4deb1a0f977b9a8ffe6f8e002bf6c9d07b918c"},

+     {file = "cryptography-43.0.3-pp310-pypy310_pp73-manylinux_2_28_aarch64.whl", hash = "sha256:a2a431ee15799d6db9fe80c82b055bae5a752bef645bba795e8e52687c69efe3"},

+     {file = "cryptography-43.0.3-pp310-pypy310_pp73-manylinux_2_28_x86_64.whl", hash = "sha256:281c945d0e28c92ca5e5930664c1cefd85efe80e5c0d2bc58dd63383fda29f83"},

+     {file = "cryptography-43.0.3-pp310-pypy310_pp73-win_amd64.whl", hash = "sha256:f18c716be16bc1fea8e95def49edf46b82fccaa88587a45f8dc0ff6ab5d8e0a7"},

+     {file = "cryptography-43.0.3-pp39-pypy39_pp73-macosx_10_9_x86_64.whl", hash = "sha256:4a02ded6cd4f0a5562a8887df8b3bd14e822a90f97ac5e544c162899bc467664"},

+     {file = "cryptography-43.0.3-pp39-pypy39_pp73-manylinux_2_28_aarch64.whl", hash = "sha256:53a583b6637ab4c4e3591a15bc9db855b8d9dee9a669b550f311480acab6eb08"},

+     {file = "cryptography-43.0.3-pp39-pypy39_pp73-manylinux_2_28_x86_64.whl", hash = "sha256:1ec0bcf7e17c0c5669d881b1cd38c4972fade441b27bda1051665faaa89bdcaa"},

+     {file = "cryptography-43.0.3-pp39-pypy39_pp73-win_amd64.whl", hash = "sha256:2ce6fae5bdad59577b44e4dfed356944fbf1d925269114c28be377692643b4ff"},

+     {file = "cryptography-43.0.3.tar.gz", hash = "sha256:315b9001266a492a6ff443b61238f956b214dbec9910a081ba5b6646a055a805"},

  ]

  

  [package.dependencies]

  cffi = {version = ">=1.12", markers = "platform_python_implementation != \"PyPy\""}

  

  [package.extras]

- docs = ["sphinx (>=5.3.0)", "sphinx-rtd-theme (>=3.0.0)"]

- docstest = ["pyenchant (>=3)", "readme-renderer (>=30.0)", "sphinxcontrib-spelling (>=7.3.1)"]

- nox = ["nox (>=2024.4.15)", "nox[uv] (>=2024.3.2)"]

- pep8test = ["check-sdist", "click (>=8.0.1)", "mypy (>=1.4)", "ruff (>=0.3.6)"]

- sdist = ["build (>=1.0.0)"]

+ docs = ["sphinx (>=5.3.0)", "sphinx-rtd-theme (>=1.1.1)"]

+ docstest = ["pyenchant (>=1.6.11)", "readme-renderer", "sphinxcontrib-spelling (>=4.0.1)"]

+ nox = ["nox"]

+ pep8test = ["check-sdist", "click", "mypy", "ruff"]

+ sdist = ["build"]

  ssh = ["bcrypt (>=3.1.5)"]

- test = ["certifi (>=2024)", "cryptography-vectors (==44.0.0)", "pretend (>=0.7)", "pytest (>=7.4.0)", "pytest-benchmark (>=4.0)", "pytest-cov (>=2.10.1)", "pytest-xdist (>=3.5.0)"]

+ test = ["certifi", "cryptography-vectors (==43.0.3)", "pretend", "pytest (>=6.2.0)", "pytest-benchmark", "pytest-cov", "pytest-xdist"]

  test-randomorder = ["pytest-randomly"]

  

  [[package]]
@@ -622,6 +622,26 @@ 

  toml = ["tomli (>=1.2.1)"]

  

  [[package]]

+ name = "dnspython"

+ version = "2.7.0"

+ description = "DNS toolkit"

+ optional = false

+ python-versions = ">=3.9"

+ files = [

+     {file = "dnspython-2.7.0-py3-none-any.whl", hash = "sha256:b4c34b7d10b51bcc3a5071e7b8dee77939f1e878477eeecc965e9835f63c6c86"},

+     {file = "dnspython-2.7.0.tar.gz", hash = "sha256:ce9c432eda0dc91cf618a5cedf1a4e142651196bbcd2c80e89ed5a907e5cfaf1"},

+ ]

+ 

+ [package.extras]

+ dev = ["black (>=23.1.0)", "coverage (>=7.0)", "flake8 (>=7)", "hypercorn (>=0.16.0)", "mypy (>=1.8)", "pylint (>=3)", "pytest (>=7.4)", "pytest-cov (>=4.1.0)", "quart-trio (>=0.11.0)", "sphinx (>=7.2.0)", "sphinx-rtd-theme (>=2.0.0)", "twine (>=4.0.0)", "wheel (>=0.42.0)"]

+ dnssec = ["cryptography (>=43)"]

+ doh = ["h2 (>=4.1.0)", "httpcore (>=1.0.0)", "httpx (>=0.26.0)"]

+ doq = ["aioquic (>=1.0.0)"]

+ idna = ["idna (>=3.7)"]

+ trio = ["trio (>=0.23)"]

+ wmi = ["wmi (>=1.5.1)"]

+ 

+ [[package]]

  name = "dogpile-cache"

  version = "1.3.3"

  description = "A caching front-end based on the Dogpile lock."
@@ -886,6 +906,89 @@ 

  ]

  

  [[package]]

+ name = "ipaclient"

+ version = "4.12.2"

+ description = "FreeIPA client library"

+ optional = false

+ python-versions = ">=3.6.0"

+ files = [

+     {file = "ipaclient-4.12.2-py2.py3-none-any.whl", hash = "sha256:f22a02acea8426a3ebd0dbefc1491618f0ce61bc87934eed213cd35175c7b7bf"},

+ ]

+ 

+ [package.dependencies]

+ cryptography = ">=1.6"

+ ipalib = "4.12.2"

+ ipapython = "4.12.2"

+ qrcode = ">=5.0"

+ six = "*"

+ 

+ [package.extras]

+ install = ["ipaplatform"]

+ ldap = ["python-ldap"]

+ otptoken-yubikey = ["python-yubico", "pyusb"]

+ 

+ [[package]]

+ name = "ipalib"

+ version = "4.12.2"

+ description = "FreeIPA common python library"

+ optional = false

+ python-versions = ">=3.6.0"

+ files = [

+     {file = "ipalib-4.12.2-py2.py3-none-any.whl", hash = "sha256:203170ff3e17466aa192aeb7da3001326a9f101343b200daba0d0dbf4f72608c"},

+ ]

+ 

+ [package.dependencies]

+ ipaplatform = "4.12.2"

+ ipapython = "4.12.2"

+ netaddr = "*"

+ pyasn1 = "*"

+ pyasn1-modules = "*"

+ six = "*"

+ urllib3 = "*"

+ 

+ [package.extras]

+ install = ["dbus-python"]

+ 

+ [[package]]

+ name = "ipaplatform"

+ version = "4.12.2"

+ description = "FreeIPA platform"

+ optional = false

+ python-versions = ">=3.6.0"

+ files = [

+     {file = "ipaplatform-4.12.2-py2.py3-none-any.whl", hash = "sha256:3f3ab6aa30869db16c003f329a9ecb7aa10d3b63a6a44e9cc1fb71fe5b2b395a"},

+ ]

+ 

+ [package.dependencies]

+ cffi = "*"

+ ipapython = "4.12.2"

+ pyasn1 = "*"

+ six = "*"

+ 

+ [[package]]

+ name = "ipapython"

+ version = "4.12.2"

+ description = "FreeIPA python support library"

+ optional = false

+ python-versions = ">=3.6.0"

+ files = [

+     {file = "ipapython-4.12.2-py2.py3-none-any.whl", hash = "sha256:5b95f03d1c83ac0c2ec8d1cc0ca8297e2fbb69aa1d9cddec235c71f954af9e45"},

+ ]

+ 

+ [package.dependencies]

+ cffi = "*"

+ cryptography = ">=1.6"

+ dnspython = ">=1.15"

+ gssapi = ">=1.2.0"

+ ipaplatform = "4.12.2"

+ netaddr = "*"

+ six = "*"

+ 

+ [package.extras]

+ ifaddr = ["ifaddr"]

+ ldap = ["python-ldap"]

+ 

+ [[package]]

  name = "isoduration"

  version = "20.11.0"

  description = "Operations with ISO 8601 durations"
@@ -1293,6 +1396,20 @@ 

  ]

  

  [[package]]

+ name = "netaddr"

+ version = "1.3.0"

+ description = "A network address manipulation library for Python"

+ optional = false

+ python-versions = ">=3.7"

+ files = [

+     {file = "netaddr-1.3.0-py3-none-any.whl", hash = "sha256:c2c6a8ebe5554ce33b7d5b3a306b71bbb373e000bbbf2350dd5213cc56e3dbbe"},

+     {file = "netaddr-1.3.0.tar.gz", hash = "sha256:5c3c3d9895b551b763779ba7db7a03487dc1f8e3b385af819af341ae9ef6e48a"},

+ ]

+ 

+ [package.extras]

+ nicer-shell = ["ipython"]

+ 

+ [[package]]

  name = "noggin-messages"

  version = "1.1.0"

  description = "Fedora Messaging message schemas for Noggin."
@@ -1766,6 +1883,25 @@ 

  ]

  

  [[package]]

+ name = "qrcode"

+ version = "8.0"

+ description = "QR Code image generator"

+ optional = false

+ python-versions = "<4.0,>=3.9"

+ files = [

+     {file = "qrcode-8.0-py3-none-any.whl", hash = "sha256:9fc05f03305ad27a709eb742cf3097fa19e6f6f93bb9e2f039c0979190f6f1b1"},

+     {file = "qrcode-8.0.tar.gz", hash = "sha256:025ce2b150f7fe4296d116ee9bad455a6643ab4f6e7dce541613a4758cbce347"},

+ ]

+ 

+ [package.dependencies]

+ colorama = {version = "*", markers = "sys_platform == \"win32\""}

+ 

+ [package.extras]

+ all = ["pillow (>=9.1.0)", "pypng"]

+ pil = ["pillow (>=9.1.0)"]

+ png = ["pypng"]

+ 

+ [[package]]

  name = "referencing"

  version = "0.35.1"

  description = "JSON Referencing + Python"
@@ -2666,4 +2802,4 @@ 

  [metadata]

  lock-version = "2.0"

  python-versions = "^3.11"

- content-hash = "10173db70148a72111141a78ae0d582727ce949bb6ac6d4e3e40db40039e717e"

+ content-hash = "8048917cc41a98dbe75498694aff79dc8163adc6c309cad211ffc8840b6990e9"

file modified
+3
@@ -27,6 +27,9 @@ 

  zstandard = "^0.23.0"

  dogpile-cache = "^1.3.3"

  pylibmc = "^1.6.3"

+ ipalib = "^4.12.2"

+ cryptography = "<44.0.0"

+ ipaclient = "^4.12.2"

  

  [tool.poetry.group.dev.dependencies]

  pytest = "*"

@@ -0,0 +1,251 @@ 

+ """

+ Unit tests for `toddler.plugins.cleaning_packager_groups`

+ """

+ 

+ import logging

+ from unittest.mock import MagicMock, patch

+ 

+ import pytest

+ 

+ from toddlers.exceptions import PagureError

+ import toddlers.plugins.cleaning_packager_groups as cleaning_packager_groups

+ 

+ 

+ class TestAcceptTopic:

+     """

+     Test class for `toddler.plugins.cleaning_packager_groups.CleanPackagerGroups.accepts_topic`

+     method.

+     """

+ 

+     toddler_cls = cleaning_packager_groups.CleanPackagerGroups

+ 

+     def test_accepts_topic_invalid(self, toddler):

+         """

+         Assert that invalid topic is not accepted.

+         """

+         assert toddler.accepts_topic("foo.bar") is False

+ 

+     @pytest.mark.parametrize(

+         "topic",

+         [

+             "org.fedoraproject.*.fas.group.member.removed",

+             "org.fedoraproject.prod.fas.group.member.removed",

+             "org.fedoraproject.stg.fas.group.member.removed",

+         ],

+     )

+     def test_accepts_topic_valid(self, topic, toddler):

+         """

+         Assert that valid topics are accepted.

+         """

+         assert toddler.accepts_topic(topic)

+ 

+ 

+ class TestProcess:

+     """

+     Test class for `toddler.plugins.cleaning_packager_groups.CleanPackagerGroups.process` method

+     """

+ 

+     def setup_method(self):

+         """Initialize toddler."""

+         self.toddler_cls = cleaning_packager_groups.CleanPackagerGroups()

+         self.toddler_cls._ipa_session = MagicMock()

+ 

+     def test_process(self):

+         """

+         Assert that process is called correctly.

+         """

+         config = {"pagure": {"pagure.prod.url"}, "pagure_stg": {"pagure.stg.url"}}

+         message = MagicMock()

+         message.body = {

+             "msg": {"agent": "lenkaseg", "group": "packager", "user": "patrikp"},

+             "headers": {

+                 "fedora_messaging_group_communishift-admins": True,

+                 "fedora_messaging_schema": "noggin.group.member.sponsor.v1",

+                 "fedora_messaging_severity": 20,

+                 "fedora_messaging_user_lenkaseg": True,

+                 "fedora_messaging_user_patrikp": True,

+                 "priority": 0,

+                 "sent-at": "2024-07-04T15:48:25+00:00",

+             },

+             "id": "35d54838-b12c-49d5-949c-c503a71e7ae5",

+             "priority": 0,

+             "queue": "null",

+             "topic": "org.fedoraproject.stg.fas.group.member.remove",

+         }

+ 

+         self.toddler_cls.process = MagicMock()

+         self.toddler_cls.process(config, message)

+         self.toddler_cls.process.assert_called_once_with(config, message)

+ 

+     @patch("toddlers.utils.pagure.set_pagure")

+     def test_not_packager_group(self, mock_set_pagure, caplog):

+         """

+         Assert that if the group in the message is not "packager", plugin stops.

+         """

+         mock_set_pagure.return_value.remove_member_from_group.return_value = None

+         mock_set_pagure.return_value.get_all_groups.return_value = ["group1", "group2"]

+         caplog.set_level(logging.INFO)

+         config = {

+             "pagure_url": "https://pagure.io",

+             "pagure_stg": "https://stg.pagure.io",

+             "watched_groups": ["packager"],

+         }

+         message = MagicMock()

+         message.body = {

+             "msg": {

+                 "group": "not-packager",

+                 "user": "lenkaseg",

+             },

+             "topic": "org.fedoraproject.stg.fas.group.member.remove",

+         }

+         self.toddler_cls.process(config, message)

+         assert (

+             caplog.records[-1].message

+             == "User lenkaseg was not removed from packager group, but "

+             "from not-packager group, nothing to do."

+         )

+ 

+     @pytest.mark.parametrize(

+         "distgit_groups,ipa_groups,message_1,message_2,message_3,message_4",

+         (

+             (

+                 ["group1", "group2"],

+                 {"result": {"memberof_group": ["group1", "group3"]}},

+                 "User lenkaseg removed from distgit group: group1",

+                 "User lenkaseg removed from ipa group: group1",

+                 "User lenkaseg should be removed from following groups: ['group1']",

+                 "Ipa groups: ['group1', 'group3']",

+             ),

+             (

+                 None,

+                 {"result": {"memberof_group": ["group1", "group3"]}},

+                 "No distgit groups found, bailing.",

+                 "Distgit groups found: None",

+                 "Fetching all distgit groups:",

+                 "User lenkaseg was removed from packager group, removing from "

+                 "packager-related groups as well.",

+             ),

+             (

+                 ["group1", "group2"],

+                 {"result": {"memberof_group": ["group3", "group4"]}},

+                 "Seems that user lenkaseg is not member of any groups shared "

+                 "between ipa and distgit",

+                 "Ipa groups: ['group3', 'group4']",

+                 "Fetching groups user lenkaseg is member of in ipa:",

+                 "Distgit groups found: ['group1', 'group2']",

+             ),

+             (

+                 ["group1", "group2"],

+                 {"result": {"memberof_group": []}},

+                 "User lenkaseg is not member of any groups in ipa, bailing.",

+                 "Ipa groups: []",

+                 "Fetching groups user lenkaseg is member of in ipa:",

+                 "Distgit groups found: ['group1', 'group2']",

+             ),

+         ),

+         ids=(

+             "distgit_groups_found",

+             "distgit_group_not_found",

+             "no intersecting distgit/ipa groups",

+             "ipa_groups_not_found",

+         ),

+     )

+     @patch("toddlers.utils.pagure.set_pagure")

+     def test_is_packager_group_and_post_is_ok(

+         self,

+         mock_pagure,

+         distgit_groups,

+         ipa_groups,

+         message_1,

+         message_2,

+         message_3,

+         message_4,

+         caplog,

+     ):

+         """

+         Assert if group is packager, the plugin removes the user from related distgit groups.

+         """

+         mock_pagure_obj = MagicMock()

+         mock_pagure_obj.get_all_groups.return_value = distgit_groups

+         mock_pagure_obj.remove_member_from_group.return_value = None

+         mock_pagure.return_value = mock_pagure_obj

+ 

+         self.toddler_cls._ipa_session.Command.user_show.return_value = ipa_groups

+         caplog.set_level(logging.INFO)

+         config = {

+             "pagure_url": "https://example.io",

+             "pagure_stg": "https://example.stg.io",

+             "watched_groups": ["packager"],

+         }

+         message = MagicMock()

+         message.body = {

+             "msg": {"group": "packager", "user": "lenkaseg", "agent": "agent"},

+             "topic": "org.fedoraproject.stg.fas.group.member.remove",

+         }

+         self.toddler_cls.process(config, message)

+         assert caplog.records[-1].message == message_1

+         assert caplog.records[-2].message == message_2

+         assert caplog.records[-3].message == message_3

+         assert caplog.records[-4].message == message_4

+ 

+     @pytest.mark.parametrize(

+         "ipa_remove_member,error,message_1,message_2",

+         (

+             (

+                 Exception,

+                 None,

+                 "User lenkaseg removed from distgit group: group1",

+                 "Error while removing user lenkaseg from ipa group group1",

+             ),

+             (

+                 None,

+                 PagureError,

+                 "Error while removing user lenkaseg from distgit group group1",

+                 "User lenkaseg removed from ipa group: group1",

+             ),

+         ),

+         ids=("ipa_error", "distgit_error"),

+     )

+     @patch("toddlers.plugins.cleaning_packager_groups.pagure.set_pagure")

+     def test_is_packager_group_and_post_is_not_ok(

+         self,

+         mock_set_pagure,

+         ipa_remove_member,

+         error,

+         message_1,

+         message_2,

+         caplog,

+     ):

+         """

+         Assert correct behavior when encountered with pagure error.

+         """

+         mock_set_pagure.return_value.get_all_groups.return_value = ["group1", "group2"]

+         mock_set_pagure.return_value.remove_member_from_group.side_effect = error

+ 

+         self.toddler_cls._ipa_session.Command.user_show.return_value = {

+             "result": {"memberof_group": ["group1", "group3"]}

+         }

+ 

+         self.toddler_cls._ipa_session.Command.group_remove_member.side_effect = (

+             ipa_remove_member

+         )

+ 

+         caplog.set_level(logging.INFO)

+         config = {

+             "pagure_url": "https://example.io",

+             "pagure_stg": "https://example.stg.io",

+             "pagure_api_key": "pagure_key",

+             "watched_groups": ["packager"],

+         }

+         message = MagicMock()

+         message.body = {

+             "msg": {"group": "packager", "user": "lenkaseg", "agent": "agent"},

+             "topic": "org.fedoraproject.stg.fas.group.member.remove",

+         }

+ 

+         self.toddler_cls.process(config, message)

+         self.toddler_cls._ipa_session.Command.user_show.assert_called_with(

+             uid="lenkaseg"

+         )

+         assert caplog.records[-1].message == message_1

+         assert caplog.records[-2].message == message_2

@@ -3,6 +3,7 @@ 

  """

  

  import json

+ import logging

  from unittest.mock import call, MagicMock, Mock, patch

  

  import pytest
@@ -1947,6 +1948,62 @@ 

          assert result is False

  

  

+ class TestPagureGetAllGroups:

+     """

+     Test class for `toddlers.utils.pagure.Pagure.get_all_groups` method.

+     """

+ 

+     @pytest.mark.parametrize(

+         "url,message",

+         (

+             (

+                 "https://stg.pagure.io",

+                 "Retrieving groups from https://stg.pagure.io/api/0/groups",

+             ),

+             (

+                 "https://stg.src.fedoraproject.org",

+                 "Retrieving groups from https://stg.src.fedoraproject.org/api/0/groups",

+             ),

+         ),

+         ids=(

+             "pagure_url",

+             "distgit_url",

+         ),

+     )

+     def test_get_all_groups(self, url, message, caplog):

+         """

+         Assert correct retrieval of distgit groups

+         """

+         config = {

+             "pagure_url": url,

+             "pagure_api_key": "Very secret key",

+         }

+         self.pagure = pagure.set_pagure(config)

+         self.pagure._requests_session = Mock()

+         caplog.set_level(logging.INFO)

+         response_mock = MagicMock()

+         response_mock.ok.return_value = True

+         response_mock.json.return_value = {

+             "groups": [

+                 "group_a",

+                 "group_b",

+             ],

+             "pagination": {

+                 "next": None,

+             },

+         }

+ 

+         # mock_request.side_effect = request

+         self.pagure._requests_session.get.return_value = response_mock

+ 

+         result = self.pagure.get_all_groups()

+         assert result == [

+             "group_a",

+             "group_b",

+         ]

+         assert caplog.records[-1].message == message

+ 

+ 

  class TestPagureOrphanPackage:

      """

      Test class for `toddlers.utils.pagure.Pagure.orphan_package` method.

file modified
+7 -1
@@ -82,7 +82,7 @@ 

  fasjson = false

  

  # Account to use to connect to FAS

- fas_url = "https://admin.fedoraproject.org/accounts"

+ fas_url = "https://fasjson.fedoraproject.org/"

  fas_username = "FAS username"

  fas_password = "FAS password for that account"

  
@@ -107,6 +107,10 @@ 

  # Temp folder to use for toddlers temp files

  temp_folder = "/var/tmp"

  

+ # Env vars for bootstraping ipalib API

+ KRB5_CONF = "path_to_krb5.conf"

+ IPA_CONFDIR = "path_to_dir/synced_folders/ipa/ipa"

+ 

  # Account to use to connect to Pagure-as-dist-git

  dist_git_url = "https://src.fedoraproject.org"

  dist_git_token_seed = "private random string to change"
@@ -315,6 +319,8 @@ 

  #Mapping of FAS groups to Pagure groups

  infra-sig = 'fedora-infra'

  

+ [consumer_config.cleaning_packager_groups]

+ watched_groups = ["packager"]

  

  [qos]

  prefetch_size = 0

@@ -0,0 +1,130 @@ 

+ """

+ The script is triggered by fedora-messaging messages published under the topic

+ ``toddlers.trigger.fas.group.member.remove`` and checks if a user was removed

+ from group "packagers". If yes, user is removed from packager related groups

+ as well.

+ """

+ 

+ import logging

+ import os

+ 

+ import ipalib

+ 

+ from toddlers.base import ToddlerBase

+ from toddlers.exceptions import PagureError

+ from toddlers.utils import pagure

+ 

+ _log = logging.getLogger(__name__)

+ 

+ 

+ class CleanPackagerGroups(ToddlerBase):

+     """Listens to messages to check for a membership removal message from FAS"""

+ 

+     name: str = "clean_packagers_groups"

+     amqp_topics: list[str] = [

+         "org.fedoraproject.*.fas.group.member.removed",

+     ]

+     pagure: pagure.Pagure

+ 

+     def __init__(self):

+         super().__init__()

+         self.pagure_url = None

+         self.dist_git = None

+         self._ipa_session = None

+ 

+     def accepts_topic(self, topic):

+         """Returns a boolean whether this toddler is interested in messages

+         from this specific topic.

+         """

+         return topic.startswith("org.fedoraproject.") and topic.endswith(

+             "fas.group.member.removed"

+         )

+ 

+     def _get_ipa_session(self, config):  # pragma: no cover

+         """Makes an ipa session that handles logging in"""

+         if self._ipa_session is None:

+             conf_vars = ("KRB5_CONFIG", "IPA_CONFDIR")

+             for conf_var in conf_vars:

+                 if not config.get(conf_var):

+                     continue

+                 os.environ[conf_var] = config[conf_var]

+             self._ipa_session = ipalib.api

+             self._ipa_session.bootstrap(context="custom")

+             self._ipa_session.load_plugins()

+             self._ipa_session.finalize()

+             self._ipa_session.Backend.rpcclient.connect()

+         return self._ipa_session

+ 

+     def process(self, config, message):

+         """Process a given message. Remove user that is being removed from

+         a packager group from all groups in distgit"""

+         msg = message.body

+         user = msg["msg"]["user"]

+         group = msg["msg"]["group"]

+         watched_groups = config.get("watched_groups")

+         # If user is not removed from packager group, bail

+         if group not in watched_groups:

+             _log.info(

+                 f"User {user} was not removed from packager group, "

+                 f"but from {group} group, nothing to do."

+             )

+             return

+ 

+         _log.info(

+             f"User {user} was removed from {group} group, removing from "

+             "packager-related groups as well."

+         )

+ 

+         self.dist_git = pagure.set_pagure(

+             {

+                 "pagure_url": config.get("dist_git_url"),

+                 "pagure_api_key": config.get("dist_git_token"),

+             }

+         )

+         _log.info("Fetching all distgit groups:")

+         distgit_groups = self.dist_git.get_all_groups()

+         _log.info(f"Distgit groups found: {distgit_groups}")

+ 

+         if not distgit_groups:

+             _log.info("No distgit groups found, bailing.")

+             return

+ 

+         ipa_session = self._get_ipa_session(config)

+         _log.info(f"Fetching groups user {user} is member of in ipa:")

+         ipa_user = ipa_session.Command.user_show(uid=user)

+         ipa_groups = ipa_user["result"]["memberof_group"]

+         _log.info(f"Ipa groups: {ipa_groups}")

+ 

+         if not ipa_groups:

+             _log.info(f"User {user} is not member of any groups in ipa, bailing.")

+             return

+ 

+         intersection = list(set(distgit_groups) & set(ipa_groups))

+         if not intersection:

+             _log.info(

+                 f"Seems that user {user} is not member of any "

+                 "groups shared between ipa and distgit"

+             )

+             return

+         _log.info(

+             f"User {user} should be removed from following groups: {intersection}"

+         )

+ 

+         for group in intersection:

+             try:

+                 # Remove user from ipa groups:

+                 ipa_session.Command.group_remove_member(cn=group, user=user)

+                 _log.info(f"User {user} removed from ipa group: {group}")

+             except Exception:

+                 _log.exception(

+                     f"Error while removing user {user} from ipa group {group}"

+                 )

+             try:

+                 # Remove user from distgit groups:

+                 # pagure token with acls=group_modify needed

+                 self.dist_git.remove_member_from_group(user, group)

+                 _log.info(f"User {user} removed from distgit group: {group}")

+             except PagureError:

+                 _log.exception(

+                     f"Error while removing user {user} from distgit group {group}"

+                 )

file modified
+13
@@ -1228,6 +1228,19 @@ 

              )

              return False

  

+     def get_all_groups(self):

+         """Returns list of all groups from Pagure or Distgit"""

+         url = f"{self._pagure_url}/api/0/groups"

+         all_groups = []

+         while url:

+             log.info(f"Retrieving groups from {url}")

+             response = self._requests_session.get(url)

+             data = response.json()

+             groups = data.get("groups", [])

+             url = data.get("pagination", dict()).get("next", "")

+             all_groups.extend(groups)

+         return all_groups

+ 

      def orphan_package(

          self, namespace: str, package: str, reason: str, info: str

      ) -> None:

Fixes issue #190

This toddler listens to topic fas.group.member.removed and when a user is removed from a packager group, it removes the user also from related distgit groups via pagure api_group_remove_member endpoint https://pagure.io/pagure/blob/1b7d305/f/pagure/api/group.py#_387
This requires ACLs group_modify, which has to be fixed still.

For the moment it removes members only from go-sig.

Signed-off-by: Lenka Segura lsegura@redhat.com

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/8ca6e25ecaa64dd895004fad17368fbe

It would be better to move this to toddlers configuration file.

This will not be needed when the PAGURE_URL will be configurable in config file.

This should be probably retrieved from dist-git.

This will only work on INFO level, not anything bellow like DEBUG.

This doesn't do any actual decoration, it will just print the message itself.

This will only work on INFO level, not anything bellow like DEBUG.

It's a helper when running a single toddler locally, to see the log messages in the terminal. It's something I "borrowed" from another toddler by pingou. If it's too much of an eyesore, I can remove it once I have it tested and working.

This will only work on INFO level, not anything bellow like DEBUG.

It's a helper when running a single toddler locally, to see the log messages in the terminal. It's something I "borrowed" from another toddler by pingou. If it's too much of an eyesore, I can remove it once I have it tested and working.

The change of logging.StreamHandler should be enough to start printing messages to console.

rebased onto 7f29d09

a year ago

Still working on removing the user from all distgit groups. Trying to find an easy way to list the groups the user is member of.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/7da6c011eda142d088a4d8bcb931f198

rebased onto 7f29d09

a year ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/9a8dcbb56b30411cb934541756e85b8d

rebased onto 7f29d09

a year ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/2729bdb83d384c0e9424da11f8373521

1 new commit added

  • cleaning_packager_groups: Get groups from distgit and verify membership with fasjson
a year ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/b130888aaa8b4d789b611d12fdca1d1a

rebased onto a932eaa

a year ago

Working in improving the coverage.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/09dc33ee9366436995fc2d2bc357cade

rebased onto 51b209c

10 months ago

rebased onto 51b209c

10 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/52193f4d62ff4e3faba38299fdbaa944

fails on toml somewhere...

3 new commits added

  • cleaning_packager_groups: Improve coverage
  • cleaning_packager_groups: Get groups from distgit and verify membership with fasjson
  • Remove user from packager-related group when not a packager
10 months ago

You can take the URL from general config section, so we don't need to change it in multiple places.

Couldn't you get all the user groups in one call? This will sent request to FAS for every group the user is in distgit. Something similar like get_distgit_groups will be more appreciated for FAS and then just compare them.

You can take the URL from general config section, so we don't need to change it in multiple places.

Sure, in fact all this section is not necessary when I look at it.

Couldn't you get all the user groups in one call? This will sent request to FAS for every group the user is in distgit. Something similar like get_distgit_groups will be more appreciated for FAS and then just compare them.

Looking at fasjson API, I think I can!
There's an endpoint to fetch user's groups: /users/{username}/groups, I will use that. Thanks for pointing this out!

This probably going to come back and bite us one day when we have more than 100 packager groups. Please do some pagination, even if it's not useful right now. Future you will be thankful! :-)

2 new commits added

  • cleaning_packager_groups: Remove CLI feature and test message
  • cleaning_packager_groups: get_all_groups from pagure module and add pagination
10 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/9114228add464280bd29aff9179c3932

6 new commits added

  • cleaning_packager_group: Reduce fasjon calls to two
  • cleaning_packager_groups: Remove CLI feature and test message
  • cleaning_packager_groups: get_all_groups from pagure module and add pagination
  • cleaning_packager_groups: Improve coverage
  • cleaning_packager_groups: Get groups from distgit and verify membership with fasjson
  • Remove user from packager-related group when not a packager
10 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/c516c31fe4c242669fe0efc39e33b6c1

6 new commits added

  • cleaning_packager_group: Reduce fasjon calls to two
  • cleaning_packager_groups: Remove CLI feature and test message
  • cleaning_packager_groups: get_all_groups from pagure module and add pagination
  • cleaning_packager_groups: Improve coverage
  • cleaning_packager_groups: Get groups from distgit and verify membership with fasjson
  • Remove user from packager-related group when not a packager
10 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/2385c7a81dad40d38a8347e90a0c6f93

6 new commits added

  • cleaning_packager_group: Reduce fasjon calls to two
  • cleaning_packager_groups: Remove CLI feature and test message
  • cleaning_packager_groups: get_all_groups from pagure module and add pagination
  • cleaning_packager_groups: Improve coverage
  • cleaning_packager_groups: Get groups from distgit and verify membership with fasjson
  • Remove user from packager-related group when not a packager
10 months ago

rebased onto 8b5ea1a

10 months ago

I think I addressed all the points of the review:
- urls taken from config
- reduced the json calls to get all user's groups at once instead of iterating through all distgit groups and making calls with fasjson whether the user is_member_of each group (from 64 calls to 2)
- created pagination
- removed all the cli related stuff and testing messages since with the new fedora-messaging reconsume command it won't be necessary
- adjusted the tests and coverage

That reminds me I have a guide for the local testing written and I will add it to the toddler documentation in another PR (thanks to @abompard!)

Why the empty section?

Maybe a dumb question, but what will remove the user from FAS groups? From what I understand here you are looking for the groups the user is member of and comparing them with existing dist-git groups. Wouldn't he be added back to the groups next time he login to dist-git?

The private method should have only one _.

But I don't understand why two methods, why not directly call list_user_groups and then handle the result?

Don't do this, if there isn't difference between dist-git and pagure.io, just do log.info("Retrieving groups from %s, self._pagure_url) This could bite you back in the future.

Why the empty section?

True, removing... and while at it, removing also several other empty config sections.

Maybe a dumb question, but what will remove the user from FAS groups? From what I understand here you are looking for the groups the user is member of and comparing them with existing dist-git groups. Wouldn't he be added back to the groups next time he login to dist-git?

Not dumb at all, we were talking about it with @abompard and figured out removing user from IPA would not be necessary, since the IPA groups actions take effect without the need of the user to log in. For distgit groups the user has to log in, that's why it has to be force-synced.

The private method should have only one _.

But I don't understand why two methods, why not directly call list_user_groups and then handle the result?

Underscore removed. This happened because I was trying to make it similar to another function earlier in the fedora-account.py file. I opened a PR to fix the original one as well: https://pagure.io/fedora-infra/toddlers/pull-request/323
The two methods are the approach I copied from the previous function as well, but here I think it makes sense. It makes the function easier to test and to mock the request call.

Don't do this, if there isn't difference between dist-git and pagure.io, just do log.info("Retrieving groups from %s, self._pagure_url) This could bite you back in the future.

Good point, done!

6 new commits added

  • cleaning_packager_group: Reduce fasjon calls to two
  • cleaning_packager_groups: Remove CLI feature and test message
  • cleaning_packager_groups: get_all_groups from pagure module and add pagination
  • cleaning_packager_groups: Improve coverage
  • cleaning_packager_groups: Get groups from distgit and verify membership with fasjson
  • Remove user from packager-related group when not a packager
10 months ago

This one is actually needed, because there is a subsection following it.

This one is actually needed, because there is a subsection following it.

This is not empty, there is just empty line between the config and section header.

You don't need this with while url.

Not dumb at all, we were talking about it with @abompard and figured out removing user from IPA would not be necessary, since the IPA groups actions take effect without the need of the user to log in. For distgit groups the user has to log in, that's why it has to be force-synced.

If the IPA groups are doing this automatically, wouldn't the user be already removed from the groups? That would mean you will never match the groups as the user will not be in them anymore.

If the IPA groups are doing this automatically, wouldn't the user be already removed from the groups? That would mean you will never match the groups as the user will not be in them anymore.

Huh, yeah, I didn't see that. I already don't remember why I decided to make the matching, I guess I got tangled in it. I will just make a call to distgit removing the user from all distgit groups, in fact I don't see a point in checking in which groups the user is in IPA. The toddler needs to know only whether the user was removed from packager and remove the user from all distgit groups.

If the IPA groups are doing this automatically, wouldn't the user be already removed from the groups? That would mean you will never match the groups as the user will not be in them anymore.

Huh, yeah, I didn't see that. I already don't remember why I decided to make the matching, I guess I got tangled in it. I will just make a call to distgit removing the user from all distgit groups, in fact I don't see a point in checking in which groups the user is in IPA. The toddler needs to know only whether the user was removed from packager and remove the user from all distgit groups.

Ok, we talked with @abompard today and finally clarified what needs to be done. It is as you and Kevin say, the toddler receives a message about a user removed from a IPA packager group and:
- fetches distgit groups the user is member of
- goes to IPA and removes the user from those groups (where group name matches)

The changes in IPA get automatically synced back with the distgit, which effectively removes the user from the distgit groups as well.

@zlopez Can you confirm that that's the correct approach?

For making changes in IPA we will use the noggin user defined in the ipa role in ansible and we have to ensure the noggin user has correct permissions for removing users from groups.
At the toddler level, we will use the ipa-client library.

Ok, we talked with @abompard today and finally clarified what needs to be done. It is as you and Kevin say, the toddler receives a message about a user removed from a IPA packager group and:
- fetches distgit groups the user is member of
- goes to IPA and removes the user from those groups (where group name matches)

The changes in IPA get automatically synced back with the distgit, which effectively removes the user from the distgit groups as well.

@zlopez Can you confirm that that's the correct approach?

For making changes in IPA we will use the noggin user defined in the ipa role in ansible and we have to ensure the noggin user has correct permissions for removing users from groups.
At the toddler level, we will use the ipa-client library.

If I understand it correctly the toddler will now do removal from both dist-git and FAS. That seems like the correct solution to this.

the toddler receives a message about a user removed from a IPA packager group and:
- fetches distgit groups the user is member of

The toddler could also retrieve all the distgit groups, independant from the user, and store that somewhere in a cache. All those groups are packaging groups. This would result in less calls to distgit because of the caching.

The changes in IPA get automatically synced back with the distgit, which effectively removes the user from the distgit groups as well.

Yes, the toddler does not need to remove the user from the distgit groups themselves, because the pagure_fas_groups_sync toddler will do that. It just needs to emit the usual message that is emitted when a user is removed from a group.
Please note that the pagure_fas_groups_sync isn't currently subscribed to the fas.group.member.removed topic, we'll need to add it to its list.

For making changes in IPA we will use the noggin user defined in the ipa role in ansible and we have to ensure the noggin user has correct permissions for removing users from groups.

Nope, we'll use a dedicated service with a dedicated role and permissions. The noggin user has more permissions than necessary, we shouldn't use it. This new service can be created in ansible, I can do that as it's a bit complex on the IPA side.

At the toddler level, we will use the ipa-client library.

Yes, it's called ipaclient and you can find it at https://pagure.io/freeipa/blob/master/f/ipaclient

Note that also pagure_fas_groups_sync is only syncing 2 groups on pagure.io, so it would need to be extended to handle src.fedoraproject.org and all the packaging groups (which we would need to keep up to date)

When I was writing pagure_fas_groups_sync my understanding was that the src.fedoraproject.org is synced automatically. This is why it sync only pagure.io, which doesn't use the FAS groups at all.

It is synced automatically, but ONLY on login.

So if user foo is in packaging group bar and they login, src.fedoraproject.org syncs that they are in bar group.

if later they are removed from bar group. Nothing happens. They still show up on src.fedoraproject.org as being in bar group.

If they then logged in again, it should sync and remove them from the group.

However, if we are removing people because they are inactive, or because we don't want them to have the permission anymore, this isn't good because they may well not login again.

At least thats my understanding of how it works.

Ok, so they have to be removed both from src.fedoraproject.org (that's easy) and from IPA as well (that's gonna be trickier).
Thanks for the extra clarification Kevin!

So the toddler should do this:
1. the toddler reacts on a message emitted that user was removed from a IPA packager group
2. toddler gets all distgit groups (those 67 or so)
3. toddler matches those groups to IPA groups by name and sends a POST via ipaclient to remove the user from said IPA groups (do we check if the user is member of said groups and send targeted remove calls or send a bulk call to remove the user from the packager groups regardless if he's member or not?)
4. toddler removes the user also from distgit groups (again - let's first check the user's membership or let's send a remove_from_groups call and remove only from those he is member of?)
The point 4 should be part of this cleaning_packager_groups toddler or made as an extension of pagure_fas_groups_sync? Or perhaps a new dedicated toddler to keep things simple?

  1. toddler matches those groups to IPA groups by name and sends a POST via ipaclient to remove the user from said IPA groups (do we check if the user is member of said groups and send targeted remove calls or send a bulk call to remove the user from the packager groups regardless if he's member or not?)

It would be better to use fasjson for this. Not sure what permissions are needed for fasjson to allow that. I'm not sure if we even have any toddler that is actually removing people from groups in FAS.

  1. toddler removes the user also from distgit groups (again - let's first check the user's membership or let's send a remove_from_groups call and remove only from those he is member of?)

We can leverage pagure_fas_group_sync for removal, just adding support for src.fedoraproject.org should be enough (we don't even need to specify the groups as we want all of them to be synced). cleaning_packager_groups toddler will just need to remove the user from packaging groups in FAS (you still need to identify them by retrieving the list of groups from src.fp.o)

It would be better to use fasjson for this. Not sure what permissions are needed for fasjson to allow that. I'm not sure if we even have any toddler that is actually removing people from groups in FAS.

From what I saw, fasjson does only GET requests, no POSTS.
Yeah, I don't think there is a toddler that would remove groups from FAS. That's gonna be the tricky part. @abompard offered to have a look at that, creating a dedicated service with defined role and permissions, in ansible.

We can leverage pagure_fas_group_sync for removal, just adding support for src.fedoraproject.org should be enough (we don't even need to specify the groups as we want all of them to be synced). cleaning_packager_groups toddler will just need to remove the user from packaging groups in FAS (you still need to identify them by retrieving the list of groups from src.fp.o)

Ok, sounds good!

rebased onto fb8951a

9 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/8aa0e6874ab947b78c937990d1e0db10

rebased onto 37b0e83

8 months ago

Ok, cleaning_packager_groups now uses ipalib for getting noggin group info and removing users from noggin.

The toddler now:
- gets triggered on message about user removed from a noggin group
- if the group is packager it:
- gets all the groups from distgit
- gets user's groups in noggin
- makes intersection => we need to identify the groups the user is member of in both noggin and distgit (I suppose there cannot be a group in distgit that would not exist in noggin at the same time, correct?
- removes the user from the intersection groups in noggin and in distgit
- goal achieved, user is not member of any distgit group and also is not member of any matching noggin group (user remains to be member of any non-distgit noggin group).

Tested in tiny-stage (thanks @abompard!) it seems to be behaving well.

Now working on tests.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/7ed5c92069704cd5a6222e4745eb085e

7 new commits added

  • cleaning_packager_groups: Remove user from IPA groups with ipalib
  • cleaning_packager_group: Reduce fasjon calls to two
  • cleaning_packager_groups: Remove CLI feature and test message
  • cleaning_packager_groups: get_all_groups from pagure module and add pagination
  • cleaning_packager_groups: Improve coverage
  • cleaning_packager_groups: Get groups from distgit and verify membership with fasjson
  • Remove user from packager-related group when not a packager
8 months ago

You should move the comment to section bellow.

You should move the comment to section bellow.

Few things:

  • I'm not sure if some of the missing sections in toml config will not cause KeyError when reading the config file. Even if they are empty.
  • I know that we talked about some regular cleaning made by cron job, but I don't see it anywhere, is that still planned?
  • It would be good to have instructions how to test this toddler locally, as with the IPA integration it's much more complex

Thanks for the review, fixing!
- Adding back the removed config sections, it should not be part of this PR anyways
- Cron job in case the toddler does not catch some mesage would be nice, I can add it as a separate PR
- Writing a guide is a good idea. I wrote one for my personal use about how to test this toddler in tiny-stage. It is a bit complex, thanks again @abompard for showing me how to set it up. I will polish it up and open a PR.

rebased onto 37b0e83

8 months ago

rebased onto 37b0e83

8 months ago

rebased onto 37b0e83

8 months ago

I still see some sections that are being removed as part of this PR.

rebased onto c874598

8 months ago

rebased onto c874598

8 months ago

rebased onto c874598

8 months ago

I still see some sections that are being removed as part of this PR.

Removed now.

Also fixing stuff @abompard mentioned in his review:
- rename noggin groups to ipa groups
- split the try/except section to do one thing at a time - first remove user from ipa group, then from distgit group
- fix the error messages
- do not hardcode "packager" group => moved to config
- fix tests accordingly

Pull-Request has been merged by lenkaseg

8 months ago

I want to merge https://pagure.io/fedora-infra/toddlers/pull-request/330# tomorrow, so I will deploy them both together on staging and production as well.