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

[14.0] group_backend: new module #116

Closed
wants to merge 4 commits into from

Conversation

petrus-v
Copy link

Parts of the module description

This module was written to extend the standard functionality regarding users
and groups management by adding a new Backend user group that only gives access
to odoo backend (/web):

  • no default access
  • no default menu
  • no default access rules
  • no default actions
  • no default views

The problem with the Internal user is when you want to gives access to the
backend to a really thin part of your business to some users, it's quite hard
to properly maintain those roles over the project life, a lot of models use
that group (base.group_user) by default which makes hard
to maintains.

So that helps creating well-defined user groups with more controls.

@petrus-v
Copy link
Author

This depends on odoo/odoo#68106 with
odoo PR: odoo/odoo#68111
or OCA PR: OCA/OCB#1005

makes hard to maintain proper access right according your business.
"""
res = super(Users, self).has_group(group_ext_id)
if not res and group_ext_id == "base.group_user":
Copy link

@jcdrubay jcdrubay Mar 19, 2021

Choose a reason for hiding this comment

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

I think this is a smart hack avoiding a lot of mess in the code.

However it makes the behavior of this module unpredictable in long term. At anytime, the native code of Odoo could be changed to rely on has_group to grant additional access and then we would loose control again on what the users of that group could or could not do. And since this is touching security, this is one of the few things that might change within the same major version of Odoo.

It seems that web module only relies one time on self.env.user.has_group('base.group_user') and for base module there are 5 cases of usage (some might not be easy to override). And it might be acceptable to not override all cases, because overriding would only make sense if you want to give the permission which might not be needed for this very special user type. So overriding only the few necessary places could make sense.

I don't think portal module is necessary to take into consideration as it is not really "core" (it's not auto_install anymore in v14).

In my opinion, the code of this PR could:

  • stay as it is if highlighting more the risk
    OR
  • add the code to override wherever necessary

To highlight the risk:

  • the content of this docstring can be added as a Limitation in the Readme
  • a warning log could alert that we are bypassing the expected behavior of has_group.
if not res and group_ext_id == "base.group_user":
    has_group_backend = super(Users, self).has_group("group_backend.group_backend")
    if has_group_backend:
        logging.waning("Forcing has_group to return True for group_backend")
    return has_group_backend

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for your feed backs.

I've fist had a try to override the code in different parts but was no very confident as behavior would change according module dependency tree if portal or website are installed this would make difficult to predict which one will be called last (thinking Home._login_redirect or Home.index methods) and I don't want to depends on such modules.

So I think it's very good Idea to add a warning log and prevent against risk in README limitation.

I'll do that first.

As a developer we have to keep in mind using this module and grant a user with 's group is
equivalent to grant 's group everywhere  has been used.
@dreispt
Copy link
Member

dreispt commented Sep 21, 2021

To follow the name conventions being used, i think the module name should be base_group_backend

Copy link

@jcdrubay jcdrubay left a comment

Choose a reason for hiding this comment

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

fix some typos

petrus-v and others added 2 commits September 22, 2021 09:28
Co-authored-by: Jean-Charles Drubay <[email protected]>
apply typo review suggestion

Co-authored-by: Jean-Charles Drubay <[email protected]>
@petrus-v
Copy link
Author

To follow the name conventions being used, i think the module name should be base_group_backend

Thanks for suggestion I will adding that in my todo list !

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 23, 2022
@github-actions github-actions bot closed this Feb 27, 2022
@bealdav
Copy link
Member

bealdav commented Mar 3, 2023

see 16.0 version of the module #205

@petrus-v petrus-v mentioned this pull request Aug 19, 2023
SiesslPhillip pushed a commit to grueneerde/OCA-server-backend that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-backend (14.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants