Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-2345] node permissioning controller #1075

Merged
merged 31 commits into from
Mar 14, 2019
Merged

[PAN-2345] node permissioning controller #1075

merged 31 commits into from
Mar 14, 2019

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Mar 8, 2019

PR description

Refactor NodeWhitelistController to NodeLocalConfigPermissioningController
Refactored NodePermissioningController to accept multiple providers and check all of them
Added to LocalConfigNodePermissioningController the notion of its own enode URL. The node will not add its own enode to the whitelist.

Fixed Issue(s)

fixes #PAN-2345

@lucassaldanha
Copy link
Contributor

For the NodePermissioningControllerTest test, we need to ensure we have the different possible scenarios covered:

  1. When using only local config permissioning
    • Should NOT check the SyncStatusPermissioningProvider
    • Should only delegate to the NodeLocalConfigPermissioningController (only one provider will be enabled)
  2. When using SmartContract based permissioning (either standalone or in conjuction with local config)
    • If SyncStatusPermissioningProvider.isPermitted() returns false, communication is not permitted
    • If SyncStatusPermissioningProvider.isPermitted() returns true:
      • communication is permitted if every other provider returns true
      • communication is forbidden if any provider returns false

The current name of the test methods imply the the SyncStatusPermissioningProvider is always used. This shouldn't happen.

Copy link
Contributor

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

LGMT. Just some optional assertion suggestions.

@macfarla macfarla merged commit 0a7d2b4 into PegaSysEng:master Mar 14, 2019
@macfarla macfarla deleted the pan-2345-node-permissioning-controller branch March 14, 2019 04:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants