Skip to content

session_rpcserver+litrpc: allow custom session with all read-only perms #457

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

Merged
merged 3 commits into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions perms/permissions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,10 @@ func TestMatchRegexURI(t *testing.T) {
uris, isRegex = m.MatchRegexURI("/poolrpc.Trader/.*")
require.True(t, isRegex)
require.Empty(t, uris)

// Assert that the read-only permission's keyword is not seen as a valid
// regex.
uris, isRegex = m.MatchRegexURI("***readonly***")
require.False(t, isRegex)
require.Empty(t, uris)
}
22 changes: 22 additions & 0 deletions session_rpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ import (
"gopkg.in/macaroon.v2"
)

// readOnlyAction defines the keyword that a permission action should be set to
// when the entity is set to "uri" in order to activate the special case that
// will result in all read-only permissions known to lit to be added to a
// session's macaroon. The purpose of the three '*'s is to make this keyword
// an invalid URI and an invalid regex so that it does not ever clash with the
// other special cases.
const readOnlyAction = "***readonly***"

// sessionRpcServer is the gRPC server for the Session RPC interface.
type sessionRpcServer struct {
litrpc.UnimplementedSessionsServer
Expand Down Expand Up @@ -152,6 +160,20 @@ func (s *sessionRpcServer) AddSession(_ context.Context,
continue
}

// If the action specified was equal to the
// readOnlyAction keyword, then this is taken to mean
// that the permissions for all read-only URIs should be
// granted.
if op.Action == readOnlyAction {
readPerms := s.cfg.permMgr.ActivePermissions(
true,
)

permissions = append(permissions, readPerms...)
Copy link
Member

Choose a reason for hiding this comment

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

Do we do a de-duplication somewhere? Because maybe the ***readonly*** overlaps with URIs matched by a regex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea re adding de-dupping. I actually though we already did that somewhere but turns out not! So added it in a new commit here :)

I dont actually think the perms from ***readonly*** will ever clash with the regex URIs since those will all be using the special case "uri" entity. But yeah - good to have the dedup stuff for future!

Copy link
Member Author

Choose a reason for hiding this comment

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

(added in the last commit so that the first 2 commits dont need to be re-reviewed. Hope that that is ok)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking forward to when we'd convert all permissions into URIs anyway, then I can imagine there would be some overlap possible. Additional commit looks good 👍


continue
}

// First check if this is a regex URI.
uris, isRegex := s.cfg.permMgr.MatchRegexURI(op.Action)
if isRegex {
Expand Down