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.

Sign up for GitHub

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

Jump to bottom

[14.0] [IMP] cetmix_tower_server: command log access #59

Merged
merged 1 commit into from
Oct 25, 2024
Merged

[14.0] [IMP] cetmix_tower_server: command log access #59

merged 1 commit into from
Oct 25, 2024

Conversation

Aldeigja
Copy link
Contributor

Restrict access to command log:
User: can see only log records he created.
Manager: + can see all log records for servers he has access to.
Root: can see all records.

@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from a7ccd68 to 11252e1 Compare May 24, 2024 15:10
@Aldeigja Aldeigja changed the title [14.0] [IMP] command log access [14.0] [IMP] cetmix_tower_server: command log access May 24, 2024
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from 8eefe62 to 1f21d1b Compare May 28, 2024 22:57
ivs-cetmix
ivs-cetmix approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

Code review LGTM

@CetmixGitDrone
Copy link

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Bearnard21
Bearnard21 approved these changes Jun 9, 2024
View reviewed changes
Copy link
Contributor

@Bearnard21 Bearnard21 left a comment

Choose a reason for hiding this comment

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

Functional LGTM

@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from 1f21d1b to 3411c5a Compare June 19, 2024 09:53
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from 3411c5a to e236e6e Compare June 26, 2024 22:39
ivs-cetmix
ivs-cetmix requested changes Jun 27, 2024
View reviewed changes
cetmix_tower_server/security/cx_tower_command_log_security.xml Outdated
<field name="groups" eval="[(4, ref('cetmix_tower_server.group_user'))]" />
<field
name="domain_force"
>[('create_uid', '=', user.id), ('command_id.access_level', '=', '1')]</field>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add related stored "access_level" field to the command log model to improve read performance?

cetmix_tower_server/security/cx_tower_command_log_security.xml Outdated
<record id="cx_tower_command_log_rule_group_manager_access" model="ir.rule">
<field name="name">Tower command log: manager access rule</field>
<field name="model_id" ref="model_cx_tower_command_log" />
<field name="domain_force">[
Copy link
Contributor

Choose a reason for hiding this comment

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

This may lead to an "Access denied" exception when Manager group member will try to open Root group restricted command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this test case satisfy your question?

cetmix_tower_server/tests/test_command_log.py Outdated
],
)
# Ensure that user_bob has access to test_command_log_1
command_name_1 = test_command_log_1.with_user(self.user_bob).read(["name"])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a command name. It is a command log name.
However it would be good to test if Manager can read command log of a command with "Root" access level.
For this we need to command(!) access level to "root" and than read not only name but also command_id field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed variable name and added tests requested

@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from e236e6e to e82b19e Compare June 30, 2024 18:32
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch 2 times, most recently from 05fdeb1 to 942b26b Compare July 17, 2024 21:32
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from 942b26b to a6e7f14 Compare August 14, 2024 20:41
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch 2 times, most recently from d7fa65c to f386aa1 Compare August 30, 2024 21:44
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch 2 times, most recently from f8ea47f to f9ee2bc Compare September 23, 2024 09:19
@Aldeigja
Copy link
Contributor Author

Aldeigja commented Oct 1, 2024

/ocabot rebase

@CetmixGitDrone
Copy link

@Aldeigja The rebase process failed, because command git rebase origin/14.0-dev failed with output:

Rebasing (1/9)
Rebasing (2/9)
Auto-merging cetmix_tower_server/tests/__init__.py
CONFLICT (content): Merge conflict in cetmix_tower_server/tests/__init__.py
error: could not apply 6af9be2... [IMP] cetmix_tower_server: added tests for cetmix tower server command log
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 6af9be2... [IMP] cetmix_tower_server: added tests for cetmix tower server command log

norlinhenrik
norlinhenrik approved these changes Oct 13, 2024
View reviewed changes
Copy link

@norlinhenrik norlinhenrik left a comment

Choose a reason for hiding this comment

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

I tested with 3 servers and 3 users, and the access restrictions work.

I was not able to test ('create_uid', '=', user.id) for a Tower User, since I couldn't run a command as a user due to a path restriction.
image

@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch 2 times, most recently from 9045ce3 to 47e299f Compare October 18, 2024 11:51
@Aldeigja Aldeigja mentioned this pull request Oct 18, 2024
Merged
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch 2 times, most recently from 265ffdb to 470719f Compare October 24, 2024 07:13
@Aldeigja
Copy link
Contributor Author

I tested with 3 servers and 3 users, and the access restrictions work.

I was not able to test ('create_uid', '=', user.id) for a Tower User, since I couldn't run a command as a user due to a path restriction.

Hi @norlinhenrik we've fixed this issue please test if you have opportunity.

ivs-cetmix
ivs-cetmix approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

Code review LGTM

@CetmixGitDrone
Copy link

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from 470719f to 536598d Compare October 25, 2024 08:25
@Aldeigja
[IMP] cetmix_tower_server: restrict access to command log
514bc1a 
Before this commit:

All users who have access to the server can view its command log

After this commit:

User: can see only log records he created.
Manager: + can see all log records for servers he has access to.
Root: can see all records.
@Aldeigja Aldeigja force-pushed the 14.0-t3630-cetmix_tower_server-command-log-access branch from 536598d to 514bc1a Compare October 25, 2024 08:27
@ivs-cetmix
Copy link
Contributor

/ocabot merge nobump

@CetmixGitDrone
Copy link

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-dev-ocabot-merge-pr-59-by-ivs-cetmix-bump-nobump, awaiting test results.

@CetmixGitDrone CetmixGitDrone merged commit 6afcfb2 into 14.0-dev Oct 25, 2024
5 checks passed
@CetmixGitDrone
Copy link

Congratulations, your PR was merged at 5f15d22. Thanks a lot for contributing to cetmix. ❤️

@CetmixGitDrone CetmixGitDrone deleted the 14.0-t3630-cetmix_tower_server-command-log-access branch October 25, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivs-cetmix ivs-cetmix ivs-cetmix approved these changes

@norlinhenrik norlinhenrik norlinhenrik approved these changes

@Bearnard21 Bearnard21 Bearnard21 approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 needs review ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Aldeigja @CetmixGitDrone @ivs-cetmix @norlinhenrik @Bearnard21