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

subserver_perms: add Lnd's registered subserver perms #413

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

ellemouton
Copy link
Member

@ellemouton ellemouton commented Aug 30, 2022

In this commit, we include all LND's rpc subserver permissions in the permission list used to create the macaroons used in Litd (for sessions etc). The way this is done is a temporary solution while we wait for an LND change that exports the permissions required by each subserver.

Fixes lightninglabs/lnc-web#60

Run alongside lightninglabs/lightning-node-connect#55 to test

jamaljsr
jamaljsr previously approved these changes Aug 30, 2022
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK LGTM 👌

Thanks for the quick fix.

rpc_proxy.go Outdated Show resolved Hide resolved
@ellemouton ellemouton requested a review from guggero August 30, 2022 16:32
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

The code looks like it would work. But to be honest, this adds a lot of new run time timing dependencies which are very hard to follow. See my other comment for a suggestion.

subserver_permissions.go Outdated Show resolved Hide resolved
@ellemouton ellemouton force-pushed the addLndSubserverPerms branch 3 times, most recently from 5f74bbc to 37a657f Compare August 31, 2022 08:52
@ellemouton
Copy link
Member Author

Thank you for the quick review on this @jamaljsr & @guggero 🙏

@guggero: You are completely right - it was definitely getting a bit hard to follow the code. I have taken your suggestion and added a PermissionsManager (1st commit just adds the manager and 2nd commit does the lnd-subserver things).

Did it slightly different to how you suggested so let me know if you think this way is fine.

@ellemouton ellemouton force-pushed the addLndSubserverPerms branch from 37a657f to 9e981e6 Compare August 31, 2022 09:18
@ellemouton ellemouton requested a review from guggero August 31, 2022 09:26
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Almost there! Looks way better with the permission manager 🎉

subserver_permissions.go Outdated Show resolved Hide resolved
terminal.go Outdated Show resolved Hide resolved
subserver_permissions.go Outdated Show resolved Hide resolved
subserver_permissions.go Outdated Show resolved Hide resolved
subserver_permissions.go Outdated Show resolved Hide resolved
terminal.go Outdated Show resolved Hide resolved
In this commit, a new PermissionsManager is added. It handles all the
active permissions that Lit has access to. This moves us away from using
global variables for permission lists. This change might seem overkill
on its own but hugely simplifies the permission management once we add
lnd subserver permissions.
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Sorry, found a few other things while adding an itest (sending you my diff out of band)...

subserver_permissions.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
subserver_permissions.go Show resolved Hide resolved
make/release_flags.mk Outdated Show resolved Hide resolved
subserver_permissions.go Outdated Show resolved Hide resolved
In this commit, we let the PermissionsManager manage LND's subserver
permissions. Once Litd is connected to LND, it can get LND's build tags
and pass them to the PermissionsManager which will then adjust its list
of permissions accordingly.
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ellemouton ellemouton requested a review from jamaljsr September 1, 2022 14:07
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

Tested the latest changes. All of the sub-servers are now working in the LNC example app. Thanks so much for working on this so quickly. LGTM 🔥

@guggero guggero merged commit c13864c into lightninglabs:master Sep 1, 2022
@ellemouton ellemouton deleted the addLndSubserverPerms branch September 2, 2022 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LNC lightning-node-connect sessions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permissions error on some methods
4 participants