From 054c0aa53823335b0730aa4f289e8b4598ce9cc3 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 8 Jun 2021 11:31:58 -0500 Subject: [PATCH] Update password generated account IDs (#1290) --- CHANGELOG.md | 8 ++ internal/auth/password/public_ids.go | 4 +- internal/auth/password/public_ids_test.go | 3 +- internal/auth/password/repository_account.go | 3 +- .../auth/password/repository_account_test.go | 15 ++-- internal/auth/subtype.go | 3 +- internal/auth/subtype_test.go | 9 ++- internal/cmd/commands/accountscmd/funcs.go | 6 +- .../commands/accountscmd/password_funcs.go | 2 +- internal/cmd/commands/dev/dev.go | 5 +- internal/intglobals/prefixes.go | 6 ++ internal/perms/acl_test.go | 33 ++++++-- internal/perms/grants_test.go | 34 ++++++-- .../handlers/accounts/account_service.go | 11 +-- .../handlers/accounts/account_service_test.go | 78 ++++++++++++++----- .../handlers/accounts/validate_test.go | 31 +++++++- .../controller/handlers/users/user_service.go | 20 ++++- internal/servers/controller/testing.go | 5 +- internal/tests/api/accounts/account_test.go | 10 ++- .../common-workflows/manage-users-groups.mdx | 6 +- 20 files changed, 225 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8800093e..f43894de2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ## Next +### Deprecations/Changes + +* `password` account IDs: When the `oidc` auth method came out, accounts were + given the prefix `acctoidc`. Unfortunately, accounts in the `password` method + were using `apw`...oops. We're standardizing on `acct` and have updated the + `password` method to generate new IDs with `acctpw` prefixes. + Previously-generated prefixes will continue to work. + ### New and Improved * oidc: The new Managed Groups feature allows groups of accounts to be created diff --git a/internal/auth/password/public_ids.go b/internal/auth/password/public_ids.go index d91d265d2..2f919fa8d 100644 --- a/internal/auth/password/public_ids.go +++ b/internal/auth/password/public_ids.go @@ -3,12 +3,12 @@ package password import ( "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/errors" + "github.com/hashicorp/boundary/internal/intglobals" ) // PublicId prefixes for the resources in the password package. const ( AuthMethodPrefix = "ampw" - AccountPrefix = "apw" ) func newAuthMethodId() (string, error) { @@ -22,7 +22,7 @@ func newAuthMethodId() (string, error) { func newAccountId() (string, error) { const op = "password.newAccountId" - id, err := db.NewPublicId(AccountPrefix) + id, err := db.NewPublicId(intglobals.NewPasswordAccountPrefix) if err != nil { return "", errors.Wrap(err, op) } diff --git a/internal/auth/password/public_ids_test.go b/internal/auth/password/public_ids_test.go index a7d8e8872..0bd6b503e 100644 --- a/internal/auth/password/public_ids_test.go +++ b/internal/auth/password/public_ids_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -17,6 +18,6 @@ func Test_PublicIds(t *testing.T) { t.Run("account", func(t *testing.T) { id, err := newAccountId() require.NoError(t, err) - assert.True(t, strings.HasPrefix(id, AccountPrefix+"_")) + assert.True(t, strings.HasPrefix(id, intglobals.NewPasswordAccountPrefix+"_")) }) } diff --git a/internal/auth/password/repository_account.go b/internal/auth/password/repository_account.go index 627c23930..d2a71bcb7 100644 --- a/internal/auth/password/repository_account.go +++ b/internal/auth/password/repository_account.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/boundary/internal/db" dbcommon "github.com/hashicorp/boundary/internal/db/common" "github.com/hashicorp/boundary/internal/errors" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/oplog" ) @@ -60,7 +61,7 @@ func (r *Repository) CreateAccount(ctx context.Context, scopeId string, a *Accou a = a.clone() if opts.withPublicId != "" { - if !strings.HasPrefix(opts.withPublicId, AccountPrefix+"_") { + if !strings.HasPrefix(opts.withPublicId, intglobals.NewPasswordAccountPrefix+"_") { return nil, errors.New(errors.InvalidParameter, op, "chosen account id does not have a valid prefix") } a.PublicId = opts.withPublicId diff --git a/internal/auth/password/repository_account_test.go b/internal/auth/password/repository_account_test.go index 27eed3183..0040cd720 100644 --- a/internal/auth/password/repository_account_test.go +++ b/internal/auth/password/repository_account_test.go @@ -10,6 +10,7 @@ import ( dbassert "github.com/hashicorp/boundary/internal/db/assert" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/oplog" "github.com/stretchr/testify/assert" @@ -242,7 +243,7 @@ func TestRepository_CreateAccount(t *testing.T) { require.NoError(err) assert.Empty(tt.in.PublicId) require.NotNil(got) - assertPublicId(t, AccountPrefix, got.PublicId) + assertPublicId(t, intglobals.NewPasswordAccountPrefix, got.PublicId) assert.NotSame(tt.in, got) assert.Equal(tt.want.Name, got.Name) assert.Equal(tt.want.Description, got.Description) @@ -289,7 +290,7 @@ func TestRepository_CreateAccount_DuplicateNames(t *testing.T) { got, err := repo.CreateAccount(context.Background(), org.GetPublicId(), in) require.NoError(err) require.NotNil(got) - assertPublicId(t, AccountPrefix, got.PublicId) + assertPublicId(t, intglobals.NewPasswordAccountPrefix, got.PublicId) assert.NotSame(in, got) assert.Equal(in.Name, got.Name) assert.Equal(in.Description, got.Description) @@ -321,7 +322,7 @@ func TestRepository_CreateAccount_DuplicateNames(t *testing.T) { got, err := repo.CreateAccount(context.Background(), org.GetPublicId(), in) require.NoError(err) require.NotNil(got) - assertPublicId(t, AccountPrefix, got.PublicId) + assertPublicId(t, intglobals.NewPasswordAccountPrefix, got.PublicId) assert.NotSame(in, got) assert.Equal(in.Name, got.Name) assert.Equal(in.Description, got.Description) @@ -331,7 +332,7 @@ func TestRepository_CreateAccount_DuplicateNames(t *testing.T) { got2, err := repo.CreateAccount(context.Background(), org.GetPublicId(), in2) assert.NoError(err) require.NotNil(got2) - assertPublicId(t, AccountPrefix, got2.PublicId) + assertPublicId(t, intglobals.NewPasswordAccountPrefix, got2.PublicId) assert.NotSame(in2, got2) assert.Equal(in2.Name, got2.Name) assert.Equal(in2.Description, got2.Description) @@ -944,7 +945,7 @@ func TestRepository_UpdateAccount(t *testing.T) { return } require.NotNil(got) - assertPublicId(t, AccountPrefix, got.PublicId) + assertPublicId(t, intglobals.NewPasswordAccountPrefix, got.PublicId) assert.Equal(tt.wantCount, gotCount, "row count") assert.NotSame(tt.orig, got) assert.Equal(tt.orig.AuthMethodId, got.AuthMethodId) @@ -1029,7 +1030,7 @@ func TestRepository_UpdateAccount_DupeNames(t *testing.T) { got, err := repo.CreateAccount(context.Background(), org.GetPublicId(), in) assert.NoError(err) require.NotNil(got) - assertPublicId(t, AccountPrefix, got.PublicId) + assertPublicId(t, intglobals.NewPasswordAccountPrefix, got.PublicId) assert.NotSame(in, got) assert.Equal(in.Name, got.Name) assert.Equal(in.Description, got.Description) @@ -1106,7 +1107,7 @@ func TestRepository_UpdateAccount_DupeNames(t *testing.T) { got, err := repo.CreateAccount(context.Background(), org.GetPublicId(), in) assert.NoError(err) require.NotNil(got) - assertPublicId(t, AccountPrefix, got.PublicId) + assertPublicId(t, intglobals.NewPasswordAccountPrefix, got.PublicId) assert.NotSame(in, got) assert.Equal(in.LoginName, got.LoginName) diff --git a/internal/auth/subtype.go b/internal/auth/subtype.go index 49244051c..a0c054b24 100644 --- a/internal/auth/subtype.go +++ b/internal/auth/subtype.go @@ -89,7 +89,8 @@ func SubtypeFromType(t string) Subtype { func SubtypeFromId(id string) Subtype { switch { case strings.HasPrefix(strings.TrimSpace(id), password.AuthMethodPrefix), - strings.HasPrefix(strings.TrimSpace(id), password.AccountPrefix): + strings.HasPrefix(strings.TrimSpace(id), intglobals.OldPasswordAccountPrefix), + strings.HasPrefix(strings.TrimSpace(id), intglobals.NewPasswordAccountPrefix): return PasswordSubtype case strings.HasPrefix(strings.TrimSpace(id), oidc.AuthMethodPrefix), strings.HasPrefix(strings.TrimSpace(id), oidc.AccountPrefix), diff --git a/internal/auth/subtype_test.go b/internal/auth/subtype_test.go index 8ab6efdd2..63d58c3cb 100644 --- a/internal/auth/subtype_test.go +++ b/internal/auth/subtype_test.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/boundary/internal/auth/oidc" "github.com/hashicorp/boundary/internal/auth/password" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -30,7 +31,13 @@ func TestSubtype(t *testing.T) { }, { subtype: PasswordSubtype, - id: password.AccountPrefix + "1234567890", + id: intglobals.OldPasswordAccountPrefix + "1234567890", + wantString: "password", + wantSubtype: PasswordSubtype, + }, + { + subtype: PasswordSubtype, + id: intglobals.NewPasswordAccountPrefix + "1234567890", wantString: "password", wantSubtype: PasswordSubtype, }, diff --git a/internal/cmd/commands/accountscmd/funcs.go b/internal/cmd/commands/accountscmd/funcs.go index cb18ecbac..ea53057d2 100644 --- a/internal/cmd/commands/accountscmd/funcs.go +++ b/internal/cmd/commands/accountscmd/funcs.go @@ -58,7 +58,7 @@ func (c *Command) extraHelpFunc(helpMap map[string]func() string) string { "", " Read a account:", "", - ` $ boundary accounts read -id apw_1234567890`, + ` $ boundary accounts read -id acctpw_1234567890`, "", " Please see the accounts subcommand help for detailed usage information.", }) @@ -70,7 +70,7 @@ func (c *Command) extraHelpFunc(helpMap map[string]func() string) string { "", " Change the password on a password-type account:", "", - ` $ boundary accounts change-password -id apw_1234567890 -current-password -new-password `, + ` $ boundary accounts change-password -id acctpw_1234567890 -current-password -new-password `, "", "", }) @@ -82,7 +82,7 @@ func (c *Command) extraHelpFunc(helpMap map[string]func() string) string { "", " Set the password on a password-type account:", "", - ` $ boundary accounts set-password -id apw_1234567890 -password `, + ` $ boundary accounts set-password -id acctpw_1234567890 -password `, "", "", }) diff --git a/internal/cmd/commands/accountscmd/password_funcs.go b/internal/cmd/commands/accountscmd/password_funcs.go index c4984925c..521c66f95 100644 --- a/internal/cmd/commands/accountscmd/password_funcs.go +++ b/internal/cmd/commands/accountscmd/password_funcs.go @@ -49,7 +49,7 @@ func (c *PasswordCommand) extraPasswordHelpFunc(helpMap map[string]func() string "", " Update a password-type account given its ID. Example:", "", - ` $ boundary accounts update password -id apw_1234567890 -name "devops" -description "Password account for DevOps"`, + ` $ boundary accounts update password -id acctpw_1234567890 -name "devops" -description "Password account for DevOps"`, "", "", }) diff --git a/internal/cmd/commands/dev/dev.go b/internal/cmd/commands/dev/dev.go index 704b60399..d253e4f5a 100644 --- a/internal/cmd/commands/dev/dev.go +++ b/internal/cmd/commands/dev/dev.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/boundary/internal/cmd/config" "github.com/hashicorp/boundary/internal/host/static" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/servers/controller" "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/servers/worker" @@ -313,10 +314,10 @@ func (c *Command) Run(args []string) int { c.DevPasswordAuthMethodId = fmt.Sprintf("%s_%s", password.AuthMethodPrefix, c.flagIdSuffix) c.DevOidcAuthMethodId = fmt.Sprintf("%s_%s", oidc.AuthMethodPrefix, c.flagIdSuffix) c.DevUserId = fmt.Sprintf("%s_%s", iam.UserPrefix, c.flagIdSuffix) - c.DevPasswordAccountId = fmt.Sprintf("%s_%s", password.AccountPrefix, c.flagIdSuffix) + c.DevPasswordAccountId = fmt.Sprintf("%s_%s", intglobals.NewPasswordAccountPrefix, c.flagIdSuffix) c.DevOidcAccountId = fmt.Sprintf("%s_%s", oidc.AccountPrefix, c.flagIdSuffix) c.DevUnprivilegedUserId = "u_" + strutil.Reverse(strings.TrimPrefix(c.DevUserId, "u_")) - c.DevUnprivilegedPasswordAccountId = fmt.Sprintf("%s_", password.AccountPrefix) + strutil.Reverse(strings.TrimPrefix(c.DevPasswordAccountId, fmt.Sprintf("%s_", password.AccountPrefix))) + c.DevUnprivilegedPasswordAccountId = fmt.Sprintf("%s_", intglobals.NewPasswordAccountPrefix) + strutil.Reverse(strings.TrimPrefix(c.DevPasswordAccountId, fmt.Sprintf("%s_", intglobals.NewPasswordAccountPrefix))) c.DevUnprivilegedOidcAccountId = fmt.Sprintf("%s_", oidc.AccountPrefix) + strutil.Reverse(strings.TrimPrefix(c.DevOidcAccountId, fmt.Sprintf("%s_", oidc.AccountPrefix))) c.DevOrgId = fmt.Sprintf("%s_%s", scope.Org.Prefix(), c.flagIdSuffix) c.DevProjectId = fmt.Sprintf("%s_%s", scope.Project.Prefix(), c.flagIdSuffix) diff --git a/internal/intglobals/prefixes.go b/internal/intglobals/prefixes.go index 8c1e951b4..c14309b5c 100644 --- a/internal/intglobals/prefixes.go +++ b/internal/intglobals/prefixes.go @@ -9,4 +9,10 @@ const ( // OidcManagedGroupPrefix defines the prefix for ManagedGroup public ids // from the OIDC auth method. OidcManagedGroupPrefix = "mgoidc" + + // OldPasswordAccountPrefix is the previously-used account prefix + OldPasswordAccountPrefix = "apw" + + // NewPasswordAccountPrefix is the new account prefix + NewPasswordAccountPrefix = "acctpw" ) diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index a09e45590..20699e412 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -1,8 +1,10 @@ package perms import ( + "fmt" "testing" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/stretchr/testify/assert" @@ -227,7 +229,7 @@ func Test_ACLAllowed(t *testing.T) { userId: "u_abcd1234", }, { - name: "bad templated account id", + name: "bad templated old account id", resource: Resource{ScopeId: "o_c"}, scopeGrants: commonGrants, actionsAuthorized: []actionAuthorized{ @@ -235,17 +237,38 @@ func Test_ACLAllowed(t *testing.T) { {action: action.Authenticate}, {action: action.Delete}, }, - accountId: "apw_1234567890", + accountId: fmt.Sprintf("%s_1234567890", intglobals.OldPasswordAccountPrefix), }, { - name: "good templated account id", - resource: Resource{ScopeId: "o_c", Id: "apw_1234567890"}, + name: "good templated old account id", + resource: Resource{ScopeId: "o_c", Id: fmt.Sprintf("%s_1234567890", intglobals.OldPasswordAccountPrefix)}, scopeGrants: commonGrants, actionsAuthorized: []actionAuthorized{ {action: action.ChangePassword, authorized: true}, {action: action.Update}, }, - accountId: "apw_1234567890", + accountId: fmt.Sprintf("%s_1234567890", intglobals.OldPasswordAccountPrefix), + }, + { + name: "bad templated new account id", + resource: Resource{ScopeId: "o_c"}, + scopeGrants: commonGrants, + actionsAuthorized: []actionAuthorized{ + {action: action.List}, + {action: action.Authenticate}, + {action: action.Delete}, + }, + accountId: fmt.Sprintf("%s_1234567890", intglobals.NewPasswordAccountPrefix), + }, + { + name: "good templated new account id", + resource: Resource{ScopeId: "o_c", Id: fmt.Sprintf("%s_1234567890", intglobals.NewPasswordAccountPrefix)}, + scopeGrants: commonGrants, + actionsAuthorized: []actionAuthorized{ + {action: action.ChangePassword, authorized: true}, + {action: action.Update}, + }, + accountId: fmt.Sprintf("%s_1234567890", intglobals.NewPasswordAccountPrefix), }, { name: "all type", diff --git a/internal/perms/grants_test.go b/internal/perms/grants_test.go index 91573188a..25d6ba3d8 100644 --- a/internal/perms/grants_test.go +++ b/internal/perms/grants_test.go @@ -1,8 +1,10 @@ package perms import ( + "fmt" "testing" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/types/action" "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" @@ -652,21 +654,43 @@ func Test_Parse(t *testing.T) { }, }, { - name: "bad account id template", + name: "bad old account id template", input: `id={{superman}};actions=read`, - accountId: "apw_1234567890", + accountId: fmt.Sprintf("%s_1234567890", intglobals.OldPasswordAccountPrefix), err: `perms.Parse: unknown template "{{superman}}" in grant "id" value: parameter violation: error #100`, }, { - name: "good account id template", + name: "bad new account id template", + input: `id={{superman}};actions=read`, + accountId: fmt.Sprintf("%s_1234567890", intglobals.NewPasswordAccountPrefix), + err: `perms.Parse: unknown template "{{superman}}" in grant "id" value: parameter violation: error #100`, + }, + { + name: "good old account id template", input: `id={{ account.id}};actions=update,read`, - accountId: "apw_1234567890", + accountId: fmt.Sprintf("%s_1234567890", intglobals.OldPasswordAccountPrefix), expected: Grant{ scope: Scope{ Id: "o_scope", Type: scope.Org, }, - id: "apw_1234567890", + id: fmt.Sprintf("%s_1234567890", intglobals.OldPasswordAccountPrefix), + actions: map[action.Type]bool{ + action.Update: true, + action.Read: true, + }, + }, + }, + { + name: "good new account id template", + input: `id={{ account.id}};actions=update,read`, + accountId: fmt.Sprintf("%s_1234567890", intglobals.NewPasswordAccountPrefix), + expected: Grant{ + scope: Scope{ + Id: "o_scope", + Type: scope.Org, + }, + id: fmt.Sprintf("%s_1234567890", intglobals.NewPasswordAccountPrefix), actions: map[action.Type]bool{ action.Update: true, action.Read: true, diff --git a/internal/servers/controller/handlers/accounts/account_service.go b/internal/servers/controller/handlers/accounts/account_service.go index ad7acd2ef..e1fae10bd 100644 --- a/internal/servers/controller/handlers/accounts/account_service.go +++ b/internal/servers/controller/handlers/accounts/account_service.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/boundary/internal/errors" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/accounts" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/requests" "github.com/hashicorp/boundary/internal/servers/controller/common" @@ -926,7 +927,7 @@ func validateGetRequest(req *pbs.GetAccountRequest) error { if req == nil { return errors.New(errors.InvalidParameter, op, "nil request") } - return handlers.ValidateGetRequest(handlers.NoopValidatorFn, req, password.AccountPrefix, oidc.AccountPrefix) + return handlers.ValidateGetRequest(handlers.NoopValidatorFn, req, intglobals.OldPasswordAccountPrefix, intglobals.NewPasswordAccountPrefix, oidc.AccountPrefix) } func validateCreateRequest(req *pbs.CreateAccountRequest) error { @@ -1022,7 +1023,7 @@ func validateUpdateRequest(req *pbs.UpdateAccountRequest) error { } } return badFields - }, password.AccountPrefix, oidc.AccountPrefix) + }, intglobals.OldPasswordAccountPrefix, intglobals.NewPasswordAccountPrefix, oidc.AccountPrefix) } func validateDeleteRequest(req *pbs.DeleteAccountRequest) error { @@ -1030,7 +1031,7 @@ func validateDeleteRequest(req *pbs.DeleteAccountRequest) error { if req == nil { return errors.New(errors.InvalidParameter, op, "nil request") } - return handlers.ValidateDeleteRequest(handlers.NoopValidatorFn, req, password.AccountPrefix, oidc.AccountPrefix) + return handlers.ValidateDeleteRequest(handlers.NoopValidatorFn, req, intglobals.OldPasswordAccountPrefix, intglobals.NewPasswordAccountPrefix, oidc.AccountPrefix) } func validateListRequest(req *pbs.ListAccountsRequest) error { @@ -1057,7 +1058,7 @@ func validateChangePasswordRequest(req *pbs.ChangePasswordRequest) error { return errors.New(errors.InvalidParameter, op, "nil request") } badFields := map[string]string{} - if !handlers.ValidId(handlers.Id(req.GetId()), password.AccountPrefix) { + if !handlers.ValidId(handlers.Id(req.GetId()), intglobals.OldPasswordAccountPrefix, intglobals.NewPasswordAccountPrefix) { badFields[idField] = "Improperly formatted identifier." } if req.GetVersion() == 0 { @@ -1081,7 +1082,7 @@ func validateSetPasswordRequest(req *pbs.SetPasswordRequest) error { return errors.New(errors.InvalidParameter, op, "nil request") } badFields := map[string]string{} - if !handlers.ValidId(handlers.Id(req.GetId()), password.AccountPrefix) { + if !handlers.ValidId(handlers.Id(req.GetId()), intglobals.OldPasswordAccountPrefix, intglobals.NewPasswordAccountPrefix) { badFields[idField] = "Improperly formatted identifier." } if req.GetVersion() == 0 { diff --git a/internal/servers/controller/handlers/accounts/account_service_test.go b/internal/servers/controller/handlers/accounts/account_service_test.go index 3d1f6204d..0556e3108 100644 --- a/internal/servers/controller/handlers/accounts/account_service_test.go +++ b/internal/servers/controller/handlers/accounts/account_service_test.go @@ -19,6 +19,7 @@ import ( scopepb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/scopes" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/servers/controller/common" "github.com/hashicorp/boundary/internal/servers/controller/handlers" @@ -184,8 +185,14 @@ func TestGet(t *testing.T) { res: &pbs.GetAccountResponse{Item: &oidcWireAccount}, }, { - name: "Get a non existing password account", - req: &pbs.GetAccountRequest{Id: password.AccountPrefix + "_DoesntExis"}, + name: "Get a non existing old password account", + req: &pbs.GetAccountRequest{Id: intglobals.OldPasswordAccountPrefix + "_DoesntExis"}, + res: nil, + err: handlers.ApiErrorWithCode(codes.NotFound), + }, + { + name: "Get a non existing new password account", + req: &pbs.GetAccountRequest{Id: intglobals.NewPasswordAccountPrefix + "_DoesntExis"}, res: nil, err: handlers.ApiErrorWithCode(codes.NotFound), }, @@ -575,9 +582,16 @@ func TestDelete(t *testing.T) { }, }, { - name: "Delete bad pw account id", + name: "Delete bad old pw account id", req: &pbs.DeleteAccountRequest{ - Id: password.AccountPrefix + "_doesntexis", + Id: intglobals.OldPasswordAccountPrefix + "_doesntexis", + }, + err: handlers.ApiErrorWithCode(codes.NotFound), + }, + { + name: "Delete bad new pw account id", + req: &pbs.DeleteAccountRequest{ + Id: intglobals.NewPasswordAccountPrefix + "_doesntexis", }, err: handlers.ApiErrorWithCode(codes.NotFound), }, @@ -693,7 +707,7 @@ func TestCreatePassword(t *testing.T) { }, }, res: &pbs.CreateAccountResponse{ - Uri: fmt.Sprintf("accounts/%s_", password.AccountPrefix), + Uri: fmt.Sprintf("accounts/%s_", intglobals.NewPasswordAccountPrefix), Item: &pb.Account{ AuthMethodId: defaultAccount.GetAuthMethodId(), Name: &wrapperspb.StringValue{Value: "name"}, @@ -715,7 +729,7 @@ func TestCreatePassword(t *testing.T) { }, }, res: &pbs.CreateAccountResponse{ - Uri: fmt.Sprintf("accounts/%s_", password.AccountPrefix), + Uri: fmt.Sprintf("accounts/%s_", intglobals.NewPasswordAccountPrefix), Item: &pb.Account{ AuthMethodId: defaultAccount.GetAuthMethodId(), Scope: &scopepb.ScopeInfo{Id: o.GetPublicId(), Type: scope.Org.String(), ParentScopeId: scope.Global.String()}, @@ -737,7 +751,7 @@ func TestCreatePassword(t *testing.T) { }, }, res: &pbs.CreateAccountResponse{ - Uri: fmt.Sprintf("accounts/%s_", password.AccountPrefix), + Uri: fmt.Sprintf("accounts/%s_", intglobals.NewPasswordAccountPrefix), Item: &pb.Account{ AuthMethodId: defaultAccount.GetAuthMethodId(), Name: &wrapperspb.StringValue{Value: "name_with_password"}, @@ -767,7 +781,7 @@ func TestCreatePassword(t *testing.T) { req: &pbs.CreateAccountRequest{ Item: &pb.Account{ AuthMethodId: defaultAccount.GetAuthMethodId(), - Id: password.AccountPrefix + "_notallowed", + Id: intglobals.NewPasswordAccountPrefix + "_notallowed", Type: "password", Attributes: createAttr("cantprovideid", ""), }, @@ -823,7 +837,7 @@ func TestCreatePassword(t *testing.T) { } if got != nil { assert.Contains(got.GetUri(), tc.res.Uri) - assert.True(strings.HasPrefix(got.GetItem().GetId(), password.AccountPrefix+"_")) + assert.True(strings.HasPrefix(got.GetItem().GetId(), intglobals.NewPasswordAccountPrefix+"_")) gotCreateTime := got.GetItem().GetCreatedTime() require.NoError(err, "Error converting proto to timestamp.") gotUpdateTime := got.GetItem().GetUpdatedTime() @@ -1304,9 +1318,23 @@ func TestUpdatePassword(t *testing.T) { }, }, { - name: "Update a Non Existing Account", + name: "Update a Non Existing Old ID Account", req: &pbs.UpdateAccountRequest{ - Id: password.AccountPrefix + "_DoesntExis", + Id: intglobals.OldPasswordAccountPrefix + "_DoesntExis", + UpdateMask: &field_mask.FieldMask{ + Paths: []string{globals.DescriptionField}, + }, + Item: &pb.Account{ + Name: &wrapperspb.StringValue{Value: "new"}, + Description: &wrapperspb.StringValue{Value: "desc"}, + }, + }, + err: handlers.ApiErrorWithCode(codes.NotFound), + }, + { + name: "Update a Non Existing New ID Account", + req: &pbs.UpdateAccountRequest{ + Id: intglobals.NewPasswordAccountPrefix + "_DoesntExis", UpdateMask: &field_mask.FieldMask{ Paths: []string{globals.DescriptionField}, }, @@ -1324,7 +1352,7 @@ func TestUpdatePassword(t *testing.T) { Paths: []string{"id"}, }, Item: &pb.Account{ - Id: password.AccountPrefix + "_somethinge", + Id: intglobals.NewPasswordAccountPrefix + "_somethinge", Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }, @@ -1663,7 +1691,7 @@ func TestUpdateOidc(t *testing.T) { { name: "Update a Non Existing Account", req: &pbs.UpdateAccountRequest{ - Id: password.AccountPrefix + "_DoesntExis", + Id: intglobals.NewPasswordAccountPrefix + "_DoesntExis", UpdateMask: &field_mask.FieldMask{ Paths: []string{globals.DescriptionField}, }, @@ -1681,7 +1709,7 @@ func TestUpdateOidc(t *testing.T) { Paths: []string{"id"}, }, Item: &pb.Account{ - Id: password.AccountPrefix + "_somethinge", + Id: intglobals.NewPasswordAccountPrefix + "_somethinge", Name: &wrapperspb.StringValue{Value: "new"}, Description: &wrapperspb.StringValue{Value: "new desc"}, }, @@ -1908,8 +1936,14 @@ func TestSetPassword(t *testing.T) { password: "somepassword", }, { - name: "notfound account id", - accountId: password.AccountPrefix + "_DoesntExis", + name: "notfound old account id", + accountId: intglobals.OldPasswordAccountPrefix + "_DoesntExis", + version: defaultAcct.GetVersion(), + password: "anewpassword", + }, + { + name: "notfound new account id", + accountId: intglobals.NewPasswordAccountPrefix + "_DoesntExis", version: defaultAcct.GetVersion(), password: "anewpassword", }, @@ -2069,9 +2103,17 @@ func TestChangePassword(t *testing.T) { newPW: "somepassword", }, { - name: "notfound account id", + name: "notfound old account id", authMethodId: defaultAcct.GetAuthMethodId(), - accountId: password.AccountPrefix + "_DoesntExis", + accountId: intglobals.OldPasswordAccountPrefix + "_DoesntExis", + version: defaultAcct.GetVersion(), + oldPW: "somepassword", + newPW: "anewpassword", + }, + { + name: "notfound new account id", + authMethodId: defaultAcct.GetAuthMethodId(), + accountId: intglobals.NewPasswordAccountPrefix + "_DoesntExis", version: defaultAcct.GetVersion(), oldPW: "somepassword", newPW: "anewpassword", diff --git a/internal/servers/controller/handlers/accounts/validate_test.go b/internal/servers/controller/handlers/accounts/validate_test.go index 2e175f821..b771d58fd 100644 --- a/internal/servers/controller/handlers/accounts/validate_test.go +++ b/internal/servers/controller/handlers/accounts/validate_test.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/boundary/internal/auth/password" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/accounts" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/fieldmaskpb" @@ -154,9 +155,19 @@ func TestValidateUpdateRequest(t *testing.T) { errContains string }{ { - name: "password to oidc change type", + name: "password to oidc change type, old prefix", req: &pbs.UpdateAccountRequest{ - Id: password.AccountPrefix + "_1234567890", + Id: intglobals.OldPasswordAccountPrefix + "_1234567890", + Item: &pb.Account{ + Type: auth.OidcSubtype.String(), + }, + }, + errContains: fieldError(typeField, "Cannot modify the resource type."), + }, + { + name: "password to oidc change type, new prefix", + req: &pbs.UpdateAccountRequest{ + Id: intglobals.NewPasswordAccountPrefix + "_1234567890", Item: &pb.Account{ Type: auth.OidcSubtype.String(), }, @@ -174,9 +185,21 @@ func TestValidateUpdateRequest(t *testing.T) { errContains: fieldError(typeField, "Cannot modify the resource type."), }, { - name: "password bad attributes", + name: "password bad attributes old prefix", req: &pbs.UpdateAccountRequest{ - Id: password.AccountPrefix + "_1234567890", + Id: intglobals.OldPasswordAccountPrefix + "_1234567890", + Item: &pb.Account{ + Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{ + "test": structpb.NewStringValue("something"), + }}, + }, + }, + errContains: fieldError(attributesField, "Attribute fields do not match the expected format."), + }, + { + name: "password bad attributes new prefix", + req: &pbs.UpdateAccountRequest{ + Id: intglobals.NewPasswordAccountPrefix + "_1234567890", Item: &pb.Account{ Attributes: &structpb.Struct{Fields: map[string]*structpb.Value{ "test": structpb.NewStringValue("something"), diff --git a/internal/servers/controller/handlers/users/user_service.go b/internal/servers/controller/handlers/users/user_service.go index 5e29132d5..e5aaba272 100644 --- a/internal/servers/controller/handlers/users/user_service.go +++ b/internal/servers/controller/handlers/users/user_service.go @@ -7,12 +7,12 @@ import ( "github.com/hashicorp/boundary/globals" "github.com/hashicorp/boundary/internal/auth" "github.com/hashicorp/boundary/internal/auth/oidc" - "github.com/hashicorp/boundary/internal/auth/password" "github.com/hashicorp/boundary/internal/errors" pb "github.com/hashicorp/boundary/internal/gen/controller/api/resources/users" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/iam/store" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/requests" "github.com/hashicorp/boundary/internal/servers/controller/common" @@ -714,7 +714,11 @@ func validateAddUserAccountsRequest(req *pbs.AddUserAccountsRequest) error { } for _, a := range req.GetAccountIds() { // TODO: Increase the type of auth accounts that can be added to a user. - if !handlers.ValidId(handlers.Id(a), password.AccountPrefix, oidc.AccountPrefix) { + if !handlers.ValidId(handlers.Id(a), + intglobals.OldPasswordAccountPrefix, + intglobals.NewPasswordAccountPrefix, + oidc.AccountPrefix, + ) { badFields["account_ids"] = "Values must be valid account ids." break } @@ -735,7 +739,11 @@ func validateSetUserAccountsRequest(req *pbs.SetUserAccountsRequest) error { } for _, a := range req.GetAccountIds() { // TODO: Increase the type of auth accounts that can be added to a user. - if !handlers.ValidId(handlers.Id(a), password.AccountPrefix, oidc.AccountPrefix) { + if !handlers.ValidId(handlers.Id(a), + intglobals.OldPasswordAccountPrefix, + intglobals.NewPasswordAccountPrefix, + oidc.AccountPrefix, + ) { badFields["account_ids"] = "Values must be valid account ids." break } @@ -759,7 +767,11 @@ func validateRemoveUserAccountsRequest(req *pbs.RemoveUserAccountsRequest) error } for _, a := range req.GetAccountIds() { // TODO: Increase the type of auth accounts that can be added to a user. - if !handlers.ValidId(handlers.Id(a), password.AccountPrefix, oidc.AccountPrefix) { + if !handlers.ValidId(handlers.Id(a), + intglobals.OldPasswordAccountPrefix, + intglobals.NewPasswordAccountPrefix, + oidc.AccountPrefix, + ) { badFields["account_ids"] = "Values must be valid account ids." break } diff --git a/internal/servers/controller/testing.go b/internal/servers/controller/testing.go index 9c63a4a23..9c3ae8529 100644 --- a/internal/servers/controller/testing.go +++ b/internal/servers/controller/testing.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/boundary/internal/cmd/config" "github.com/hashicorp/boundary/internal/db/schema" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/servers" "github.com/hashicorp/boundary/sdk/strutil" @@ -33,9 +34,9 @@ const ( DefaultTestUnprivilegedLoginName = "user" DefaultTestPassword = "passpass" DefaultTestUserId = "u_1234567890" - DefaultTestPasswordAccountId = "apw_1234567890" + DefaultTestPasswordAccountId = intglobals.NewPasswordAccountPrefix + "_1234567890" DefaultTestOidcAccountId = "acctoidc_1234567890" - DefaultTestUnprivilegedPasswordAccountId = "apw_0987654321" + DefaultTestUnprivilegedPasswordAccountId = intglobals.NewPasswordAccountPrefix + "_0987654321" DefaultTestUnprivilegedOidcAccountId = "acctoidc_0987654321" ) diff --git a/internal/tests/api/accounts/account_test.go b/internal/tests/api/accounts/account_test.go index a5cc28e7e..ee84c2704 100644 --- a/internal/tests/api/accounts/account_test.go +++ b/internal/tests/api/accounts/account_test.go @@ -9,8 +9,8 @@ import ( "github.com/hashicorp/boundary/api/accounts" "github.com/hashicorp/boundary/api/authmethods" "github.com/hashicorp/boundary/internal/auth/oidc" - "github.com/hashicorp/boundary/internal/auth/password" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/intglobals" "github.com/hashicorp/boundary/internal/servers/controller" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -298,7 +298,13 @@ func TestErrors(t *testing.T) { apiErr = api.AsServerError(err) require.NotNil(apiErr) - _, err = accountClient.Read(tc.Context(), password.AccountPrefix+"_doesntexis") + _, err = accountClient.Read(tc.Context(), intglobals.OldPasswordAccountPrefix+"_doesntexis") + require.Error(err) + apiErr = api.AsServerError(err) + require.NotNil(apiErr) + assert.EqualValues(http.StatusNotFound, apiErr.Response().StatusCode()) + + _, err = accountClient.Read(tc.Context(), intglobals.NewPasswordAccountPrefix+"_doesntexis") require.Error(err) apiErr = api.AsServerError(err) require.NotNil(apiErr) diff --git a/website/content/docs/common-workflows/manage-users-groups.mdx b/website/content/docs/common-workflows/manage-users-groups.mdx index 4785e2d3c..9b7d5a920 100644 --- a/website/content/docs/common-workflows/manage-users-groups.mdx +++ b/website/content/docs/common-workflows/manage-users-groups.mdx @@ -117,7 +117,7 @@ Account information: Auth Method ID: ampw_PbE6nNT72a Created Time: Tue, 29 Sep 2020 08:35:02 PDT Description: My password account - ID: apw_BOn3EcqQfe + ID: acctpw_BOn3EcqQfe Name: my_account Type: password Updated Time: Tue, 29 Sep 2020 08:35:02 PDT @@ -195,7 +195,7 @@ User information: Then associate the user with the account previously created: ```bash -$ boundary users set-accounts -id u_yXhZpt5PX3 -account apw_BOn3EcqQfe +$ boundary users set-accounts -id u_yXhZpt5PX3 -account acctpw_BOn3EcqQfe User information: Created Time: Tue, 29 Sep 2020 08:43:43 PDT @@ -212,7 +212,7 @@ User information: Type: org Accounts: - ID: apw_BOn3EcqQfe + ID: acctpw_BOn3EcqQfe Scope ID: o_1234567890 ```