Refactor permission system + rename whitelist to allowlist #936

mfocko · 2021-01-28T12:43:04Z

TODO

what about Koji?
resolution: keeping as is
~~GitLab namespace (they are multi-part, we probably want to support allowlisting redhat, but accepting all nested namespaces, e.g. redhat/rpms)~~
- safest would be iteratively stripping subnamespace from right and checking against DB
~~we need to differentiate between users with permissions on GitLab and GitHub~~
Switch on allowlisting for GitLab (turned off right now; all events are processed)
resolution: turned on for MRs, push and issue comments
add migration script for allowlist model
resolution: done, not pushed, may be merged together with enum for gitlab/github

Differentiating GitLab/GitHub allowed users (with related not done TODOs) will be done in follow-up PR

Changes

permissions on pull requests:
- before:
  write access > ( Packit admins OR allowlisted namespace of repository OR user that triggered event )
- after:
  Packit admins > ( allowlisted namespace AND ( author of pull request OR write access ) )
issue comments:
- before: allowlisted user OR namespace
- after: allowlisted namespace AND write permissions of user
checking of administrator is moved to allowlist
checking of write permissions of user on repository is moved to allowlist
renamed whitelist to allowlist

Fixes #920
Fixes #937

softwarefactory-project-zuul · 2021-01-28T13:05:26Z

Build succeeded.

pre-commit : SUCCESS in 3m 17s
packit-service-tests : SUCCESS in 21m 01s
packit-service-tests-requre : SUCCESS in 19m 20s

TomasTomecek · 2021-01-28T14:15:29Z

yes, please let's leave koji as it is now and do this only for the CI PR builds

TomasTomecek

nice, LGTM!

interesting that no tests need an update 🤔

mfocko · 2021-01-28T14:36:38Z

nice, LGTM!

interesting that no tests need an update 🤔

removed code is unreachable, it gets denied even sooner 🙃😪

mfocko · 2021-01-29T12:27:54Z

@TomasTomecek @lachmanfrantisek please check, there is also description of allow/deny changes in last commit

Edit: small correction:

Change behaviour on pull requests
- Priority before:
  write access > Packit admins OR (allowlisted ~~namespace or repository~~ namespace of repository OR user that triggered event)
- ~~Priotiry~~ Priority now:
  Packit admins > allowlisted namespace AND (author of PR or write access)

softwarefactory-project-zuul · 2021-01-29T12:47:48Z

Build succeeded.

pre-commit : SUCCESS in 2m 39s
packit-service-tests : SUCCESS in 12m 41s
packit-service-tests-requre : SUCCESS in 20m 02s

lachmanfrantisek · 2021-01-29T12:49:54Z

GitLab namespace

BTW, do we differentiate between git-forges? What about GitLab namespace, that matches the name of the GitHub one that is whitelisted...

mfocko · 2021-01-29T12:53:40Z

GitLab namespace

BTW, do we differentiate between git-forges? What about GitLab namespace, that matches the name of the GitHub one that is whitelisted...

there's no difference between user and namespace either; which probably makes sense? (even though if we enforce allowing namespace and write/pr author, there's no reason to manipulate with users)

shouldn't be a big problem to fix, since it's only github right now (gitlab and pagure is not checked against database)

lachmanfrantisek

Thanks for making it more straightforward!

lachmanfrantisek · 2021-01-29T12:50:54Z

packit_service/constants.py

-DOCS_URL = "https://packit.dev/packit-as-a-service/"
-FAQ_URL = f"{DOCS_URL}#faq"
+DOCS_URL = "https://packit.dev/docs"
+FAQ_URL = f"{DOCS_URL}/faq"
 FAQ_URL_HOW_TO_RETRIGGER = (
-    f"{DOCS_URL}#how-to-re-trigger-packit-service-actions-in-your-pull-request"
+    f"{DOCS_URL}/packit-as-a-service/"
+    "#how-to-re-trigger-packit-service-actions-in-your-pull-request"


Thanks for fixing that!

packit_service/worker/jobs.py

lachmanfrantisek · 2021-01-29T12:55:00Z

packit_service/worker/whitelist.py

-        # whitelist checks dont apply to CentOS (Pagure, Gitlab)
-        if isinstance(
-            event,
+        for callback, related_events in (


What about some mapping, this is becoming too magical to me.

it is a mapping, without map :D i'll add it

My point was more about readability. Just that it's really clear what is going on here.

mfocko · 2021-01-29T13:15:27Z

also I could probably switch the naming of whitelist since I'm probably going to touch the database because of github vs gitlab

softwarefactory-project-zuul · 2021-01-29T14:27:31Z

Build succeeded.

pre-commit : SUCCESS in 2m 40s
packit-service-tests : SUCCESS in 11m 36s
packit-service-tests-requre : SUCCESS in 17m 30s

softwarefactory-project-zuul · 2021-01-29T15:40:02Z

Build failed.

pre-commit : SUCCESS in 2m 06s
packit-service-tests : SUCCESS in 12m 10s
packit-service-tests-requre : FAILURE in 22m 09s

lachmanfrantisek

Sorry for another group of comments...

packit_service/worker/allowlist.py

lachmanfrantisek · 2021-01-29T15:41:32Z

packit_service/worker/allowlist.py

+            )
+        ]
+
+    def unchecked_event(


What about some prefix for those methods? Something like validate_unchecked_event, validate_release_push_event,...

lachmanfrantisek · 2021-01-29T15:44:10Z

packit_service/worker/allowlist.py

+        for related_events, callback in CALLBACKS.items():
+            if isinstance(event, related_events):
+                return callback(event, project, service_config, job_configs)


Personally, I like that approach, but tbh, I am still not sure about the readability of this.

But let's use this and improve later if someone complains...;)

packit_service/worker/jobs.py

mfocko · 2021-01-29T15:48:53Z

Sorry for another group of comments...

definitely won't be last one till finish :D

softwarefactory-project-zuul · 2021-02-01T20:41:35Z

Build failed.

pre-commit : SUCCESS in 2m 24s
packit-service-tests : SUCCESS in 9m 58s
packit-service-tests-requre : FAILURE in 25m 25s

softwarefactory-project-zuul · 2021-02-01T23:21:46Z

Build succeeded.

pre-commit : SUCCESS in 2m 17s
packit-service-tests : SUCCESS in 9m 51s
packit-service-tests-requre : SUCCESS in 21m 22s

packit_service/service/api/allowlist.py

lachmanfrantisek

Thanks! That was a huge amount of work. 🚀

Disables checking of user's write permissions in the repository that PR is created against. Fixes packit#920 Signed-off-by: Matej Focko <mfocko@redhat.com>

Signed-off-by: Matej Focko <mfocko@redhat.com>

- Separate handling of events into methods - Move check for write access to repository from job processing to allowlisting - Fix tests - Change behaviour on pull requests - Priority before: write access > Packit admins > (allowlisted namespace OR repository) - Priority now: Packit admins > allowlisted namespace > (author of pr OR write access) Signed-off-by: Matej Focko <mfocko@redhat.com>

- Add specific types to event callbacks - Enable checking permissions on GitLab - Move callbacks to dictionary Signed-off-by: Matej Focko <mfocko@redhat.com>

Fixes packit#937 Signed-off-by: Matej Focko <mfocko@redhat.com>

softwarefactory-project-zuul · 2021-02-03T10:06:32Z

Build succeeded.

pre-commit : SUCCESS in 2m 24s
packit-service-tests : SUCCESS in 11m 33s
packit-service-tests-requre : SUCCESS in 19m 15s

softwarefactory-project-zuul · 2021-02-03T10:14:22Z

Build succeeded (gate pipeline).

pre-commit : SUCCESS in 3m 00s

lachmanfrantisek approved these changes Jan 28, 2021

View reviewed changes

TomasTomecek approved these changes Jan 28, 2021

View reviewed changes

csomh changed the base branch from master to main January 28, 2021 17:27

mfocko force-pushed the disable-repo-permissions branch from 4df438f to df86589 Compare January 29, 2021 12:26

lachmanfrantisek reviewed Jan 29, 2021

View reviewed changes

mfocko changed the title ~~Disable checking of write permissions on PRs~~ Refactor permission system + rename whitelist to allowlist Jan 29, 2021

mfocko force-pushed the disable-repo-permissions branch from 7e6afc1 to 27f10a4 Compare February 1, 2021 20:15

mfocko force-pushed the disable-repo-permissions branch from 27f10a4 to 57d1da0 Compare February 1, 2021 22:59

mfocko requested review from TomasTomecek and lachmanfrantisek February 2, 2021 09:20

lachmanfrantisek reviewed Feb 2, 2021

View reviewed changes

packit_service/service/api/allowlist.py Show resolved Hide resolved

lachmanfrantisek approved these changes Feb 2, 2021

View reviewed changes

mfocko added 4 commits February 3, 2021 10:37

Disable checking of write permissions on PRs

ec80268

Disables checking of user's write permissions in the repository that PR is created against. Fixes packit#920 Signed-off-by: Matej Focko <mfocko@redhat.com>

Fix documentation URL in constants

c5afa5e

Signed-off-by: Matej Focko <mfocko@redhat.com>

Move admin check to allowlist

dc3150c

Signed-off-by: Matej Focko <mfocko@redhat.com>

mfocko added 2 commits February 3, 2021 10:44

Refactor allowlist and enable checking on GitLab

6bc8eb0

- Add specific types to event callbacks - Enable checking permissions on GitLab - Move callbacks to dictionary Signed-off-by: Matej Focko <mfocko@redhat.com>

Rename whitelist to allowlist

c6c92a7

Fixes packit#937 Signed-off-by: Matej Focko <mfocko@redhat.com>

mfocko force-pushed the disable-repo-permissions branch from 57d1da0 to c6c92a7 Compare February 3, 2021 09:46

mfocko added the mergeit When set, zuul wil gate and merge the PR. label Feb 3, 2021

softwarefactory-project-zuul bot merged commit e59d45d into packit:main Feb 3, 2021

mfocko deleted the disable-repo-permissions branch February 12, 2021 11:44

mfocko mentioned this pull request Feb 23, 2021

Run jobs for a PR if the sender of the event can trigger Packit #697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor permission system + rename whitelist to allowlist #936

Refactor permission system + rename whitelist to allowlist #936

mfocko commented Jan 28, 2021 •

edited

Loading

softwarefactory-project-zuul bot commented Jan 28, 2021

TomasTomecek commented Jan 28, 2021

TomasTomecek left a comment

mfocko commented Jan 28, 2021

mfocko commented Jan 29, 2021 •

edited

Loading

softwarefactory-project-zuul bot commented Jan 29, 2021

lachmanfrantisek commented Jan 29, 2021

mfocko commented Jan 29, 2021 •

edited

Loading

lachmanfrantisek left a comment

lachmanfrantisek Jan 29, 2021

lachmanfrantisek Jan 29, 2021

mfocko Jan 29, 2021

lachmanfrantisek Jan 29, 2021

mfocko commented Jan 29, 2021

softwarefactory-project-zuul bot commented Jan 29, 2021

softwarefactory-project-zuul bot commented Jan 29, 2021

lachmanfrantisek left a comment

lachmanfrantisek Jan 29, 2021

lachmanfrantisek Jan 29, 2021

mfocko commented Jan 29, 2021

softwarefactory-project-zuul bot commented Feb 1, 2021

softwarefactory-project-zuul bot commented Feb 1, 2021

lachmanfrantisek left a comment

softwarefactory-project-zuul bot commented Feb 3, 2021

softwarefactory-project-zuul bot commented Feb 3, 2021

Refactor permission system + rename whitelist to allowlist #936

Refactor permission system + rename whitelist to allowlist #936

Conversation

mfocko commented Jan 28, 2021 • edited Loading

TODO

Changes

softwarefactory-project-zuul bot commented Jan 28, 2021

TomasTomecek commented Jan 28, 2021

TomasTomecek left a comment

Choose a reason for hiding this comment

mfocko commented Jan 28, 2021

mfocko commented Jan 29, 2021 • edited Loading

softwarefactory-project-zuul bot commented Jan 29, 2021

lachmanfrantisek commented Jan 29, 2021

mfocko commented Jan 29, 2021 • edited Loading

lachmanfrantisek left a comment

Choose a reason for hiding this comment

lachmanfrantisek Jan 29, 2021

Choose a reason for hiding this comment

lachmanfrantisek Jan 29, 2021

Choose a reason for hiding this comment

mfocko Jan 29, 2021

Choose a reason for hiding this comment

lachmanfrantisek Jan 29, 2021

Choose a reason for hiding this comment

mfocko commented Jan 29, 2021

softwarefactory-project-zuul bot commented Jan 29, 2021

softwarefactory-project-zuul bot commented Jan 29, 2021

lachmanfrantisek left a comment

Choose a reason for hiding this comment

lachmanfrantisek Jan 29, 2021

Choose a reason for hiding this comment

lachmanfrantisek Jan 29, 2021

Choose a reason for hiding this comment

mfocko commented Jan 29, 2021

softwarefactory-project-zuul bot commented Feb 1, 2021

softwarefactory-project-zuul bot commented Feb 1, 2021

lachmanfrantisek left a comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Feb 3, 2021

softwarefactory-project-zuul bot commented Feb 3, 2021

mfocko commented Jan 28, 2021 •

edited

Loading

mfocko commented Jan 29, 2021 •

edited

Loading

mfocko commented Jan 29, 2021 •

edited

Loading