webhooks #36
webhooks #36
Conversation
- using composer autoloader - new settings -- ip protection -- webhook settings
|
WoW, that's a great feature! Thank you for your contribution. I'll need some time to review and provide you with constructive feedback, but I will try to come back to you soon. |
|
@keywan-ghadami-oxid I'm sorry I haven't had the time to give you feedback on this, but I don't have much time lately. |
There was a problem hiding this comment.
global nitpick: Some functions and properties have documentations, others have not, others have mostly useless ones (e.g., @var $projects Projects). Please, make sure this is consistent. I don't mind having some trivial/obvious functions without a comment. But if there is one, it should be meaningful.
@keywan-ghadami-oxid Sorry for the (very long) delay.
I finally had/took the time to have a good look at your PR.
It's great work and I really like the way it's going.
Most of my comments are nitpick or misc Code Style notes.
However, a couple of them are a bit more important.
| * User: keywan | ||
| * Date: 30.07.18 | ||
| * Time: 08:49 | ||
| */ |
There was a problem hiding this comment.
nitpick: I'd prefer if we didn't have editor-specific comments. I'm definitely fine with an @author, but I'd prefer we didn't have the "Created by PhpStorm" comment. The date and time are somewhat irrelevant as well.
If you're to include an @author comment. Please include a link to your github profile or a public email address.
| * @link http://www.oxid-esales.com | ||
| * @copyright (C) OXID eSales AG | ||
| * Created at 7/30/18 1:08 PM by Keywan Ghadami | ||
| */ |
There was a problem hiding this comment.
I'm a bit concerned by the Copyright notice here. Especially the "it is NOT Freeware" mention.
Since it's the only file with it, I believe you simply forgot it... But please, remove it.
As I mentioned earlier, I'm totally fine with the @author comment (although I'd prefer something that would allow to reach you easily via github or email).
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
nitpick: I'm not sure about the point of this empty file...
| * User: keywan | ||
| * Date: 30.07.18 | ||
| * Time: 08:40 | ||
| */ |
There was a problem hiding this comment.
nitpick: Same comment as for Auth.php.
| // See ../confs/samples/gitlab.ini | ||
| $config_file = __DIR__ . '/../confs/gitlab.ini'; | ||
| if (!file_exists($config_file)) { | ||
| header('HTTP/1.0 500 Internal Server Error'); |
There was a problem hiding this comment.
cs: You sometime use http_response_code, sometime header (as here). Please make them consistent.
| $confs = (new Config())->getConfs(); | ||
| $a=new AuthWebhook(); | ||
| $a->setConfig($confs); | ||
| $a->auth(); |
There was a problem hiding this comment.
If IP restrictions apply. This will require having the Gitlab server in the set of allowed IPs.
This might become an issue with complex setups.
What do you think about separating the IP restriction config for hooks and regular queries?
| $ref_name = str_replace('refs/tags/','', $ref_name); | ||
| $ref_name = str_replace('refs/heads/','', $ref_name); | ||
|
|
||
| $ref = ['name'=>$ref_name, 'commit' => ['id'=> $data['checkout_sha']]]; |
There was a problem hiding this comment.
Since we're extracting data directly from the hook, it would be great to have a way to double check the authenticity of the hook call.
Gitlab offers secret tokens (https://docs.gitlab.com/ee/user/project/integrations/webhooks.html#secret-token), It might a good idea to provide configuration for the secret tokens? (maybe per package?)
| * @param array $project | ||
| * @return array Same as $fetch_ref, but for all refs | ||
| */ | ||
| $fetch_refs = function ($project) use ($repos) { |
There was a problem hiding this comment.
Since we are going toward OOP, shouldn't we exploit it fully and replace these closures with methods?
| * @param array $project | ||
| * @return array Same as $fetch_refs | ||
| */ | ||
| $load_data = function ($project) use ($fetch_refs) { |
There was a problem hiding this comment.
Since we are going toward OOP, shouldn't we exploit it fully and replace these closures with methods?
| * @param array $project | ||
| * @return string The name of the project | ||
| */ | ||
| $get_package_name = function ($project) use ($repos) { |
There was a problem hiding this comment.
Since we are going toward OOP, shouldn't we exploit it fully and replace these closures with methods?
|
@lemoinem thank you for the feedback, I agree on all points. Unfortunately I am not sure if I will find time to fix that soon. To all: any help by any volunteers are welcome, e.g. just
|
Webhooks
This is feature allows to get immediately updates to the composer registry if a new version (tag) was released.
It comes with a lot of new security settings e.g. a webhook token can be defined, that is used to authenticate requests from gitlab, and a webhook url that is used to define the self url in a secure way. also some ip settings to restrict access to certain ips.
Performance of webhooks is good, because they write directly in the cache files and by that they are avoiding the cache to become stale.
You may wonder about huge refactoring
Refactoring was made to make the script make object oriented with the idea of being able to reuse things easier and to separate things into different files. There is room for improvements because not everything was refactored yet. BTW the old code had it's beauty, anyway when continuing adding features it will hopefully be a benefit to move into this object oriented direction.
Outlook
The webhooks can be used to preserve the cache for a long time, but there is a known limitation: deleting a version/tag is not covered by the current logic (because it is not my major use case) and should be implemented in future to make things feature complete.
Based on the refactoring maybe someone will add more features. The new authentication layer would allow to add new features e. G. a gitlab token based authenticate as alternative or addition to the IP based authentication.