From 2b651cfbed51c470f160da90674c753a73316270 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 2 Dec 2024 20:43:26 +0100 Subject: [PATCH] =?UTF-8?q?Misc=20fixes=20@=5F@=E2=81=B5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- helpers/helpers.v1.d/permission | 32 +++--------- src/permission.py | 91 ++++++++++++++++++++------------- src/tests/test_permission.py | 7 +-- 3 files changed, 68 insertions(+), 62 deletions(-) diff --git a/helpers/helpers.v1.d/permission b/helpers/helpers.v1.d/permission index c81c16fa99..2193d00abc 100644 --- a/helpers/helpers.v1.d/permission +++ b/helpers/helpers.v1.d/permission @@ -3,7 +3,7 @@ # Create a new permission for the app # # Example 1: `ynh_permission_create --permission=admin --url=/admin --additional_urls=domain.tld/admin /superadmin --allowed=alice bob \ -# --label="My app admin" --show_tile=true` +# --show_tile=true` # # This example will create a new permission permission with this following effect: # - A tile named "My app admin" in the SSO will be available for the users alice and bob. This tile will point to the relative url '/admin'. @@ -13,7 +13,7 @@ # Example 2: # # ynh_permission_create --permission=api --url=domain.tld/api --auth_header=false --allowed=visitors \ -# --label="MyApp API" --protected=true +# --protected=true # # This example will create a new protected permission. So the admin won't be able to add/remove the visitors group of this permission. # In case of an API with need to be always public it avoid that the admin break anything. @@ -25,14 +25,13 @@ # # # usage: ynh_permission_create --permission="permission" [--url="url"] [--additional_urls="second-url" [ "third-url" ]] [--auth_header=true|false] -# [--allowed=group1 [ group2 ]] [--label="label"] [--show_tile=true|false] +# [--allowed=group1 [ group2 ]] [--show_tile=true|false] # [--protected=true|false] # | arg: -p, --permission= - the name for the permission (by default a permission named "main" already exist) # | arg: -u, --url= - (optional) URL for which access will be allowed/forbidden. Note that if 'show_tile' is enabled, this URL will be the URL of the tile. # | arg: -A, --additional_urls= - (optional) List of additional URL for which access will be allowed/forbidden # | arg: -h, --auth_header= - (optional) Define for the URL of this permission, if SSOwat pass the authentication header to the application. Default is true # | arg: -a, --allowed= - (optional) A list of group/user to allow for the permission -# | arg: -l, --label= - (optional) Define a name for the permission. This label will be shown on the SSO and in the admin. Default is "APP_LABEL (permission name)". # | arg: -t, --show_tile= - (optional) Define if a tile will be shown in the SSO. If yes the name of the tile will be the 'label' parameter. Defaults to false for the permission different than 'main'. # | arg: -P, --protected= - (optional) Define if this permission is protected. If it is protected the administrator won't be able to add or remove the visitors group of this permission. Defaults to 'false'. # @@ -66,13 +65,12 @@ ynh_permission_create() { # Declare an array to define the options of this helper. local legacy_args=puAhaltP - local -A args_array=([p]=permission= [u]=url= [A]=additional_urls= [h]=auth_header= [a]=allowed= [l]=label= [t]=show_tile= [P]=protected=) + local -A args_array=([p]=permission= [u]=url= [A]=additional_urls= [h]=auth_header= [a]=allowed= [t]=show_tile= [P]=protected=) local permission local url local additional_urls local auth_header local allowed - local label local show_tile local protected ynh_handle_getopts_args "$@" @@ -80,7 +78,6 @@ ynh_permission_create() { additional_urls=${additional_urls:-} auth_header=${auth_header:-} allowed=${allowed:-} - label=${label:-} show_tile=${show_tile:-} protected=${protected:-} @@ -116,12 +113,6 @@ ynh_permission_create() { allowed=",allowed=['${allowed//;/\',\'}']" fi - if [[ -n ${label:-} ]]; then - label=",label='$label'" - else - label=",label='$permission'" - fi - if [[ -n ${show_tile:-} ]]; then if [ $show_tile == "true" ]; then show_tile=",show_tile=True" @@ -138,7 +129,7 @@ ynh_permission_create() { fi fi - yunohost tools shell -c "from yunohost.permission import permission_create; permission_create('$app.$permission' $url $additional_urls $auth_header $allowed $label $show_tile $protected)" + yunohost tools shell -c "from yunohost.permission import permission_create; permission_create('$app.$permission' $url $additional_urls $auth_header $allowed $show_tile $protected)" } # Remove a permission for the app (note that when the app is removed all permission is automatically removed) @@ -248,11 +239,10 @@ ynh_permission_url() { # Update a permission for the app # # usage: ynh_permission_update --permission "permission" [--add="group" ["group" ...]] [--remove="group" ["group" ...]] -# [--label="label"] [--show_tile=true|false] [--protected=true|false] +# [--show_tile=true|false] [--protected=true|false] # | arg: -p, --permission= - the name for the permission (by default a permission named "main" already exist) # | arg: -a, --add= - the list of group or users to enable add to the permission # | arg: -r, --remove= - the list of group or users to remove from the permission -# | arg: -l, --label= - (optional) Define a name for the permission. This label will be shown on the SSO and in the admin. # | arg: -t, --show_tile= - (optional) Define if a tile will be shown in the SSO # | arg: -P, --protected= - (optional) Define if this permission is protected. If it is protected the administrator won't be able to add or remove the visitors group of this permission. # @@ -260,17 +250,15 @@ ynh_permission_url() { ynh_permission_update() { # Declare an array to define the options of this helper. local legacy_args=parltP - local -A args_array=([p]=permission= [a]=add= [r]=remove= [l]=label= [t]=show_tile= [P]=protected=) + local -A args_array=([p]=permission= [a]=add= [r]=remove= [t]=show_tile= [P]=protected=) local permission local add local remove - local label local show_tile local protected ynh_handle_getopts_args "$@" add=${add:-} remove=${remove:-} - label=${label:-} show_tile=${show_tile:-} protected=${protected:-} @@ -293,10 +281,6 @@ ynh_permission_update() { remove=",remove=['${remove//';'/"','"}']" fi - if [[ -n $label ]]; then - label=",label='$label'" - fi - if [[ -n $show_tile ]]; then if [ $show_tile == "true" ]; then show_tile=",show_tile=True" @@ -313,7 +297,7 @@ ynh_permission_update() { fi fi - yunohost tools shell -c "from yunohost.permission import user_permission_update; user_permission_update('$app.$permission' $add $remove $label $show_tile $protected , force=True)" + yunohost tools shell -c "from yunohost.permission import user_permission_update; user_permission_update('$app.$permission' $add $remove $show_tile $protected , force=True)" } # Check if a permission has an user diff --git a/src/permission.py b/src/permission.py index c948989cf2..17c9c2308c 100644 --- a/src/permission.py +++ b/src/permission.py @@ -455,7 +455,7 @@ def permission_create( new_permission = _update_ldap_group_permission( permission=permission, - allowed=allowed, + allowed=allowed or [], sync_perm=sync_perm, ) @@ -511,23 +511,23 @@ def permission_url( app_main_path = domain + path # Fetch existing permission + update_settings = {} + existing_permission = app_setting(app, "_permissions") or {} + if sub_permission not in existing_permission: + existing_permission[sub_permission] = {} + existing_permission = existing_permission[sub_permission] - existing_permission = user_permission_info(permission) - - show_tile = existing_permission["show_tile"] - - if url is None: - url = existing_permission["url"] - else: + if url is not None: url = _validate_and_sanitize_permission_url(url, app_main_path, app) + update_settings["url"] = url - if url.startswith("re:") and existing_permission["show_tile"]: + if url.startswith("re:") and existing_permission.get("show_tile"): logger.warning( m18n.n("regex_incompatible_with_tile", regex=url, permission=permission) ) - show_tile = False + update_settings["show_tile"] = False - current_additional_urls = existing_permission["additional_urls"] + current_additional_urls = existing_permission.get("additional_urls", []) new_additional_urls = copy.copy(current_additional_urls) if add_url: @@ -556,19 +556,18 @@ def permission_url( if set_url: new_additional_urls = set_url - if auth_header is None: - auth_header = existing_permission["auth_header"] + # Guarantee uniqueness of all values, which would otherwise make ldap.update angry. + update_settings["additional_urls"] = list(set(new_additional_urls)) - if clear_urls: - url = None - new_additional_urls = [] - show_tile = False + if auth_header is not None: + update_settings["auth_header"] = auth_header - # Guarantee uniqueness of all values, which would otherwise make ldap.update angry. - new_additional_urls = set(new_additional_urls) + if clear_urls: + update_settings["url"] = None + update_settings["additional_urls"] = [] + update_settings["show_tile"] = False # Actually commit the change - operation_logger.related_to.append(("app", app)) operation_logger.start() @@ -576,12 +575,8 @@ def permission_url( perm_settings = app_setting(app, "_permissions") or {} if sub_permission not in perm_settings: perm_settings[sub_permission] = {} - perm_settings[sub_permission].update({ - "url": url, - "additional_urls": list(new_additional_urls), - "auth_header": auth_header, - "show_tile": show_tile, - }) + + perm_settings[sub_permission].update(update_settings) app_setting(app, "_permissions", perm_settings) except Exception as e: raise YunohostError("permission_update_failed", permission=permission, error=e) @@ -610,6 +605,7 @@ def permission_delete(operation_logger, permission, force=False, sync_perm=True) raise YunohostValidationError("permission_cannot_remove_main") from yunohost.utils.ldap import _get_ldap_interface + from yunohost.app import app_setting, _is_installed ldap = _get_ldap_interface() @@ -629,6 +625,13 @@ def permission_delete(operation_logger, permission, force=False, sync_perm=True) "permission_deletion_failed", permission=permission, error=e ) + app, subperm = permission.split(".") + if _is_installed(app): + perm_settings = app_setting(app, "_permissions") or {} + if subperm in perm_settings: + del perm_settings[subperm] + app_setting(app, "_permissions", perm_settings) + if sync_perm: permission_sync_to_user() logger.debug(m18n.n("permission_deleted", permission=permission)) @@ -701,6 +704,9 @@ def _update_app_permission_setting(permission, label=None, show_tile=None, prote app, sub_permission = permission.split(".") update_settings = {} + perm_settings = app_setting(app, "_permissions") or {} + if sub_permission not in perm_settings: + perm_settings[sub_permission] = {} if app in SYSTEM_PERMS: logger.warning(f"Can't change label / show_tile / protected for system permission {permission}") @@ -713,9 +719,10 @@ def _update_app_permission_setting(permission, label=None, show_tile=None, prote update_settings["protected"] = protected if show_tile is not None: - existing_permission = user_permission_info(permission) + update_settings["show_tile"] = show_tile + existing_permission_url = perm_settings[sub_permission].get("url") if show_tile is True: - if not existing_permission["url"]: + if not existing_permission_url: logger.warning( m18n.n( "show_tile_cant_be_enabled_for_url_not_defined", @@ -723,7 +730,7 @@ def _update_app_permission_setting(permission, label=None, show_tile=None, prote ) ) update_settings["show_tile"] = False - elif existing_permission["url"].startswith("re:"): + elif existing_permission_url.startswith("re:"): logger.warning( m18n.n("show_tile_cant_be_enabled_for_regex", permission=permission) ) @@ -733,9 +740,6 @@ def _update_app_permission_setting(permission, label=None, show_tile=None, prote label = update_settings.pop("label") app_setting(app, "label", label) - perm_settings = app_setting(app, "_permissions") or {} - if sub_permission not in perm_settings: - perm_settings[sub_permission] = {} perm_settings[sub_permission].update(update_settings) app_setting(app, "_permissions", perm_settings) @@ -756,20 +760,37 @@ def _update_ldap_group_permission(permission, allowed, sync_perm=True): """ from yunohost.hook import hook_callback - from yunohost.utils.ldap import _get_ldap_interface + from yunohost.utils.ldap import _get_ldap_interface, _ldap_path_extract ldap = _get_ldap_interface() app, sub_permission = permission.split(".") update_ldap = {} - existing_permission = user_permission_info(permission) + # NB: this must be fetched *BEFORE* we modifiy the perm + existing_permission = ldap.search( + "ou=permission", + f"(&(objectclass=permissionYnh)(cn={permission}))", + [ + "cn", + "groupPermission", + "inheritPermission", + ], + ) + if existing_permission: + existing_permission = existing_permission[0] + else: + logger.warning(f"Trying to update unknown permission {permission}") + return {} + + existing_permission["allowed"] = [_ldap_path_extract(p, "cn") for p in existing_permission.get("groupPermission", [])] + existing_permission["corresponding_users"] = [_ldap_path_extract(p, "uid") for p in existing_permission.get("inheritPermission", [])] assert isinstance(allowed, list) or isinstance(allowed, str) allowed = [allowed] if not isinstance(allowed, list) else allowed # Guarantee uniqueness of values in allowed, which would otherwise make ldap.update angry. allowed = set(allowed) update_ldap["groupPermission"] = [ - "cn=" + g + ",ou=groups,dc=yunohost,dc=org" for g in allowed + f"cn={g},ou=groups,dc=yunohost,dc=org" for g in allowed ] try: diff --git a/src/tests/test_permission.py b/src/tests/test_permission.py index 5ea6c50677..799543af1f 100644 --- a/src/tests/test_permission.py +++ b/src/tests/test_permission.py @@ -441,7 +441,7 @@ def test_permission_list(): def test_permission_create_main(): with message("permission_created", permission="site.main"): - permission_create("site.main", allowed=["all_users"], protected=False) + _permission_create_with_dummy_app("site.main", allowed=["all_users"], protected=False) res = user_permission_list(full=True)["permissions"] assert "site.main" in res @@ -452,7 +452,7 @@ def test_permission_create_main(): def test_permission_create_extra(): with message("permission_created", permission="site.test"): - permission_create("site.test") + _permission_create_with_dummy_app("site.test", protected=None) res = user_permission_list(full=True)["permissions"] assert "site.test" in res @@ -463,7 +463,8 @@ def test_permission_create_extra(): def test_permission_create_with_specific_user(): - permission_create("site.test", allowed=["alice"]) + with message("permission_created", permission="site.test"): + _permission_create_with_dummy_app("site.test", allowed=["alice"]) res = user_permission_list(full=True)["permissions"] assert "site.test" in res