-
Notifications
You must be signed in to change notification settings - Fork 4
security(oauth): restrict OAuth signing-key settings to system admins #265
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
Open
gonzalesedwin1123
wants to merge
2
commits into
19.0
Choose a base branch
from
security-oauth-signing-keys-acl
base: 19.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1 @@ | ||
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | ||
| access_res_config_settings_spp_oauth_user,res.config.settings spp_oauth user,base.model_res_config_settings,base.group_user,1,1,0,0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| from . import test_rsa_encode_decode | ||
| from . import test_oauth_errors | ||
| from . import test_config_settings_acl |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # Part of OpenSPP. See LICENSE file for full copyright and licensing details. | ||
| """Security: OAuth signing keys must be restricted to system administrators. | ||
|
|
||
| Regression test for "OAuth signing keys exposed to all internal users": the | ||
| module granted ``base.group_user`` read/write on ``res.config.settings``, which | ||
| exposes ``oauth_priv_key`` (the RS256 JWT signing key) and ``oauth_pub_key``. | ||
| Any internal user could therefore: | ||
| - read the model directly (``read``/``search``) via RPC, and | ||
| - retrieve the keys through ``res.config.settings.default_get()``, which reads | ||
| the backing ``ir.config_parameter`` values with ``sudo()`` and performs no | ||
| model-ACL or field-group check. | ||
|
|
||
| Access must require a system administrator (``base.group_system``), and the | ||
| signing keys must not leak through ``default_get`` to non-admins. | ||
| """ | ||
|
|
||
| from odoo import Command | ||
| from odoo.exceptions import AccessError | ||
| from odoo.tests import TransactionCase, tagged | ||
|
|
||
| PRIV_KEY_SENTINEL = "TEST-OAUTH-PRIVATE-KEY-DO-NOT-LEAK" | ||
| PUB_KEY_SENTINEL = "TEST-OAUTH-PUBLIC-KEY" | ||
|
|
||
|
|
||
| @tagged("post_install", "-at_install") | ||
| class TestOAuthConfigSettingsAcl(TransactionCase): | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| super().setUpClass() | ||
| icp = cls.env["ir.config_parameter"].sudo() | ||
| icp.set_param("spp_oauth.oauth_priv_key", PRIV_KEY_SENTINEL) | ||
| icp.set_param("spp_oauth.oauth_pub_key", PUB_KEY_SENTINEL) | ||
|
|
||
| # A plain internal user: base.group_user only, no system access. | ||
| cls.plain_user = cls.env["res.users"].create( | ||
| { | ||
| "name": "Plain Internal User", | ||
| "login": "plain_internal_oauth_test", | ||
| "group_ids": [Command.link(cls.env.ref("base.group_user").id)], | ||
| } | ||
| ) | ||
| # A system administrator. | ||
| cls.admin_user = cls.env["res.users"].create( | ||
| { | ||
| "name": "System Admin User", | ||
| "login": "system_admin_oauth_test", | ||
| "group_ids": [Command.link(cls.env.ref("base.group_system").id)], | ||
| } | ||
| ) | ||
|
|
||
| def test_plain_user_cannot_access_settings_model(self): | ||
| """base.group_user (any internal user) must NOT have read access to | ||
| res.config.settings — that is the model exposing the signing keys.""" | ||
| with self.assertRaises(AccessError): | ||
| self.env["res.config.settings"].with_user(self.plain_user).check_access("read") | ||
|
|
||
| def test_plain_user_cannot_read_keys_via_default_get(self): | ||
| """The signing keys must not leak to a non-admin through default_get(), | ||
| which reads the backing config parameters with sudo().""" | ||
| settings = self.env["res.config.settings"].with_user(self.plain_user) | ||
| values = settings.default_get(["oauth_priv_key", "oauth_pub_key"]) | ||
| self.assertNotIn("oauth_priv_key", values) | ||
| self.assertNotIn("oauth_pub_key", values) | ||
|
|
||
| def test_admin_can_access_settings_model(self): | ||
| """A system administrator retains read access to res.config.settings.""" | ||
| # Raises AccessError only if admin access was wrongly removed. | ||
| self.env["res.config.settings"].with_user(self.admin_user).check_access("read") | ||
|
|
||
| def test_admin_can_read_keys_via_default_get(self): | ||
| """A system administrator can still read the signing keys, so the | ||
| Settings UI keeps working for admins.""" | ||
| settings = self.env["res.config.settings"].with_user(self.admin_user) | ||
| values = settings.default_get(["oauth_priv_key", "oauth_pub_key"]) | ||
| self.assertEqual(values.get("oauth_priv_key"), PRIV_KEY_SENTINEL) | ||
| self.assertEqual(values.get("oauth_pub_key"), PUB_KEY_SENTINEL) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.