Accept comma separated macaroons #146
Conversation
There was a problem hiding this comment.
Thanks for the feature!
I think the approach is good. Most of my comments are around code style. I know this isn't specified explicitly in this project, but we try to adhere to the same formatting rules in all our projects.
| @@ -71,14 +72,22 @@ func FromHeader(header *http.Header) (*macaroon.Macaroon, lntypes.Preimage, erro | |||
| macBase64, preimageHex := matches[2], matches[3] | |||
| macBytes, err := base64.StdEncoding.DecodeString(macBase64) | |||
| if err != nil { | |||
| return nil, lntypes.Preimage{}, fmt.Errorf("base64 "+ | |||
| "decode of macaroon failed: %v", err) | |||
| // macBase64 might be multiple macaroons separated by commas, | |||
There was a problem hiding this comment.
nit: comments should always be full sentences, including punctuation.
I also think this needs some more explanation that we're using macaroon.Slice below, which can actually decode multiple macaroons that are in one continuous blob of data.
| macBase64 = strings.ReplaceAll(macBase64, ",", "") | ||
| macBytes, err = base64.StdEncoding.DecodeString(macBase64) | ||
| if err != nil { | ||
| return nil, lntypes.Preimage{}, |
There was a problem hiding this comment.
nit: please break the lines before 80 characters, while configuring your IDE to show the tabulator character as 8 spaces (most of the code in this block is now too long).
| } | ||
| mac := &macaroon.Macaroon{} | ||
| // macBytes can contain multiple macaroons, |
There was a problem hiding this comment.
nit: add empty line before comment. Also missing full stop.
| return nil, lntypes.Preimage{}, fmt.Errorf("unable to "+ | ||
| "unmarshal macaroon: %v", err) | ||
| return nil, lntypes.Preimage{}, | ||
| fmt.Errorf("unable to unmarshal macaroon: %v", err) |
There was a problem hiding this comment.
Why this formatting change here and below?
| "not found") | ||
| var preimageHex string | ||
| var ok bool | ||
| for _, m := range mac { |
There was a problem hiding this comment.
I think if there are multiple macaroons, then all of them should have the caveat. Not just one of them. Otherwise IMO the whole idea of having multiple macaroons is weird.
| if !ok { | ||
| return nil, lntypes.Preimage{}, errors.New("preimage caveat " + | ||
| "not found") | ||
| var preimageHex string |
There was a problem hiding this comment.
nit: multiple var statements can be combined into one:
var (
...
)
| break | ||
| } | ||
| } | ||
| if preimageHex == "" || !ok { |
There was a problem hiding this comment.
We probably also want to make sure all macaroons reference the same preimage.
| if err != nil { | ||
| log.Debugf("Deny: %v", err) | ||
| return false | ||
| } | ||
|
|
||
| // TODO: Be able to accept multiple macaroons |
There was a problem hiding this comment.
nit: add full stop to end of sentence. Here and in server_interceptor.go.
| } | ||
| mac := &macaroon.Macaroon{} | ||
| mac := make(macaroon.Slice, 0, 1) |
There was a problem hiding this comment.
I know there isn't a unit test now, but it would be really nice to add one so this could be tested.
|
@gofeuer, remember to re-request review from reviewers when ready |
#143
This will allow Aperture to receive comma separated macaroons and not reject the request as an error.
If multiple macaroons are received, the first one is used and the others are ignored (for now)