Skip to content

Commit 85c6766

Browse files
committed
fix(roles): block escalation to protected roles
* Validate role updates at the API level * Prevent regular roles from becoming protected roles * Enforce protected group permissions consistently * Update group identifier references * Add tests for protected roles and super admin access * groups pagination for late-alphabet roles raise groups default_max_results to 50 to allow deeper paging update pagination test expectations to match real page/size handling * fix(schemas): set proper dump only Neither ID nor domain should be allowed to be set from the API since they are automatically assigned either by the database or the email property setter.
1 parent 8ceb965 commit 85c6766

File tree

12 files changed

+185
-56
lines changed

12 files changed

+185
-56
lines changed

invenio_users_resources/config.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -271,12 +271,19 @@
271271
"""Invenio groups admin search configuration."""
272272

273273
USERS_RESOURCES_PROTECTED_GROUP_NAMES = [
274-
"superuser-access",
275274
"admin",
276275
"administration",
276+
"superuser-access",
277277
"administration-moderation",
278278
]
279-
"""Group identifiers that cannot be mutated via the public API (system process only)."""
279+
"""Group identifiers that cannot be mutated via API (system process only).
280+
281+
References:
282+
- superuser-access: https://github.com/inveniosoftware/invenio-access/blob/master/invenio_access/permissions.py
283+
- administration: https://github.com/inveniosoftware/invenio-administration/blob/master/invenio_administration/permissions.py
284+
- admin: https://github.com/inveniosoftware/invenio-cli/blob/master/invenio_cli/commands/services.py (created during instance setup)
285+
- administration-moderation: https://github.com/inveniosoftware/invenio-users-resources/blob/master/invenio_users_resources/permissions.py
286+
"""
280287

281288

282289
class OrgPropsSchema(Schema):

invenio_users_resources/ext.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# Copyright (C) 2022 CERN.
44
# Copyright (C) 2022 TU Wien.
55
# Copyright (C) 2023-2024 Graz University of Technology.
6+
# Copyright (C) 2025 KTH Royal Institute of Technology.
67
#
78
# Invenio-Users-Resources is free software; you can redistribute it and/or
89
# modify it under the terms of the MIT License; see LICENSE file for more

invenio_users_resources/records/api.py

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
from invenio_records_resources.records.api import Record
2828
from invenio_records_resources.records.systemfields import IndexField
2929
from marshmallow import ValidationError
30-
from sqlalchemy import or_
30+
from sqlalchemy import select
3131
from sqlalchemy.exc import NoResultFound
3232

3333
from .dumpers import EmailFieldDumperExt
@@ -142,29 +142,25 @@ def _validate_user_data(user_data):
142142
raise ValidationError(errors)
143143

144144

145-
def _validate_group_data(group_data):
146-
"""Validate data for group creation."""
147-
group_id = group_data.get("id")
148-
name = group_data.get("name")
149-
150-
if not (group_id or name):
145+
def _validate_group_data(data, exclude_id=None):
146+
"""Validate the group data."""
147+
name = data.get("name")
148+
if not name:
151149
return
152150

153-
role = current_datastore.role_model
154-
155-
stmt = db.select(role).where(or_(role.id == group_id, role.name == name)).limit(1)
151+
errors = {}
152+
stmt = select(current_datastore.role_model.id).where(
153+
current_datastore.role_model.name == name
154+
)
155+
if exclude_id is not None:
156+
stmt = stmt.where(current_datastore.role_model.id != exclude_id)
156157

157-
row = db.session.execute(stmt).scalars().first()
158-
if not row:
159-
return
158+
existing_role_id = db.session.execute(stmt).scalar_one_or_none()
160159

161-
errors = {}
162-
if group_id and row.id == group_id:
163-
errors["id"] = [_("Role id already used by another group.")]
164-
if name and row.name == name:
160+
if existing_role_id:
165161
errors["name"] = [_("Role name already used by another group.")]
166-
167-
raise ValidationError(errors)
162+
if errors:
163+
raise ValidationError(errors)
168164

169165

170166
class UserAggregate(BaseAggregate):
@@ -270,9 +266,6 @@ def avatar_color(self):
270266
def create(cls, data, id_=None, validator=None, format_checker=None, **kwargs):
271267
"""Create a new User and store it in the database."""
272268
try:
273-
# Admin group view passes in an empty string as id, which will be a valid Role id.
274-
if "id" in data and data["id"] == "":
275-
data.pop("id")
276269
# Check if email and username already exists by another account.
277270
_validate_user_data(data)
278271
# Create User
@@ -437,6 +430,9 @@ def update(self, data, id_):
437430
if role is None:
438431
role = db.session.get(current_datastore.role_model, id_)
439432

433+
if "name" in data:
434+
_validate_group_data({"name": data["name"]}, exclude_id=role.id)
435+
440436
role.name = data.get("name", role.name)
441437
role.description = data.get("description", role.description)
442438
role = current_datastore.update_role(role)

invenio_users_resources/resources/groups/config.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class GroupSearchRequestArgsSchema(SearchRequestArgsSchema):
3535

3636
def handle_group_validation_error(err):
3737
"""Handle groups errors."""
38-
marshmallow_error = ValidationError(err.errors or {})
38+
marshmallow_error = ValidationError(err.errors)
3939
response = HTTPJSONException(
4040
code=400,
4141
description=err.description,

invenio_users_resources/resources/groups/errors.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,6 @@ class GroupValidationError(GroupsException):
2323

2424
def __init__(self, errors):
2525
"""Marshmallow validation errors."""
26-
self.errors = errors or {}
26+
self.errors = errors
2727
self.description = self.message
2828
super().__init__(self.message)

invenio_users_resources/resources/groups/resource.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ def create_url_rules(self):
3333
routes = self.config.routes
3434
return [
3535
route("GET", routes["list"], self.search),
36-
route("POST", routes["list"], self.create),
3736
route("GET", routes["item"], self.read),
37+
route("POST", routes["list"], self.create),
3838
route("PUT", routes["item"], self.update),
3939
route("DELETE", routes["item"], self.delete),
4040
route("GET", routes["item-avatar"], self.avatar),

invenio_users_resources/services/generators.py

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
# Copyright (C) 2022 TU Wien.
44
# Copyright (C) 2022 CERN.
55
# Copyright (C) 2023 Graz University of Technology.
6-
# Copyright (C) 2024 KTH Royal Institute of Technology.
6+
# Copyright (C) 2024-2025 KTH Royal Institute of Technology.
77
# Copyright (C) 2025 Ubiquity Press.
88
#
99
# Invenio-Users-Resources is free software; you can redistribute it and/or
@@ -186,19 +186,28 @@ def excludes(self, **kwargs):
186186
class ProtectedGroupIdentifiers(Generator):
187187
"""Exclude access to protected groups unless system process."""
188188

189-
def _is_protected(self, record):
190-
protected = current_app.config.get("USERS_RESOURCES_PROTECTED_GROUP_NAMES", [])
189+
def _is_protected(self, record, **kwargs):
190+
"""Check if the role is a protected."""
191+
protected = set(
192+
current_app.config.get("USERS_RESOURCES_PROTECTED_GROUP_NAMES", [])
193+
)
191194
if not protected or record is None:
192195
return False
193196

194-
values = [getattr(record, "id", None), getattr(record, "name", None)]
197+
# create new role
198+
name_dict = record.get("name") if isinstance(record, dict) else None
199+
# update protected role (object form)
200+
name_obj = getattr(record, "name", None)
201+
id_obj = getattr(record, "id", None)
202+
# update role to protected
203+
update_name = kwargs.get("update_grp", {}).get("name")
195204

196-
candidates = {str(val) for val in values if val is not None}
197-
return bool(set(protected).intersection(candidates))
205+
candidates = {str(v) for v in (name_dict, name_obj, id_obj, update_name) if v}
206+
return bool(protected & candidates)
198207

199208
def excludes(self, record=None, identity=None, **kwargs):
200209
"""Exclude non-system identities from protected groups."""
201-
if not self._is_protected(record):
210+
if not self._is_protected(record, **kwargs):
202211
return []
203212
if identity and system_process in identity.provides:
204213
return []

invenio_users_resources/services/groups/config.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,14 @@
2121
from invenio_records_resources.services.base.config import ConfiguratorMixin
2222
from invenio_records_resources.services.records.params import (
2323
FacetsParam,
24+
PaginationParam,
2425
QueryStrParam,
2526
SortParam,
2627
)
2728
from invenio_records_resources.services.records.queryparser import QueryParser
2829

2930
from ...records.api import GroupAggregate
3031
from ..common import EndpointLinkWithId
31-
from ..params import FixedPagination
3232
from ..permissions import GroupsPermissionPolicy
3333
from ..schemas import GroupSchema
3434
from . import facets as groups_facets
@@ -39,8 +39,8 @@ class GroupSearchOptions(SearchOptions):
3939
"""Search options."""
4040

4141
pagination_options = {
42-
"default_results_per_page": 10,
43-
"default_max_results": 10,
42+
"default_results_per_page": 20,
43+
"default_max_results": 50,
4444
}
4545

4646
query_parser_cls = QueryParser.factory(fields=["id", "name"])
@@ -77,7 +77,7 @@ class GroupSearchOptions(SearchOptions):
7777
params_interpreters_cls = [
7878
QueryStrParam,
7979
SortParam,
80-
FixedPagination,
80+
PaginationParam,
8181
FacetsParam,
8282
]
8383

invenio_users_resources/services/groups/service.py

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
from flask import current_app
1616
from invenio_accounts.models import Role
1717
from invenio_db import db
18-
from invenio_i18n import gettext as _
1918
from invenio_records_resources.resources.errors import PermissionDeniedError
2019
from invenio_records_resources.services import RecordService
2120
from invenio_records_resources.services.uow import (
@@ -24,7 +23,6 @@
2423
unit_of_work,
2524
)
2625
from marshmallow import ValidationError
27-
from sqlalchemy.exc import IntegrityError
2826

2927
from ...records.api import GroupAggregate
3028
from ...resources.groups.errors import GroupValidationError
@@ -48,12 +46,10 @@ def create(self, identity, data, raise_errors=True, uow=None):
4846
raise GroupValidationError(err.messages)
4947

5048
try:
49+
# Create group using API
5150
group = self.record_cls.create(data)
52-
except IntegrityError as err:
53-
db.session.rollback()
54-
raise GroupValidationError(
55-
{"name": [_("Role name already used by another group.")]}
56-
) from err
51+
except ValidationError as err:
52+
raise GroupValidationError(err.messages) from err
5753

5854
current_app.logger.debug(
5955
"Group created: '%s' by user %s", group.name, identity.id
@@ -90,7 +86,7 @@ def update(self, identity, id_, data, raise_errors=True, uow=None):
9086
group = GroupAggregate.get_record(id_)
9187
if group is None:
9288
raise PermissionDeniedError()
93-
self.require_permission(identity, "update", record=group)
89+
self.require_permission(identity, "update", record=group, update_grp=data)
9490

9591
try:
9692
data, errors = self.schema.load(
@@ -103,12 +99,10 @@ def update(self, identity, id_, data, raise_errors=True, uow=None):
10399
raise GroupValidationError(err.messages) from err
104100

105101
try:
102+
# Update group using API
106103
group = group.update(data, id_)
107-
except IntegrityError as err:
108-
db.session.rollback()
109-
raise GroupValidationError(
110-
{"name": [_("Role name already used by another group.")]}
111-
) from err
104+
except ValidationError as err:
105+
raise GroupValidationError(err.messages) from err
112106
current_app.logger.debug(
113107
"Group updated: '%s' by user %s", group.name, identity.id
114108
)

invenio_users_resources/services/schemas.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ class UserSchema(BaseRecordSchema, FieldPermissionsMixin):
105105
}
106106

107107
# NOTE: API should only deliver users that are active & confirmed
108+
id = fields.Str(dump_only=True)
108109
active = fields.Boolean()
109110
confirmed = fields.Boolean(dump_only=True)
110111
blocked = fields.Boolean(dump_only=True)
@@ -114,7 +115,7 @@ class UserSchema(BaseRecordSchema, FieldPermissionsMixin):
114115
is_current_user = fields.Method("is_self", dump_only=True)
115116

116117
email = fields.Email(required=True)
117-
domain = fields.String()
118+
domain = fields.String(dump_only=True)
118119
domaininfo = fields.Nested(DomainInfoSchema)
119120
identities = fields.Nested(IdentitiesSchema, dump_default={})
120121
username = fields.String(validate=validate_username)
@@ -144,6 +145,7 @@ def is_self(self, obj):
144145
class GroupSchema(BaseRecordSchema):
145146
"""Schema for user groups."""
146147

148+
id = fields.Str(dump_only=True)
147149
name = fields.String(
148150
required=True,
149151
validate=[

0 commit comments

Comments
 (0)