Skip to content

Fix panic in display only case for license - forward port #1408 to master #1415

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

Merged
thaJeztah merged 2 commits into from
Nov 14, 2018
Merged

Fix panic in display only case for license - forward port #1408 to master #1415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
53f053e
Fix panic in display only case for license
Oct 2, 2018
864aef7
Add test coverage for display only with hub licenses
Oct 2, 2018
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
25 changes: 15 additions & 10 deletions cli/command/engine/activate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,21 @@ import (
)

type activateOptions struct {
licenseFile string
version string
registryPrefix string
format string
image string
quiet bool
displayOnly bool
sockPath string
licenseFile string
version string
registryPrefix string
format string
image string
quiet bool
displayOnly bool
sockPath string
licenseLoginFunc func(ctx context.Context, authConfig *types.AuthConfig) (licenseutils.HubUser, error)
}

// newActivateCommand creates a new `docker engine activate` command
func newActivateCommand(dockerCli command.Cli) *cobra.Command {
var options activateOptions
options.licenseLoginFunc = licenseutils.Login

cmd := &cobra.Command{
Use: "activate [OPTIONS]",
Expand Down Expand Up @@ -60,7 +62,7 @@ https://hub.docker.com/ then specify the file with the '--license' flag.
flags.StringVar(&options.registryPrefix, "registry-prefix", clitypes.RegistryPrefix, "Override the default location where engine images are pulled")
flags.StringVar(&options.image, "engine-image", "", "Specify engine image")
flags.StringVar(&options.format, "format", "", "Pretty-print licenses using a Go template")
flags.BoolVar(&options.displayOnly, "display-only", false, "only display the available licenses and exit")
flags.BoolVar(&options.displayOnly, "display-only", false, "only display license information and exit")
flags.BoolVar(&options.quiet, "quiet", false, "Only display available licenses by ID")
flags.StringVar(&options.sockPath, "containerd", "", "override default location of containerd endpoint")

Expand Down Expand Up @@ -90,6 +92,9 @@ func runActivate(cli command.Cli, options activateOptions) error {
if license, err = getLicenses(ctx, authConfig, cli, options); err != nil {
return err
}
if options.displayOnly {
Copy link
Contributor

@silvin-lubecki silvin-lubecki Oct 4, 2018

Choose a reason for hiding this comment

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

I guess the fix is here, but isn't it safer and simpler to just check if license is nil?

if license, err = getLicenses(ctx, authConfig, cli, options); err != nil || license == nil {
    return err
}

Copy link
Member

@thaJeztah thaJeztah Oct 17, 2018

Choose a reason for hiding this comment

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

@dhiltgen ^

Copy link
Member

@thaJeztah thaJeztah Nov 13, 2018

Choose a reason for hiding this comment

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

@silvin-lubecki discussing with @dhiltgen out of band; let's merge this one as-is, and do a follow up with improvements

Copy link
Contributor

@silvin-lubecki silvin-lubecki Nov 14, 2018

Choose a reason for hiding this comment

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

I'm fine with it, let's merge the PR then.

return nil
}
} else {
if license, err = licenseutils.LoadLocalIssuedLicense(ctx, options.licenseFile); err != nil {
return err
Expand Down Expand Up @@ -136,7 +141,7 @@ Restart docker with 'systemctl restart docker' to complete the activation.`)
}

func getLicenses(ctx context.Context, authConfig *types.AuthConfig, cli command.Cli, options activateOptions) (*model.IssuedLicense, error) {
user, err := licenseutils.Login(ctx, authConfig)
user, err := options.licenseLoginFunc(ctx, authConfig)
if err != nil {
return nil, err
}
Expand Down
75 changes: 75 additions & 0 deletions cli/command/engine/activate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
package engine

import (
"context"
"fmt"
"testing"
"time"

"github.com/docker/cli/internal/licenseutils"
"github.com/docker/cli/internal/test"
clitypes "github.com/docker/cli/types"
"github.com/docker/docker/api/types"
"github.com/docker/docker/client"
"github.com/docker/licensing"
"github.com/docker/licensing/model"
"gotest.tools/assert"
"gotest.tools/fs"
"gotest.tools/golden"
Expand Down Expand Up @@ -69,3 +74,73 @@ func TestActivateExpiredLicenseDryRun(t *testing.T) {
assert.NilError(t, err)
golden.Assert(t, c.OutBuffer().String(), "expired-license-display-only.golden")
}

type mockLicenseClient struct{}

func (c mockLicenseClient) LoginViaAuth(ctx context.Context, username, password string) (authToken string, err error) {
return "", fmt.Errorf("not implemented")
}

func (c mockLicenseClient) GetHubUserOrgs(ctx context.Context, authToken string) (orgs []model.Org, err error) {
return nil, fmt.Errorf("not implemented")
}
func (c mockLicenseClient) GetHubUserByName(ctx context.Context, username string) (user *model.User, err error) {
return nil, fmt.Errorf("not implemented")
}
func (c mockLicenseClient) VerifyLicense(ctx context.Context, license model.IssuedLicense) (res *model.CheckResponse, err error) {
return nil, fmt.Errorf("not implemented")
}
func (c mockLicenseClient) GenerateNewTrialSubscription(ctx context.Context, authToken, dockerID, email string) (subscriptionID string, err error) {
return "", fmt.Errorf("not implemented")
}
func (c mockLicenseClient) ListSubscriptions(ctx context.Context, authToken, dockerID string) (response []*model.Subscription, err error) {
expires := time.Date(2010, time.January, 1, 0, 0, 0, 0, time.UTC)
return []*model.Subscription{
{
State: "active",
Expires: &expires,
},
}, nil
}
func (c mockLicenseClient) ListSubscriptionsDetails(ctx context.Context, authToken, dockerID string) (response []*model.SubscriptionDetail, err error) {
return nil, fmt.Errorf("not implemented")
}
func (c mockLicenseClient) DownloadLicenseFromHub(ctx context.Context, authToken, subscriptionID string) (license *model.IssuedLicense, err error) {
return nil, fmt.Errorf("not implemented")
}
func (c mockLicenseClient) ParseLicense(license []byte) (parsedLicense *model.IssuedLicense, err error) {
return nil, fmt.Errorf("not implemented")
}
func (c mockLicenseClient) StoreLicense(ctx context.Context, dclnt licensing.WrappedDockerClient, licenses *model.IssuedLicense, localRootDir string) error {
return fmt.Errorf("not implemented")
}
func (c mockLicenseClient) LoadLocalLicense(ctx context.Context, dclnt licensing.WrappedDockerClient) (*model.Subscription, error) {
return nil, fmt.Errorf("not implemented")
}
func (c mockLicenseClient) SummarizeLicense(res *model.CheckResponse, keyID string) *model.Subscription {
return nil
}
func TestActivateDisplayOnlyHub(t *testing.T) {
isRoot = func() bool { return true }
c := test.NewFakeCli(&verClient{client.Client{}, types.Version{}, nil, types.Info{}, nil})
c.SetContainerizedEngineClient(
func(string) (clitypes.ContainerizedClient, error) {
return &fakeContainerizedEngineClient{}, nil
},
)

hubUser := licenseutils.HubUser{
Client: mockLicenseClient{},
}
options := activateOptions{
licenseLoginFunc: func(ctx context.Context, authConfig *types.AuthConfig) (licenseutils.HubUser, error) {
return hubUser, nil
},
displayOnly: true,
}
c.OutBuffer().Reset()
err := runActivate(c, options)

assert.NilError(t, err)
golden.Assert(t, c.OutBuffer().String(), "expired-hub-license-display-only.golden")
}
3 changes: 3 additions & 0 deletions cli/command/engine/testdata/expired-hub-license-display-only.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Looking for existing licenses for ...
NUM OWNER PRODUCT ID EXPIRES PRICING COMPONENTS
0 2010-01-01 00:00:00 +0000 UTC
14 changes: 7 additions & 7 deletions internal/licenseutils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
// HubUser wraps a licensing client and holds key information
// for a user to avoid multiple lookups
type HubUser struct {
client licensing.Client
Client licensing.Client
token string
User model.User
Orgs []model.Org
Expand Down Expand Up @@ -73,7 +73,7 @@ func Login(ctx context.Context, authConfig *types.AuthConfig) (HubUser, error) {
return HubUser{}, err
}
return HubUser{
client: lclient,
Client: lclient,
token: token,
User: *user,
Orgs: orgs,
Expand All @@ -83,12 +83,12 @@ func Login(ctx context.Context, authConfig *types.AuthConfig) (HubUser, error) {

// GetAvailableLicenses finds all available licenses for a given account and their orgs
func (u HubUser) GetAvailableLicenses(ctx context.Context) ([]LicenseDisplay, error) {
subs, err := u.client.ListSubscriptions(ctx, u.token, u.User.ID)
subs, err := u.Client.ListSubscriptions(ctx, u.token, u.User.ID)
if err != nil {
return nil, err
}
for _, org := range u.Orgs {
orgSub, err := u.client.ListSubscriptions(ctx, u.token, org.ID)
orgSub, err := u.Client.ListSubscriptions(ctx, u.token, org.ID)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -134,16 +134,16 @@ func (u HubUser) GetAvailableLicenses(ctx context.Context) ([]LicenseDisplay, er

// GenerateTrialLicense will generate a new trial license for the specified user or org
func (u HubUser) GenerateTrialLicense(ctx context.Context, targetID string) (*model.IssuedLicense, error) {
subID, err := u.client.GenerateNewTrialSubscription(ctx, u.token, targetID, u.User.Email)
subID, err := u.Client.GenerateNewTrialSubscription(ctx, u.token, targetID, u.User.Email)
if err != nil {
return nil, err
}
return u.client.DownloadLicenseFromHub(ctx, u.token, subID)
return u.Client.DownloadLicenseFromHub(ctx, u.token, subID)
}

// GetIssuedLicense will download a license by ID
func (u HubUser) GetIssuedLicense(ctx context.Context, ID string) (*model.IssuedLicense, error) {
return u.client.DownloadLicenseFromHub(ctx, u.token, ID)
return u.Client.DownloadLicenseFromHub(ctx, u.token, ID)
}

// LoadLocalIssuedLicense will load a local license file
Expand Down
12 changes: 6 additions & 6 deletions internal/licenseutils/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestGetOrgByID(t *testing.T) {
func TestGetAvailableLicensesListFail(t *testing.T) {
ctx := context.Background()
user := HubUser{
client: &fakeLicensingClient{
Client: &fakeLicensingClient{
listSubscriptionsFunc: func(ctx context.Context, authToken, dockerID string) (response []*model.Subscription, err error) {
return nil, fmt.Errorf("list subscriptions error")
},
Expand All @@ -59,7 +59,7 @@ func TestGetAvailableLicensesOrgFail(t *testing.T) {
Orgs: []model.Org{
{ID: "orgid"},
},
client: &fakeLicensingClient{
Client: &fakeLicensingClient{
listSubscriptionsFunc: func(ctx context.Context, authToken, dockerID string) (response []*model.Subscription, err error) {
if dockerID == "orgid" {
return nil, fmt.Errorf("list subscriptions org error")
Expand All @@ -86,7 +86,7 @@ func TestGetAvailableLicensesHappy(t *testing.T) {
Orgname: "orgname",
},
},
client: &fakeLicensingClient{
Client: &fakeLicensingClient{
listSubscriptionsFunc: func(ctx context.Context, authToken, dockerID string) (response []*model.Subscription, err error) {
if dockerID == "orgid" {
return []*model.Subscription{
Expand Down Expand Up @@ -146,7 +146,7 @@ func TestGetAvailableLicensesHappy(t *testing.T) {
func TestGenerateTrialFail(t *testing.T) {
ctx := context.Background()
user := HubUser{
client: &fakeLicensingClient{
Client: &fakeLicensingClient{
generateNewTrialSubscriptionFunc: func(ctx context.Context, authToken, dockerID, email string) (subscriptionID string, err error) {
return "", fmt.Errorf("generate trial failure")
},
Expand All @@ -160,7 +160,7 @@ func TestGenerateTrialFail(t *testing.T) {
func TestGenerateTrialHappy(t *testing.T) {
ctx := context.Background()
user := HubUser{
client: &fakeLicensingClient{
Client: &fakeLicensingClient{
generateNewTrialSubscriptionFunc: func(ctx context.Context, authToken, dockerID, email string) (subscriptionID string, err error) {
return "subid", nil
},
Expand All @@ -174,7 +174,7 @@ func TestGenerateTrialHappy(t *testing.T) {
func TestGetIssuedLicense(t *testing.T) {
ctx := context.Background()
user := HubUser{
client: &fakeLicensingClient{},
Client: &fakeLicensingClient{},
}
id := "idgoeshere"
_, err := user.GetIssuedLicense(ctx, id)
Expand Down