Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Neo CLI New Feature] Allow custom plugins #3295

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Jun 6, 2024

Description

Currently we only have a default download url for plugins, this makes it hard for users to add custom plugin urls. Thus i create this pr to allow user to add their own custom plugin download url.

Why not using existing download url directly?

Cause user might not only want to have their own customed plugin, but also want to use default plugins.

Fixes #

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Test Configuration:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Jim8y Jim8y added blocked This issue can't be worked at the moment waiting for review labels Jun 6, 2024
@Jim8y
Copy link
Contributor Author

Jim8y commented Jun 6, 2024

@superboyiii may you please test it

@vncoelho
Copy link
Member

vncoelho commented Jun 6, 2024

@lingyido, can you take a look at this version?

@@ -24,7 +24,8 @@
"NeoNameService": "0x7061fbd31562664b58f422c3dee4acfd70dba8af"
},
"Plugins": {
"DownloadUrl": "https://api.github.com/repos/neo-project/neo-modules/releases"
"DownloadUrl": "https://api.github.com/repos/neo-project/neo/releases",
"CustomUrls": []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the CustomUrls is not so important in my opinion, the important thing now is to show the installed plugins correctly, if they are custom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can just change this download URL, and release on their github already.

@Jim8y
Copy link
Contributor Author

Jim8y commented Jun 6, 2024

@lingyido, can you take a look at this version?

isnt the list thing already fixed by chris?

@vncoelho
Copy link
Member

vncoelho commented Jun 6, 2024

isnt the list thing already fixed by chris?

As i understood from his last message in the issue (that I re-opened), the custom plugins were not being printed. I did not test yet.

@cschuchardt88
Copy link
Member

isnt the list thing already fixed by chris?

As i understood from his last message in the issue (that I re-opened), the custom plugins were not being printed. I did not test yet.

That is correct

@lingyido
Copy link
Contributor

lingyido commented Jun 6, 2024

@lingyido, can you take a look at this version?

From my own view, I don't need this one and won't use this feature at all. Sorry @Jim8y. I don't want to hurt you here. I'm just telling my feelings.

And maybe it's helpful for others to distribute their own plugins.

I only want to build an extension myself and put it into my neo-node's Plugin folder. Hope that #3268 has been fixed and I can see my own plugin's information by plugins command. I won't update my neo-cli until next version is released because I don't trust this master branch.

Actually, it's not safe to allow others to distribute plugins in this way. Any malicious developer could develop a plugin and then tell others that they can download by adding his link to their config file. If one day he wants to do harm, he would simply change his binary plugin and stole users' secret.

Now that the users trust the official binary and download the neo-cli, I think it's somehow reasonable to put a trustable official plugin repo their. But custom repo is actually dangerous for users that has low security consciousness. And what's more, I hate that users can change the official plugin link by config file because it could also be a good way for bad ones to stole private key.

@Jim8y
Copy link
Contributor Author

Jim8y commented Jun 6, 2024

@lingyido, can you take a look at this version?

From my own view, I don't need this one and won't use this feature at all. Sorry @Jim8y. I don't want to hurt you here. I'm just telling my feelings.

And maybe it's helpful for others to distribute their own plugins.

I only want to build an extension myself and put it into my neo-node's Plugin folder. Hope that #3268 has been fixed and I can see my own plugin's information by plugins command. I won't update my neo-cli until next version is released because I don't trust this master branch.

Actually, it's not safe to allow others to distribute plugins in this way. Any malicious developer could develop a plugin and then tell others that they can download by adding his link to their config file. If one day he wants to do harm, he would simply change his binary plugin and stole users' secret.

Now that the users trust the official binary and download the neo-cli, I think it's somehow reasonable to put a trustable official plugin repo their. But custom repo is actually dangerous for users that has low security consciousness. And what's more, I hate that users can change the official plugin link by config file because it could also be a good way for bad ones to stole private key.

this is not a pr for your concern, i have no idea why its connected. your issue should have being fixed by chris. as to the security concern to the dowload url, its always configurable, node config security is the duty of the node owner. you cant blame the lock if the owner lost his key.

@lingyido
Copy link
Contributor

lingyido commented Jun 6, 2024

this is not a pr for your concern

Sorry. I'm here because of @vncoelho's invitation. @lingyido, can you take a look at this version?

This PR itself has nothing to do with my issue.

as to the security concern to the dowload url, its always configurable, node config security is the duty of the node owner. you cant blame the lock if the owner lost his key.

You're right. That's why I'm saying it's my own feeling. I have no right on this PR. I respect everyone's effort for improving NEO, especially you who contributed a lot.

And I like the 'lock' analogy. If a smart lock leaves an remote admin configuration, I personally will hate it. Although it provides the ability that you can unlock it remotely by smart phone or other things. From my view, it leaves a easy way for bad guys to hack in.

And I'm not going to debate with you on this. Let you developers focus on the PR itself.

Really sorry for carrying too many personal views here. Wish you all a good day and wish NEO GOTOTHEMOON.

@Jim8y
Copy link
Contributor Author

Jim8y commented Jun 6, 2024

Really sorry for carrying too many personal views here. Wish you all a good day and wish NEO GOTOTHEMOON.

@lingyido Totally understand. Please stop saying sorry anymore, i am more than happy to see people join the discussion.

@Jim8y Jim8y removed blocked This issue can't be worked at the moment need update labels Jun 7, 2024
@Jim8y Jim8y requested a review from vncoelho June 10, 2024 14:00
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the change but I do not use this feature. I will wait for someone that uses the link to test it.

@NGDAdmin NGDAdmin merged commit e0ba897 into neo-project:master Jun 14, 2024
8 checks passed
Jim8y added a commit to Jim8y/neo that referenced this pull request Jun 14, 2024
* master:
  Fixed pathing and Property (neo-project#3306)
  [Neo CLI New Feature] Allow custom plugins (neo-project#3295)
  Return expect to verify neo-cli (neo-project#3318)
  [Neo CLI Optimization] fix exception message (neo-project#3314)
  fix plugin download url (neo-project#3329)
@Jim8y Jim8y deleted the custom-plugins branch July 19, 2024 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants