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

fix: Hiding widgets / links from unauthorized users on welcome screen. #1078

Conversation

TommyJackson85
Copy link
Contributor

@TommyJackson85 TommyJackson85 commented Jun 28, 2020

Related Issue

Closes #1074

Your solution

Attempting to hide welcomeScreenWidgets from unauthorised users, as they should not have access to if they're not authorised to use them.
Using app-form-builder as a first attempt.

My initial attempt was to do it from each welcomeScreenWidget but I only could manage to hide the link button. Here is what it looked like:

Screenshot 2020-06-28 at 13 19 07

So I then applied SecureView to each mapped out Widget plugin from app-plugin-admin-welcome-screen/src/components/Welcome.tsx file, while passing in a scopes key from the app-form-builder welcomeScreenWidget plugin.
From both unauthorized and authorized accounts, here are the results as it appears to work:

Screenshot 2020-06-28 at 13 49 00

Screenshot 2020-06-28 at 13 48 00

@EmilK15 what do you think? Should I keep the Secure View from the Welcome.tsx file instead and just pass in Secure View scopes from welcomeScreenWidget? I am still yet to do this to other widgets. Just doing one as a first attempt.

How Has This Been Tested?

I am testing visually and manually through both authorized and unauthorized accounts

@TommyJackson85
Copy link
Contributor Author

@EmilK15 I updated my initial post to try something different. :)

@Pavel910 Pavel910 marked this pull request as draft June 28, 2020 13:16
Copy link
Collaborator

@Pavel910 Pavel910 left a comment

Choose a reason for hiding this comment

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

@TommyJackson85 this approach with defining scopes in the widget plugin is ok. Let's go that way. The only thing to remove is that SecureView from each plugin itself.

Another approach to the whole problem would be to convert widget on each plugin into a function, which would receive a Widget component, and then you'd only need to render it. That way the control would be in each individual plugin.

Hard to tell which one is better at this point.

@@ -19,15 +22,17 @@ const buttonStyle = css({
const plugin: AdminWelcomeScreenWidgetPlugin = {
type: "admin-welcome-screen-widget",
name: "admin-welcome-screen-widget-form-builder",
scopes: ROLE_FORMS_EDITOR,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is an acceptable approach 👍

<Link to="/forms" className={linkStyle}>
<ButtonSecondary className={buttonStyle}>Create a new Form</ButtonSecondary>
</Link>
<SecureView scopes={ROLE_FORMS_EDITOR}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can safely remove this SecureView.

</div>
</Elevation>
</Widget>
<SecureView scopes={pl.scopes}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good to me. Let's go with this approach.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jun 28, 2020 via email

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jun 28, 2020

@Pavel910 @EmilK15 I removed the unnecessary SecureViews from the widget and added the remaining scopes key values to the other displayed widgets. Now on unauthorised user accounts it looks like this:

Screenshot 2020-06-28 at 21 02 59

For user accounts who have no permissions set, this above message should be removed or edited to something more welcoming and applicable because right now they don't have any actions to perform so it's misleading.
Maybe it should be for a new issue though.

On a side note, do you know where else in the project to test the code? Like automated tests? If they exist I could see if it covers the code I wrote.

@adrians5j
Copy link
Member

@TommyJackson85, for Cypress tests, please check the https://github.com/webiny/webiny-js/blob/master/docs/CONTRIBUTING.md#cypress-tests.

Let us know if you'll have additional questions.

@Pavel910
Copy link
Collaborator

@TommyJackson85 regarding an empty Welcome screen - let's do it this way: when you get all plugins, you have an array of scopes required by each particular widget. If the user doesn't have access to any of the widgets, we can print a message to contact the administrator. I think this will be enough for now.

You can check plugin scopes programmatically, by using import { hasScopes } from "@webiny/app-security"; and render the message accordingly.

I really like how you try to handle all the different edge cases 🚀 🍻 👏

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jun 29, 2020

@TommyJackson85 regarding an empty Welcome screen - let's do it this way: when you get all plugins, you have an array of scopes required by each particular widget. If the user doesn't have access to any of the widgets, we can print a message to contact the administrator. I think this will be enough for now.

You can check plugin scopes programmatically, by using import { hasScopes } from "@webiny/app-security"; and render the message accordingly.

I really like how you try to handle all the different edge cases 🚀 🍻 👏

@Pavel910 I think i managed to use hasScopes correctly. :) Can you double check my recent commit? It displays a different message for users with no authorization. I manually tested it and it seems to work.

@doitadrian thanks! :) Will check it over the week and I will follow up with some questions if I need.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jun 30, 2020

@Pavel910 my apologies, I didn't use hasScopes correctly and received this error:
##[error]@webiny/app-plugin-admin-welcome-screen: src/components/Welcome.tsx(111,46): error TS2345: Argument of type 'AdminWelcomeScreenWidgetPlugin[]' is not assignable to parameter of type 'ResourcesType'.

Let me try again. Any advice would be great.

@Pavel910
Copy link
Collaborator

Pavel910 commented Jul 2, 2020

Hey @TommyJackson85, sorry for the delay. This is what I think will solve the problem:

    // Check if user has access to at least 1 widget
    const canSeeAnything = getPlugins<AdminWelcomeScreenWidgetPlugin>(
        "admin-welcome-screen-widget"
    ).some(pl => hasScopes(pl.scopes, { forceBoolean: true }));

We need to see if a user can see at least 1 of the widgets. Each Widget plugin has scopes in form of an array. Array.some will tell us if at least 1 element passes the test. And hasScopes will process an array of scopes defined in the widget plugin, and tell you whether the user has all of the required scopes. It works in the AND manner, meaning that the user must have ALL requested scopes for hasScopes to return true.

Let me know if this helps!

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 2, 2020

i thaught it would be fixed as all checks have passed but i am getting this error or running the app locally. The Webiny loading icon appears then this error appears:

Screenshot 2020-07-02 at 13 50 31

Screenshot 2020-07-02 at 13 50 16

I didn't make any changes to that security file. Could my AWS IAM user be outdated or maybe its Mongodb related? Maybe check on your end if its working.

To get started - pick one of the actions below:
</p>
<Typography use={"headline6"}>
<h6>{getPlugins<AdminWelcomeScreenWidgetPlugin>("admin-welcome-screen-widget")}</h6>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@TommyJackson85 this line here is causing your React error. You're trying to render an array of objects. Not sure what you wanted to render here, but this is the place to fix.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 2, 2020 via email

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 2, 2020

How do I grant permission for a user to use each widget? ( i.e. from an administrative account I want to grant an unauthorized account permissions for each widget access). This is just so I can do further manual testing on an account to get authorization for one or two widgets.

So far from and administrator account, I can only give a user these permissions:
Screenshot 2020-07-02 at 21 19 29

@Pavel910
Copy link
Collaborator

Pavel910 commented Jul 3, 2020

@TommyJackson85 roles contain scopes. So you need to create a Role (or several of them) with a specific set of scopes. This way we allow administrators to structure their permissions however they like, and we only care the scopes at the code level.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 4, 2020

@Pavel910 I think I managed to grant permissions for page building, but they only seem to appear in the side nav bar :(

Screenshot 2020-07-04 at 13 01 43

As you can see, there is no page builder widget on the main page.
I added these scopes to try and create permissions (from an administrator account):

Screenshot 2020-07-04 at 13 07 00

Is there anything I am missing?

@Pavel910
Copy link
Collaborator

Pavel910 commented Jul 4, 2020

@TommyJackson85 that's because the scope in widget plugins are not really related to what you have assigned to the user. In the Page Builder widget you have scopes: ["pb:settings"], should be pb:page:crud, and so on for all the plugins.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 4, 2020

That is fair enough. So 'pb:settings' has nothing to do with the widget plugins? I will set all scopes to page:crud related scopes instead so.

@TommyJackson85
Copy link
Contributor Author

@Pavel910 would I be better to apply all potential scope types to the the scopes array including both pb:settings and pb:page:crud for example?

@Pavel910
Copy link
Collaborator

Pavel910 commented Jul 4, 2020

@TommyJackson85 I think pb:page:crud makes the most sense, because the widget is linking to Pages List, which is a module for creating pages. pb:settings is not relevant here. Also since hasScopes works in an AND fashion, you need to be careful not to include scopes that may not be present in user's token.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 4, 2020 via email

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 4, 2020

@Pavel910
These are the scopes I found, as you see there is nothing for CMS

Screenshot 2020-07-04 at 19 54 52

Screenshot 2020-07-04 at 19 54 40

Screenshot 2020-07-04 at 19 53 57

Screenshot 2020-07-04 at 19 52 33

Screenshot 2020-07-04 at 19 50 35

Screenshot 2020-07-04 at 19 50 20

I found these via Security Roles -> Security - Users.

@Pavel910
Copy link
Collaborator

Pavel910 commented Jul 4, 2020

@TommyJackson85 CMS scopes are in progress in another PR: #1088.
You can safely ignore that one for now, simply set the scopes to [] to let that widget always be visible, and we'll come back to it later.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 4, 2020

Ok, no problem! If we successfully merge this I can let me know about the scope array from the welcomeScreenWidget file.

@TommyJackson85
Copy link
Contributor Author

TommyJackson85 commented Jul 4, 2020

pb:page:crud and forms:form:crud are now the scopes being used for their respective plugins and they appear to work as I've tested them manually. CMS Widgets remains accessable for all as its scopes are empty.
Are you happy to merge this PR?

Just for further context, from the side nav related pull request, "settings" scopes were being used for page builder, form builder and headless cms tabs under settings in the side nav. It might be worth retesting them. Should the crud scopes be applied to them too?

@Pavel910
Copy link
Collaborator

Pavel910 commented Jul 4, 2020

@TommyJackson85 Great! I think this is good for merging, and no you don’t need to add crud scopes to settings navigation items. Settings scopes are for settings, crud scopes are for crud modules. So it’s all good now. We’ll do further refinement later on, when the need arises.

I’ll merge this for the next release, feel free to change the draft status of PR and please update the PR body to remove the temporary text.

Thanks and good work! :)

@TommyJackson85 TommyJackson85 marked this pull request as ready for review July 4, 2020 20:21
@Pavel910 Pavel910 merged commit d3a0546 into webiny:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: Action link Widgets displayed on main page to unauthorised users.
3 participants