Skip to content

Conversation

@florentx
Copy link
Contributor

@florentx florentx commented Jan 8, 2026

Having group 0 as primary group for odoo user is not optimal.
It was reported by AI review here:

This change reverts to having container user odoo in its own group odoo. And it takes care to set file group appropriately for use-case involving Openshift,

Similar mechanism is in place when DEV_MODE = 1:

  • camptocamp/odoo-template#828

https://www.redhat.com/en/blog/a-guide-to-openshift-and-uids

Copy link
Member

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Seems good

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request changes the user group configuration for the odoo user from using the root group (GID=0) to using a dedicated odoo group (GID=999). This addresses a security concern identified in a previous AI review. The change also adds explicit group ownership settings for the virtual environment to ensure OpenShift compatibility.

Key changes:

  • Changed default GID from 0 (root) to 999 (odoo group)
  • Added explicit groupadd command to create the odoo group before user creation
  • Added chgrp -R root /odoo/.venv to ensure virtual environment is accessible in OpenShift environments

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
19.0/Dockerfile Updated user/group creation from GID=0 to GID=999 with dedicated odoo group, added chgrp for .venv
18.0/Dockerfile Updated user/group creation from GID=0 to GID=999 with dedicated odoo group, added chgrp for .venv
17.0/Dockerfile Updated user/group creation from GID=0 to GID=999 with dedicated odoo group, added chgrp for .venv
16.0/Dockerfile Updated user/group creation from GID=0 to GID=999 with dedicated odoo group, added chgrp for .venv
15.0/Dockerfile Updated user/group creation from GID=0 to GID=999 with dedicated odoo group, added chgrp for .venv
14.0/Dockerfile Updated user/group creation from GID=0 to GID=999 with dedicated odoo group, added chgrp for .venv

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

&& python3 -m venv /odoo/.venv --system-site-packages \
&& /odoo/.venv/bin/pip install -r /odoo/base_requirements.txt \
&& /odoo/.venv/bin/pip install -r /odoo/extra_requirements.txt" \
&& chgrp -R root /odoo/.venv \
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

The install command on line 10 uses -o odoo to set the owner but doesn't explicitly set the group. After the change from GID=0 to GID=999, these directories will be owned by group odoo (GID 999) instead of root (GID 0). However, line 63 only changes the group of /odoo/.venv to root, leaving other directories (like /var/log/odoo, /odoo/src, /odoo/data, etc.) with group odoo. For OpenShift compatibility where containers run with arbitrary UIDs but GID 0, these directories should also have their group set to root to ensure proper access permissions.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@florentx florentx Jan 8, 2026

Choose a reason for hiding this comment

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

This statement is wrong.

When install command is used without -g, it sets file group to process' current group (see install --help).
In this case build process is run as root with group root.

I've checked the resulting image, and can confirm that file/directory ownerships are correctly set.

@florentx florentx merged commit bcca6ed into master Jan 9, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants