Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cds/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1173,7 +1173,7 @@ def _parse_env_bool(var_name, default=None):
"https://auth.cern.ch/auth/realms/cern/protocol/openid-connect/userinfo",
)

OAUTHCLIENT_CERN_OPENID_ALLOWED_ROLES = ["cern-user"]
OAUTHCLIENT_CERN_OPENID_ALLOWED_ROLES = ["cern-user", "authenticated-user"]

OAUTHCLIENT_CERN_OPENID_REFRESH_TIMEDELTA = timedelta(minutes=-5)
"""Default interval for refreshing CERN extra data (e.g. groups).
Expand Down
3 changes: 3 additions & 0 deletions cds/modules/deposit/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"""CDS interface."""


from cds.modules.ldap.decorators import require_upload_permission
from flask import (
Blueprint,
abort,
Expand Down Expand Up @@ -118,6 +119,7 @@ def to_links_js(pid, deposit=None, dep_type=None):

@blueprint.route("/deposit/reportnumbers/new", methods=["GET", "POST"])
@login_required
@require_upload_permission()
def reserve_report_number():
"""Form to reserver a new report number."""
if not has_read_record_eos_path_permission(current_user, None):
Expand Down Expand Up @@ -156,6 +158,7 @@ def reserve_report_number():
"/deposit/reportnumbers/assign/<string:depid>", methods=["GET", "POST"]
)
@login_required
@require_upload_permission()
def assign_report_number(depid):
"""Form to reserver a new report number."""
if not has_read_record_eos_path_permission(current_user, None):
Expand Down
3 changes: 3 additions & 0 deletions cds/modules/home/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
from invenio_cache.decorators import cached_unless_authenticated
from invenio_i18n import lazy_gettext as _

from ..records.permissions import has_upload_permission

blueprint = Blueprint(
"cds_home",
__name__,
Expand Down Expand Up @@ -58,4 +60,5 @@ def init_menu(app):
"invenio_deposit_ui.index",
_("Upload"),
order=2,
visible_when=lambda: has_upload_permission()
)
2 changes: 2 additions & 0 deletions cds/modules/invenio_deposit/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from flask import request
from invenio_oauth2server import require_api_auth, require_oauth_scopes

from cds.modules.ldap.decorators import require_upload_permission
from .scopes import write_scope


Expand Down Expand Up @@ -84,6 +85,7 @@ def check_oauth2_scope(can_method, *myscopes):

def check(record, *args, **kwargs):
@require_api_auth()
@require_upload_permission()
@require_oauth_scopes(*myscopes)
def can(self):
return can_method(record)
Expand Down
3 changes: 3 additions & 0 deletions cds/modules/invenio_deposit/views/ui.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

from copy import deepcopy

from cds.modules.ldap.decorators import require_upload_permission
from flask import Blueprint, current_app, render_template, request
from flask_login import login_required
from invenio_pidstore.errors import PIDDeletedError
Expand Down Expand Up @@ -73,12 +74,14 @@ def tombstone_errorhandler(error):

@blueprint.route("/deposit")
@login_required
@require_upload_permission()
def index():
"""List user deposits."""
return render_template(current_app.config["DEPOSIT_UI_INDEX_TEMPLATE"])

@blueprint.route("/deposit/new")
@login_required
@require_upload_permission()
def new():
"""Create new deposit."""
deposit_type = request.values.get("type")
Expand Down
17 changes: 17 additions & 0 deletions cds/modules/ldap/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,20 @@ def decorated_api_view(*args, **kwargs):
abort(401)
return func(*args, **kwargs)
return decorated_api_view


def require_upload_permission():
"""Restrict access using the has_upload_permission check."""
def decorator(f):
from cds.modules.records.permissions import has_upload_permission
@wraps(f)
def decorated_function(*args, **kwargs):
if not current_user.is_authenticated:
abort(401)

if not has_upload_permission():
abort(403)

return f(*args, **kwargs)
return decorated_function
return decorator
25 changes: 18 additions & 7 deletions cds/modules/oauthclient/cern_openid.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,10 @@ def find_remote_by_client_id(client_id):

def fetch_extra_data(resource):
"""Return a dict with extra data retrieved from CERN OAuth."""
person_id = resource.get("cern_person_id")
return dict(person_id=person_id, groups=resource["groups"])
data = {"groups": resource.get("groups", [])}
if resource.get("cern_person_id"):
data["person_id"] = resource["cern_person_id"]
return data


def account_roles_and_extra_data(account, resource, refresh_timedelta=None):
Expand Down Expand Up @@ -178,10 +180,19 @@ def _account_info(remote, resp):
resp,
)

email = resource["email"]
external_id = str(resource["cern_uid"])
nice = resource["preferred_username"]
name = resource["name"]
email = resource.get("email")
if not email:
raise OAuthCERNRejectedAccountError("No email in userinfo", remote, resp)

external_id = resource.get("cern_uid") or resource.get("sub")
if not external_id:
raise OAuthCERNRejectedAccountError("No external_id in userinfo", remote, resp)
external_id = str(external_id)
raw_username = resource.get("preferred_username") or email
if "@" in raw_username:
raw_username = raw_username.replace("@", "_").replace(".", "_")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in which cases we have a . and you replace it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we have @ in preferred_username which is email/gmail account, you have ......@gmail.com and as username . also invalid. that's why I'm replacing but it can change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting, we did not do that in CDS-RDM. We are using the sub for the username, see here.

nice = raw_username
name = resource.get("name") or nice
Comment thread
zubeydecivelek marked this conversation as resolved.

return dict(
user=dict(email=email.lower(), profile=dict(username=nice, full_name=name)),
Expand Down Expand Up @@ -231,7 +242,7 @@ def account_setup(remote, token, resp):
resource = get_resource(remote, resp)

with db.session.begin_nested():
external_id = resource.get("cern_uid")
external_id = resource.get("cern_uid") or resource.get("sub")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the sub key here? Is it unique?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes it's unique, See here


# Set CERN person ID in extra_data.
token.remote_account.extra_data = {"external_id": external_id}
Expand Down
14 changes: 12 additions & 2 deletions cds/modules/records/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

from flask import current_app
from flask_security import current_user
from invenio_access import Permission
from invenio_access import Permission, action_factory
from invenio_files_rest.models import Bucket, MultipartObject, ObjectVersion
from invenio_records_files.api import FileObject
from invenio_records_files.models import RecordsBuckets
Expand All @@ -35,6 +35,8 @@
from .utils import get_user_provides, is_deposit, is_record, lowercase_value


upload_access_action = action_factory("videos-upload-access")

def files_permission_factory(obj, action=None):
"""Permission for files are always based on the type of bucket.

Expand Down Expand Up @@ -228,7 +230,7 @@ def can(self):
def create(cls, record, action, user=None):
"""Create a record permission."""
if action in cls.create_actions:
return cls(record, allow, user)
return cls(record, has_upload_permission, user)
elif action in cls.read_actions:
return cls(record, has_read_record_permission, user)
elif action in cls.read_eos_path_actions:
Expand Down Expand Up @@ -334,6 +336,9 @@ def has_update_permission(user, record):
"""Check if user has update access to the record."""
user_id = int(user.get_id()) if user.is_authenticated else None

if not has_upload_permission():
return False

# Allow owners
deposit_creator = record.get("_deposit", {}).get("created_by", -1)
if user_id == deposit_creator:
Expand All @@ -359,3 +364,8 @@ def has_admin_permission(user=None, record=None):
"""
# Allow administrators
return Permission(action_admin_access).can()


def has_upload_permission(*args, **kwargs):
"""Return permission to allow only cern users."""
return Permission(upload_access_action).can()
3 changes: 3 additions & 0 deletions scripts/setup
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,12 @@ cds users create test@test.ch -a --password=123456
# Create an admin user
cds users create admin@test.ch -a --password=123456
cds roles create admin
cds roles create cern-user
cds roles add test@test.ch cern-user
cds roles add admin@test.ch admin
cds access allow deposit-admin-access role admin
cds access allow superuser-access role admin
cds access allow videos-upload-access role cern-user

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remember to apply this when deploying. You might want to already do it in all instances to be sure that you don't forget.


# Create a default files location
cds files location --default videos /tmp/files
Expand Down
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ invenio_oauth2server.scopes =
deposit_actions = cds.modules.invenio_deposit.scopes:actions_scope
invenio_access.actions =
deposit_admin_access = cds.modules.invenio_deposit.permissions:action_admin_access
upload_access_action = cds.modules.records.permissions:upload_access_action
invenio_db.models =
cds_migration_models = cds.modules.legacy.models

Expand Down
23 changes: 23 additions & 0 deletions tests/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
from cds.modules.invenio_deposit.permissions import action_admin_access
from cds.modules.records.resolver import record_resolver
from cds.modules.redirector.views import api_blueprint as cds_api_blueprint
from cds.modules.records.permissions import upload_access_action


@pytest.yield_fixture(scope="module", autouse=True)
Expand Down Expand Up @@ -203,13 +204,35 @@ def users(app, db):
superadmin_role = Role(name="superadmin")
db.session.add(ActionRoles(action=superuser_access.value, role=superadmin_role))
datastore.add_role_to_user(superadmin, superadmin_role)
# Give upload permission to all users
cern_user_role = Role(name="cern-user")
db.session.add(
ActionRoles(action=upload_access_action.value, role=cern_user_role)
)
datastore.add_role_to_user(admin, cern_user_role)
datastore.add_role_to_user(user1, cern_user_role)
datastore.add_role_to_user(user2, cern_user_role)
datastore.add_role_to_user(superadmin, cern_user_role)
db.session.commit()
id_1 = user1.id
id_2 = user2.id
id_4 = admin.id
return [id_1, id_2, id_4]


@pytest.fixture()
def external_user(app, db):
"""Create external user."""
with db.session.begin_nested():
datastore = app.extensions["security"].datastore
user = datastore.create_user(
email="external@gmail.com", password="tester", active=True
)
db.session.commit()
id = user.id
return id


@pytest.fixture()
def u_email(db, users):
"""Valid user email."""
Expand Down
Loading