-
Notifications
You must be signed in to change notification settings - Fork 117
Disable all commands for change if connection is readonly #2799
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one issue I found:
The deploy command still works even in readonly mode (should be a simple package.json
addition)
Once that's done, I think we can merge.
Great work!
@chrjorgensen I saw you mentioned you're on holiday - let me take over this PR for you. |
Signed-off-by: worksofliam <[email protected]>
@sebjulliand I've added one small commit to this PR. Any chance you can have a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works fine overall @chrjorgensen , that's great; this is true protection 😉
However, I have a remark about the actions.
Also, (but that's a question) It seems odd that even opening a terminal is disabled. I know you can wreck havoc with a terminal, but not being able to open one in this context seems a bit too much. That's up for debate!
@@ -1162,7 +1162,7 @@ | |||
}, | |||
{ | |||
"command": "code-for-ibmi.runAction", | |||
"enablement": "code-for-ibmi:connected", | |||
"enablement": "code-for-ibmi:connected && !code-for-ibmi:isReadonly", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about disabling Run Action
entirely since the actions will be disabled when in read-only anyway, unless the "Enable in protected mode" option is enabled on the action. Disabling it entirely bypasses this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment below...
@sebjulliand Thanks for reviewing this - but there is no such thing as true protection! 😉 I gave this change a lot of thoughts - mostly from a system administrators point of view... and I think we should not leave any options open for the C4i user if the admin has defined the system as readonly in the systemwide settings file Allowing actions in readonly mode gives the user the option to create a new action, which circumvent the protection, e.g. So this PR is created by a system admin 😅 and may seem harsh in its implementation. I'm open for a more granular control, distinguishing between system wide readonly and user defined readonly. This could be added in another PR? Thoughts? 😃 |
Changes
This PR will disable all commands used for changing content on the server, if the connection is marked readonly.
This include terminals and run of actions, but not library list change command or action management etc.
EDIT: C4i will now ask for reload when the readonly value changes in the config.
EDIT: The readonly field has been moved to the Features section as the first option:
EDIT: The readonly connection setting is decribed in the PR #71 in the Docs repo.
EDIT: Save As will now fail with filesystem error if the connection is in readonly mode.
How to test this PR
Checklist