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

feat(plugins): require permissions to include content on the app #1191

Merged
merged 4 commits into from May 6, 2019

Conversation

@j-a-m-l
Copy link
Contributor

commented Apr 16, 2019

Proposed changes

Currently permissions are required only to access some elements of the app inside the plugin, but users can't block plugins that introduce other elements on the app or modify them.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
@codecov-io

This comment has been minimized.

Copy link

commented Apr 17, 2019

Codecov Report

Merging #1191 into v2.4.0 will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           v2.4.0   #1191      +/-   ##
=========================================
- Coverage   38.72%   38.7%   -0.03%     
=========================================
  Files         222     222              
  Lines        5882    5886       +4     
  Branches     1164    1164              
=========================================
  Hits         2278    2278              
- Misses       3407    3411       +4     
  Partials      197     197
Impacted Files Coverage Δ
src/renderer/services/plugin-manager.js 2.38% <0%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7da8f8...eab3837. Read the comment docs.

@j-a-m-l j-a-m-l referenced this pull request Apr 17, 2019
4 of 4 tasks complete
await this.loadAvatars(pluginObject, plugin, profileId)
await this.loadWalletTabs(pluginObject, plugin, profileId)
// COMPONENTS, ROUTES, MENU_ITEMS, AVATARS, WALLET_TABS
for (const permission of uniq(plugin.config.permissions)) {

This comment has been minimized.

Copy link
@luciorubeens

luciorubeens Apr 17, 2019

Member

I think it needs a validation before calling the method:

Suggested change
for (const permission of uniq(plugin.config.permissions)) {
for (const permission of ['COMPONENTS, ROUTES, MENU_ITEMS, AVATARS, WALLET_TABS']) {
if (plugin.config.permission[permission]) {
// ...
}
}

This comment has been minimized.

Copy link
@j-a-m-l

j-a-m-l Apr 18, 2019

Author Contributor

In this case, permissions is an Array, so it wouldn't be necessary to do that. The only possible problem that it could cause it's that some permissions should be loaded in order. I'll change it now.

This comment has been minimized.

Copy link
@luciorubeens

luciorubeens Apr 18, 2019

Member

You are considering that all plugins will write the permissions correctly.

This comment has been minimized.

Copy link
@j-a-m-l

j-a-m-l Apr 18, 2019

Author Contributor

It's an array of permissions, if someone uses numbers, null or an object, it wouldn't work and it would throw an Error, so they could realize that it's a mistake.

@j-a-m-l j-a-m-l referenced this pull request Apr 18, 2019
3 of 3 tasks complete
@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

This is much more complicated than it needs to be. 4 packages from lodash is unnecessary.

No packages are needed, you can just reduce the array. I'm willing to settle for a variation of your first commit:

if (!plugin.config.permissions || !plugin.config.permissions.length) {
  return
}

const permissions = plugin.config.permissions.reduce((map, value) => {
  map[value] = true

  return map
}, {})

if (permissions.COMPONENTS) {
  await this.loadComponents(pluginObject, plugin)
}
if (permissions.ROUTES) {
  await this.loadRoutes(pluginObject, plugin)
}
if (permissions.MENU_ITEMS) {
  await this.loadMenuItems(pluginObject, plugin, profileId)
}
if (permissions.AVATARS) {
  await this.loadAvatars(pluginObject, plugin, profileId)
}
if (permissions.WALLET_TABS) {
  await this.loadWalletTabs(pluginObject, plugin, profileId)
}

In fact, the array could still be built in the fetchPlugin method since the same data is already being stored. Meaning it doesn't need doing every time a plugin is enabled.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@alexbarnsley more permissions are going to be added, some of them depending on each other, so a way to avoid those duplicated check + load is loading first the essential ones.

@faustbrian
Copy link
Contributor

left a comment

Changes in #1191 (comment) should be made to not rely on a dozen functions for simple filtering.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

We're going to add potentially more permissions, @alexbarnsley. As an example:

  • THEMES: #1194
  • IFRAME_URL: to allow inserting iframes that loads URLs (not all pages could be used on inframes: #1192)
  • BACKGROUNDS: to add new backgrounds
  • IDENTICONS: to use other kind of identicons
  • WALLET: to read information about a specific wallet
  • VOTES: to read information about the votes of a wallet
  • TRANSACTIONS: to access the transactions of a wallet
  • TRANSFER: to send
  • VOTE: to vote
  • I18N: to use different messaages for the same language
  • DASHBOARD: to add content to the dashboard
  • FOOTER: to add content to the footer
  • IMAGE: to use other images (on the welcome screen, logo or other places)
  • FEED: to use a different source of announcements

Do you really think that the best solution is duplicating manually the block?

if (permissions.X) {
  await this.loadX(pluginObject, plugin, profileId)
}

That approach also would check all the possible permissions on plugins that only require a subset of them or even none.

Changed

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@j-a-m-l I prefer verbose over the hard-to-read code and the use of 4 different lodash packages.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

We are already using lodash, so what's the problem on that?

What parts do you think that are hard to read? I could refactored them.

Apart from that, checking every permission on every plugin is unnecessary. In this case, we only check the permissions that are required by the plugin.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented May 3, 2019

The fact we load lodash as one package is a problem. it's a big package which is unnecessary to load everything since we don't use the majority of it. And by using 4 different ones seems unnecessary in itself for such a small task.

If you're insistent on a loop, just get it to loop through the permissions and run the load if it exists. Similar to how Lucio suggested to do it:

import { camelCase } from 'lodash'

...

for (const permission of plugin.config.permissions) {
  const method = camelCase(`LOAD_PERMISSION_${permission}`)
  if (this.hasOwnProperty(method)) {
    this[method](pluginObject, plugin, profileId)
  }
}

and rename all load methods to loadPermissionX

@alexbarnsley alexbarnsley changed the base branch from v2.4.0 to next May 3, 2019

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Webpack tree-shakes the modules (https://webpack.js.org/guides/tree-shaking/), so it should include only the code that is actually used. Those functions don't change anything since all of them are already being used on other parts. Anyway, we could use camelCase(`LOAD_PERMISSION_${permission}`) to avoid upperFirst.

As I explained on #1191 (comment) that approach would not fail if the user is trying to load permissions that don't exist, so it would make it harder to discover why the plugin don't work (a typo like COMPONENT instead of COMPONENTS would be harder to spot). An alternative is checking the existence and throw an Error in case the method doesn't exist.

It also wouldn't work with permissions that depends on loading others before (basically COMPONENTS and ROUTE).

I think that renaming those methods to loadPermissionX is a good idea to differentiate them from other methods, but it could be misleading because those methods load components, routes, etc. not their permissions. Instead of that, since, only loadSandbox uses the loadX form, I'd rename it to createSandbox, prepareSandbox or something like that.

@j-a-m-l j-a-m-l referenced this pull request May 3, 2019
3 of 3 tasks complete
@alexbarnsley

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Fair enough about the tree shaking, I wasn't aware of that. And accept the point of loading certain permissions prior to others. I still think verbose would be perfectly fine here, instead of making it overly complicated.

Regardless, I've asked @luciorubeens to take a look since we clearly disagree with each other. I am happy with whichever way Lucio goes so we can get this PR merged.

@luciorubeens
Copy link
Member

left a comment

I also think at first glance it is difficult to understand. And it does not seem right to directly call an internal method by simply formatting the method name.

Do not see a good reason for not mapping the permissions and their respective methods to a variable, it will be useful to quickly inspect what is available, how it should be specified when declaring and what method it will call.

const availablePermissions = [
  {
	description: 'Manage and edit your themes',
    name: 'THEMES',
    method: 'loadTheme',
	version: 2.4.0,
  },
  ...
]

But this PR is doing the job for now, we should improve next.

@j-a-m-l j-a-m-l merged commit f64cfbb into next May 6, 2019

1 check passed

ci/circleci: test-node-11 Your tests passed on CircleCI!
Details

@ArkEcosystemBot ArkEcosystemBot deleted the plugin-permissions branch May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.