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.

Sign up for GitHub

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

Jump to bottom

itest: add tests for macaroon authentication #1152

Merged
merged 2 commits into from
Nov 21, 2019
Merged

itest: add tests for macaroon authentication #1152

merged 2 commits into from
Nov 21, 2019

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Apr 29, 2018 •
edited
Loading

This PR adds a new integration test for the macaroon authentication of the gRPC interface, as mentioned in #284.

The following six scenarios are tested:

  1. Expect an error when using no macaroons.
  2. Expect an error when using an invalid macaroon.
  3. Expect an error when trying to invoke a write with a read-only macaroon.
  4. Expect an error when the first-party caveat TimeoutConstraint is not satisfied.
  5. Expect an error when the first-party caveat IPLockConstraint is not satisfied.
  6. Expect a successful response when the correct macaroon with valid first-party caveats is used.

Closes #284.

@meshcollider meshcollider added authentication macaroons testing Improvements/modifications to the test suite labels May 1, 2018
@Roasbeef Roasbeef requested a review from aakselrod May 1, 2018 19:54
@guggero guggero force-pushed the macaroon-integrationtest branch 6 times, most recently from dffd28d to c7e9fc1 Compare May 8, 2018 10:06
@guggero guggero force-pushed the macaroon-integrationtest branch 2 times, most recently from 15027b8 to 6899cf8 Compare May 12, 2018 10:51
@guggero guggero force-pushed the macaroon-integrationtest branch 2 times, most recently from 19bae76 to 4b3408a Compare May 23, 2018 10:58
@guggero guggero force-pushed the macaroon-integrationtest branch 3 times, most recently from 65efe8c to 264ccf2 Compare May 26, 2018 13:24
@guggero guggero mentioned this pull request May 26, 2018
Merged
@guggero guggero force-pushed the macaroon-integrationtest branch 3 times, most recently from 5a54b72 to 355d9d9 Compare June 5, 2018 08:24
@guggero guggero force-pushed the macaroon-integrationtest branch 3 times, most recently from a00ca06 to 0804aa0 Compare June 14, 2018 06:21
@guggero guggero force-pushed the macaroon-integrationtest branch 2 times, most recently from 8adc2f4 to ec89753 Compare July 1, 2018 10:30
@guggero guggero force-pushed the macaroon-integrationtest branch from ec89753 to 8c32631 Compare July 10, 2018 06:57
@Roasbeef Roasbeef added P3 might get fixed, nice to have needs review PR needs review by regular contributors labels Jul 10, 2018
@guggero guggero force-pushed the macaroon-integrationtest branch from 8c32631 to ccb2bb1 Compare July 15, 2018 07:22
@guggero guggero force-pushed the macaroon-integrationtest branch from 167e65b to 709eb41 Compare April 17, 2019 10:33
@guggero guggero force-pushed the macaroon-integrationtest branch from 709eb41 to c65a9b1 Compare May 26, 2019 12:53
@guggero guggero force-pushed the macaroon-integrationtest branch from c65a9b1 to 2693d2b Compare June 4, 2019 06:48
@guggero guggero force-pushed the macaroon-integrationtest branch from 2693d2b to cdeb04a Compare September 30, 2019 08:06
@guggero guggero force-pushed the macaroon-integrationtest branch from cdeb04a to 92d73fb Compare November 12, 2019 16:14
@guggero guggero changed the title Integration tests: add tests for macaroon authentication itest: add tests for macaroon authentication Nov 12, 2019
@guggero guggero force-pushed the macaroon-integrationtest branch 2 times, most recently from 139eb95 to abebbcf Compare November 13, 2019 14:13
@guggero
Copy link
Collaborator Author

guggero commented Nov 13, 2019

I tried some different setups and found that for this particular case, there is no overhead of adding a separate test case, because there are no new nodes spun up or channels opened.

@guggero guggero requested review from halseth and wpaulino and removed request for aakselrod November 13, 2019 14:18
@guggero guggero added this to the 0.9.0 milestone Nov 13, 2019
@guggero guggero force-pushed the macaroon-integrationtest branch from abebbcf to e90ee6d Compare November 13, 2019 14:29
@guggero guggero added the itests Issues related to integration tests. label Nov 13, 2019
wpaulino
wpaulino approved these changes Nov 13, 2019
View reviewed changes
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Solid set of tests!

lntest/itest/rpc.go Outdated Show resolved Hide resolved
lntest/itest/rpc.go Outdated
t.Fatalf("unable to connect to alice: %v", err)
}
defer conn.Close()
ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we usually ignore cancel here. Any reason not to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the linter complained... All other tests were probably added before the new linter?

wpaulino
wpaulino reviewed Nov 13, 2019
View reviewed changes
lntest/itest/rpc.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the macaroon-integrationtest branch from e90ee6d to 543b258 Compare November 14, 2019 15:38
halseth
halseth approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Good test, LGTM 👍

lntest/itest/macaroons.go
t.Fatalf("unable to connect to alice: %v", err)
}
defer conn.Close()
ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: newline above for some breathing room

@halseth halseth merged commit b1e6d9c into lightningnetwork:master Nov 21, 2019
@guggero guggero deleted the macaroon-integrationtest branch November 21, 2019 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wpaulino wpaulino wpaulino approved these changes

@halseth halseth halseth approved these changes

Assignees
No one assigned
Labels
authentication itests Issues related to integration tests. macaroons needs review PR needs review by regular contributors P3 might get fixed, nice to have testing Improvements/modifications to the test suite
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.

Add macaroon-specific tests
6 participants
@guggero @halseth @wpaulino @Roasbeef @meshcollider @joostjager