search

stakater / GitWebhookProxy

A proxy to let webhooks reach running services behind a firewall – [✩Star] if you're using it!
https://stakater.com
Apache License 2.0
188 stars 52 forks source link

allowedusers logic should allow empty list #80

Closed davidkarlsen closed 4 years ago

davidkarlsen commented 4 years ago

It seems that the proxy requires to whitelist all users: https://github.com/stakater/GitWebhookProxy/blob/035d2b417545d2781746c3147fabd9ac93a27b1d/pkg/proxy/proxy.go#L73

https://github.com/stakater/GitWebhookProxy/blob/035d2b417545d2781746c3147fabd9ac93a27b1d/pkg/proxy/proxy.go#L145

Would it make more sense to the allow-check if arraysize > 0, else accept it? It seems a bit cumbersome to have to list each and every user.

davidkarlsen commented 4 years ago

Also - should not this check be || instead of &&?

davidkarlsen commented 4 years ago

@kahootali - WDYT?

kahootali commented 4 years ago

Would it make more sense to the allow-check if arraysize > 0, else accept it? It seems a bit cumbersome to have to list each and every user.

Yes, you are right, if the user has not defined any allowedUsers, then we should allow all and ignore the ones in ignoredUsers only.

Also - should not this check be || instead of &&?

Yes with your suggested change,this should be ||, Previously, this check was that GWP allows commit of a user whose name is both in ignoredUsers as well as allowedUsers

davidkarlsen commented 4 years ago

@kahootali OK - updated the PR. PTAL?