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

The default permissions are not set when creating new tenant #13457

Closed
MikeAlhayek opened this issue Mar 22, 2023 · 11 comments
Closed

The default permissions are not set when creating new tenant #13457

MikeAlhayek opened this issue Mar 22, 2023 · 11 comments
Labels

Comments

@MikeAlhayek
Copy link
Member

Describe the bug

When creating a tenant from a recipe like the one below, the Administrator is created without any permissions.

{
  "name": "TestRecipe",
  "displayName": "Test Recipe",
 "issetuprecipe": true,
 "steps":     {
      "name": "Roles",
      "Roles": [
        {
          "Name": "Administrator",
          "Description": "Administrator Role",
          "Permissions": []
        },
        {
          "Name": "Authenticated",
          "Description": "Authenticated role",
          "Permissions": [
            "ManageNotifications",
            "View_SitePage"
          ]
        },
        {
          "Name": "Anonymous",
          "Description": "Anonymous role",
          "Permissions": [
            "View_SitePage"
          ]
        }
      ]
    }
}

To Reproduce

Steps to reproduce the behavior:

  1. Create a new tenant with the above recipe
  2. Go to Dashboard >> Security >> Roles
  3. Edit "Administrator" role.
  4. Here I am expected all permissions to be selected. But none is selected.

Expected behavior

A clear and concise description of what you expected to happen.

a workaround is to give the "Administrator" role the SiteOwner permission.

@jtkech you worked on the permissions, maybe something way missed?

@hishamco
Copy link
Member

I'm suggesting to add the administrator with SiteOwner by default

@jtkech
Copy link
Member

jtkech commented Mar 22, 2023

@MikeAlhayek

Yes I already thought about this, see at the end of #13040 (comment), I don't remember all details but the RoleStep (as it was before) doesn't merge but replace permissions, at some point I thought about updating it but didn't want to change too much the previous behavior.

RoleStep: Hmm a little annoying, it specifies permissions (can be empty) that also rely on features activation, maybe the step should merge (not replace) permissions, at least when in a setup recipe, or maybe a new step for role creation only and if it already exists don't replace/clear its permissions.

@MikeAlhayek
Copy link
Member Author

@jtkech
Well this is bad. I thought we documented the change in the release notes but it does not seem like it. https://docs.orchardcore.net/en/latest/docs/releases/1.6.0/

We should mention in the release logs that a user MUST specify the roles they want to use in their setup recipe otherwise no roles will be created. You documented the change in the recipe's read-me file but not the release logs. https://docs.orchardcore.net/en/latest/docs/reference/modules/Recipes/

The docs states

The Roles feature will automatically map all known permissions to the defined roles each time a feature is enabled.

However, we do not actually map all know permissions as per the document. This is something we should fix prior releasing 1.6.

@jtkech
Copy link
Member

jtkech commented Mar 22, 2023

We should mention in the release logs that a user MUST specify the roles they want to use in their setup recipe otherwise no roles will be created.

Yes, as introduced by your PR #12510, then what I did is #13040 to fix #13024 and #13035

However, we do not actually map all know permissions as per the document.

I think we do as it is working for example with the blog recipe where we define Roles with empty permissions, but when the roles are created, in the handler default permissions based on enabled features are assigned.

What I described above from memory is about the RoleStep itself that I didn't change at all (so no release note to do for this). If you run this step on an exiting role (not a setup recipe) currently it overwrites the permissions because it doesn't do a merge, as it was before.

If part of a setup recipe as we are using with empty permissions, no problem, when creating the role the related handler will assign the default permissions depending on the currently enabled features.

Finally, if part of a setup recipe but with some defined permissions as in your example, finally not sure need to be tried, normally the handler does a merge by taking into account the existing permissions, so normally it will not remove the ones defined in the step.

@jtkech
Copy link
Member

jtkech commented Mar 22, 2023

Maybe I didn't understand the issue, in your setup recipe you define the Administrator with no permissions, as we do with the blog recipe and where default permissions are auto assigned.

@MikeAlhayek
Copy link
Member Author

@jtkech I have to try the blog recipe. But with my recipe, I did not supply any permissions. I simply supplied an empty array. When the role was created it did not include any permission. I am expecting the default permissions to be included.

I'll try the blog recipe and see if there is something I am doing wrong on my side.

@jtkech
Copy link
Member

jtkech commented Mar 23, 2023

Okay, and as I remember only the default permissions of the currently enabled features (e.g. by a previous recipe step); and if the role doesn't exist yet, otherwise as said currently the RoleStep will just overwrite them with what you define in the step.

@MikeAlhayek
Copy link
Member Author

@jtkech the blog recipe indeed worked as expected. I am not sure why my custom startup recipe did not assign any permission during the setup. I'll try to reproduce the issue on my side and check the logs for possible errors.

@MikeAlhayek
Copy link
Member Author

I updated the 1.6 release docs

@vitalybrandes
Copy link

@jtkech I have to try the blog recipe. But with my recipe, I did not supply any permissions. I simply supplied an empty array. When the role was created it did not include any permission. I am expecting the default permissions to be included.

I'll try the blog recipe and see if there is something I am doing wrong on my side.

Have the same issue i think,
Once moved to 1.6 from 1.5 ,clean new start of the app,
nO permissions from my custom modules or any default permissions exist!
image

@MikeAlhayek
Copy link
Member Author

@vitalybrandes it looks like your missing roles not permissions.

Per the release notes, you need to define roles in your setup recipe. If you don't explicitly specify permissions for every role, the default permissions will be applies to each of the defined roles

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants