Skip to content

Commit

Permalink
Update primary error functions to take a context, deprecate old funct…
Browse files Browse the repository at this point in the history 
…ions (#1358)

* add new error funcs which take a ctx and deprecate existing funcs

* use context where available when writing error events

* remove circular internal/errors pkg dep

* suppress some chatty errors using errors.WithoutEvent()

* convert auth oidc funcs to take context where needed to write events

Co-authored-by: Jim <[email protected]>
  • Loading branch information
@jimlambrt
s-christoff and jimlambrt authored Jul 30, 2021
1 parent 02ff2db commit 6b78108
Show file tree
Hide file tree
Showing 227 changed files with 3,945 additions and 3,736 deletions.
17 changes: 9 additions & 8 deletions internal/auth/oidc/account.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package oidc

import (
"context"
"net/url"

"github.com/hashicorp/boundary/internal/auth/oidc/store"
Expand Down Expand Up @@ -40,7 +41,7 @@ type Account struct {
// value being unique
//
// See: https://openid.net/specs/openid-connect-core-1_0.html
func NewAccount(authMethodId string, subject string, opt ...Option) (*Account, error) {
func NewAccount(ctx context.Context, authMethodId string, subject string, opt ...Option) (*Account, error) {
const op = "oidc.NewAccount"
opts := getOpts(opt...)
a := &Account{
Expand All @@ -56,29 +57,29 @@ func NewAccount(authMethodId string, subject string, opt ...Option) (*Account, e
if opts.withIssuer != nil {
a.Issuer = opts.withIssuer.String()
}
if err := a.validate(op); err != nil {
if err := a.validate(ctx, op); err != nil {
return nil, err // intentionally not wrapped.
}

return a, nil
}

// validate the Account. On success, it will return nil.
func (a *Account) validate(caller errors.Op) error {
func (a *Account) validate(ctx context.Context, caller errors.Op) error {
if a.AuthMethodId == "" {
return errors.New(errors.InvalidParameter, caller, "missing auth method id")
return errors.New(ctx, errors.InvalidParameter, caller, "missing auth method id")
}
if a.Subject == "" {
return errors.New(errors.InvalidParameter, caller, "missing subject")
return errors.New(ctx, errors.InvalidParameter, caller, "missing subject")
}
if _, err := url.Parse(a.Issuer); a.Issuer != "" && err != nil {
return errors.New(errors.InvalidParameter, caller, "not a valid issuer", errors.WithWrap(err))
return errors.New(ctx, errors.InvalidParameter, caller, "not a valid issuer", errors.WithWrap(err))
}
if a.Email != "" && len(a.Email) > 320 {
return errors.New(errors.InvalidParameter, caller, "email address is too long")
return errors.New(ctx, errors.InvalidParameter, caller, "email address is too long")
}
if a.FullName != "" && len(a.FullName) > 512 {
return errors.New(errors.InvalidParameter, caller, "full name is too long")
return errors.New(ctx, errors.InvalidParameter, caller, "full name is too long")
}
return nil
}
Expand Down
19 changes: 10 additions & 9 deletions internal/auth/oidc/account_claim_map.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package oidc

import (
"context"
"fmt"

"github.com/hashicorp/boundary/internal/auth/oidc/store"
Expand All @@ -21,7 +22,7 @@ const (
ToNameClaim AccountToClaim = "name"
)

func ConvertToAccountToClaim(s string) (AccountToClaim, error) {
func ConvertToAccountToClaim(ctx context.Context, s string) (AccountToClaim, error) {
const op = "oidc.(AccountToClaim).convertToAccountToClaim"
switch s {
case string(ToSubClaim):
Expand All @@ -31,7 +32,7 @@ func ConvertToAccountToClaim(s string) (AccountToClaim, error) {
case string(ToNameClaim):
return ToNameClaim, nil
default:
return "", errors.New(errors.InvalidParameter, op, fmt.Sprintf("%s is not a valid ToAccountClaim value", s))
return "", errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("%s is not a valid ToAccountClaim value", s))
}
}

Expand All @@ -42,7 +43,7 @@ type AccountClaimMap struct {
tableName string
}

func NewAccountClaimMap(authMethodId, fromClaim string, toClaim AccountToClaim) (*AccountClaimMap, error) {
func NewAccountClaimMap(ctx context.Context, authMethodId, fromClaim string, toClaim AccountToClaim) (*AccountClaimMap, error) {
const op = "oidc.NewAccountClaimMap"
cs := &AccountClaimMap{
AccountClaimMap: &store.AccountClaimMap{
Expand All @@ -51,22 +52,22 @@ func NewAccountClaimMap(authMethodId, fromClaim string, toClaim AccountToClaim)
ToClaim: string(toClaim),
},
}
if err := cs.validate(op); err != nil {
if err := cs.validate(ctx, op); err != nil {
return nil, err
}
return cs, nil
}

// validate the AccountClaimMap. On success, it will return nil.
func (cs *AccountClaimMap) validate(caller errors.Op) error {
func (cs *AccountClaimMap) validate(ctx context.Context, caller errors.Op) error {
if cs.OidcMethodId == "" {
return errors.New(errors.InvalidParameter, caller, "missing oidc auth method id")
return errors.New(ctx, errors.InvalidParameter, caller, "missing oidc auth method id")
}
if cs.FromClaim == "" {
return errors.New(errors.InvalidParameter, caller, "missing from claim")
return errors.New(ctx, errors.InvalidParameter, caller, "missing from claim")
}
if _, err := ConvertToAccountToClaim(cs.ToClaim); err != nil {
return errors.Wrap(err, caller)
if _, err := ConvertToAccountToClaim(ctx, cs.ToClaim); err != nil {
return errors.Wrap(ctx, err, caller)
}
return nil
}
Expand Down
10 changes: 5 additions & 5 deletions internal/auth/oidc/account_claim_map_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func TestAccountClaimMap_Create(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert, require := assert.New(t), require.New(t)
got, err := NewAccountClaimMap(tt.args.authMethodId, tt.args.from, tt.args.to)
got, err := NewAccountClaimMap(context.TODO(), tt.args.authMethodId, tt.args.from, tt.args.to)
if tt.wantErr {
require.Error(err)
assert.True(errors.Match(errors.T(tt.wantIsErr), err))
Expand Down Expand Up @@ -150,7 +150,7 @@ func TestAccountClaimMap_Delete(t *testing.T) {
WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]), WithAudClaims("alice.com")) // seed an extra callback url to just make sure the delete only gets the right num of rows

testResource := func(authMethodId string, fromClaim string, toClaim AccountToClaim) *AccountClaimMap {
c, err := NewAccountClaimMap(authMethodId, fromClaim, toClaim)
c, err := NewAccountClaimMap(context.TODO(), authMethodId, fromClaim, toClaim)
require.NoError(t, err)
return c
}
Expand Down Expand Up @@ -228,7 +228,7 @@ func TestAccountClaimMap_Clone(t *testing.T) {
require.NoError(err)
m := TestAuthMethod(t, conn, databaseWrapper, org.PublicId, InactiveState, "alice_rp", "my-dogs-name",
WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]))
orig, err := NewAccountClaimMap(m.PublicId, "oid", ToSubClaim)
orig, err := NewAccountClaimMap(context.TODO(), m.PublicId, "oid", ToSubClaim)
require.NoError(err)
cp := orig.Clone()
assert.True(proto.Equal(cp.AccountClaimMap, orig.AccountClaimMap))
Expand All @@ -240,9 +240,9 @@ func TestAccountClaimMap_Clone(t *testing.T) {
require.NoError(err)
m := TestAuthMethod(t, conn, databaseWrapper, org.PublicId, InactiveState, "alice_rp", "my-dogs-name",
WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]))
orig, err := NewAccountClaimMap(m.PublicId, "oid", ToSubClaim)
orig, err := NewAccountClaimMap(context.TODO(), m.PublicId, "oid", ToSubClaim)
require.NoError(err)
orig2, err := NewAccountClaimMap(m.PublicId, "uid", ToSubClaim)
orig2, err := NewAccountClaimMap(context.TODO(), m.PublicId, "uid", ToSubClaim)
require.NoError(err)

cp := orig.Clone()
Expand Down
31 changes: 17 additions & 14 deletions internal/auth/oidc/account_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (

func TestAccount_Create(t *testing.T) {
t.Parallel()
ctx := context.TODO()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kmsCache := kms.TestKms(t, conn, wrapper)
Expand Down Expand Up @@ -53,7 +54,7 @@ func TestAccount_Create(t *testing.T) {
},
create: true,
want: func() *Account {
want, err := NewAccount(testAuthMethod.PublicId, "alice", WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithEmail("[email protected]"), WithFullName("Alice Eve Smith"), WithName("alice's restuarant"), WithDescription("A good place to eat"))
want, err := NewAccount(ctx, testAuthMethod.PublicId, "alice", WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithEmail("[email protected]"), WithFullName("Alice Eve Smith"), WithName("alice's restuarant"), WithDescription("A good place to eat"))
require.NoError(t, err)
return want
}(),
Expand All @@ -67,7 +68,7 @@ func TestAccount_Create(t *testing.T) {
},
create: true,
want: func() *Account {
want, err := NewAccount(testAuthMethod.PublicId, "alice", WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithEmail("[email protected]"), WithFullName("Alice Eve Smith"), WithName("alice's restuarant"), WithDescription("A good place to eat"))
want, err := NewAccount(ctx, testAuthMethod.PublicId, "alice", WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithEmail("[email protected]"), WithFullName("Alice Eve Smith"), WithName("alice's restuarant"), WithDescription("A good place to eat"))
require.NoError(t, err)
return want
}(),
Expand All @@ -83,7 +84,7 @@ func TestAccount_Create(t *testing.T) {
},
create: true,
want: func() *Account {
want, err := NewAccount(testAuthMethod.PublicId, "newsubject", WithIssuer(TestConvertToUrls(t, "https://somethingelse.com")[0]))
want, err := NewAccount(ctx, testAuthMethod.PublicId, "newsubject", WithIssuer(TestConvertToUrls(t, "https://somethingelse.com")[0]))
require.NoError(t, err)
return want
}(),
Expand Down Expand Up @@ -135,7 +136,7 @@ func TestAccount_Create(t *testing.T) {
},
create: true,
want: func() *Account {
want, err := NewAccount(testAuthMethod.PublicId, "alice", WithIssuer(&url.URL{}))
want, err := NewAccount(ctx, testAuthMethod.PublicId, "alice", WithIssuer(&url.URL{}))
require.NoError(t, err)
return want
}(),
Expand All @@ -150,7 +151,7 @@ func TestAccount_Create(t *testing.T) {
},
create: true,
want: func() *Account {
want, err := NewAccount(testAuthMethod.PublicId, "alice")
want, err := NewAccount(ctx, testAuthMethod.PublicId, "alice")
require.NoError(t, err)
return want
}(),
Expand All @@ -161,7 +162,7 @@ func TestAccount_Create(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert, require := assert.New(t), require.New(t)
got, err := NewAccount(tt.args.authMethodId, tt.args.subject, tt.args.opts...)
got, err := NewAccount(ctx, tt.args.authMethodId, tt.args.subject, tt.args.opts...)
if tt.wantErr {
require.Error(err)
assert.True(errors.Match(errors.T(tt.wantIsErr), err))
Expand All @@ -171,7 +172,7 @@ func TestAccount_Create(t *testing.T) {
assert.Equal(tt.want, got)
if tt.create {
ctx := context.Background()
id, err := newAccountId(testAuthMethod.GetPublicId(), testAuthMethod.GetIssuer(), tt.args.subject)
id, err := newAccountId(ctx, testAuthMethod.GetPublicId(), testAuthMethod.GetIssuer(), tt.args.subject)
require.NoError(err)
got.PublicId = id
err = rw.Create(ctx, got)
Expand All @@ -194,9 +195,9 @@ func TestAccount_Create(t *testing.T) {
t.Run("account issuer stays when auth method discovery url changes", func(t *testing.T) {
am := TestAuthMethod(t, conn, databaseWrapper, org.PublicId, InactiveState, "client", "secret",
WithIssuer(TestConvertToUrls(t, "https://discovery.com")[0]), WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]))
a, err := NewAccount(am.GetPublicId(), "subject", WithIssuer(TestConvertToUrls(t, am.GetIssuer())[0]))
a, err := NewAccount(ctx, am.GetPublicId(), "subject", WithIssuer(TestConvertToUrls(t, am.GetIssuer())[0]))
require.NoError(t, err)
id, err := newAccountId(am.GetPublicId(), am.GetIssuer(), a.GetSubject())
id, err := newAccountId(ctx, am.GetPublicId(), am.GetIssuer(), a.GetSubject())
require.NoError(t, err)
a.PublicId = id
ctx := context.Background()
Expand All @@ -218,6 +219,7 @@ func TestAccount_Create(t *testing.T) {

func TestAccount_Delete(t *testing.T) {
t.Parallel()
ctx := context.TODO()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kmsCache := kms.TestKms(t, conn, wrapper)
Expand All @@ -242,9 +244,9 @@ func TestAccount_Delete(t *testing.T) {
testResource := func(authMethodId string, subject string) *Account {
u, err := url.Parse(testAuthMethod.GetIssuer())
require.NoError(t, err)
a, err := NewAccount(authMethodId, subject, WithIssuer(u))
a, err := NewAccount(ctx, authMethodId, subject, WithIssuer(u))
require.NoError(t, err)
id, err := newAccountId(testAuthMethod.GetPublicId(), testAuthMethod.GetIssuer(), subject)
id, err := newAccountId(ctx, testAuthMethod.GetPublicId(), testAuthMethod.GetIssuer(), subject)
require.NoError(t, err)
a.PublicId = id
return a
Expand Down Expand Up @@ -307,6 +309,7 @@ func TestAccount_Delete(t *testing.T) {

func TestAccount_Clone(t *testing.T) {
t.Parallel()
ctx := context.TODO()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kmsCache := kms.TestKms(t, conn, wrapper)
Expand All @@ -318,7 +321,7 @@ func TestAccount_Clone(t *testing.T) {
require.NoError(err)
m := TestAuthMethod(t, conn, databaseWrapper, org.PublicId, InactiveState, "alice_rp", "my-dogs-name",
WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]))
orig, err := NewAccount(m.PublicId, "alice", WithFullName("Alice Eve Smith"), WithEmail("[email protected]"))
orig, err := NewAccount(ctx, m.PublicId, "alice", WithFullName("Alice Eve Smith"), WithEmail("[email protected]"))
require.NoError(err)
cp := orig.Clone()
assert.True(proto.Equal(cp.Account, orig.Account))
Expand All @@ -330,9 +333,9 @@ func TestAccount_Clone(t *testing.T) {
require.NoError(err)
m := TestAuthMethod(t, conn, databaseWrapper, org.PublicId, InactiveState, "alice_rp", "my-dogs-name",
WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]))
orig, err := NewAccount(m.PublicId, "alice", WithFullName("Alice Eve Smith"), WithEmail("[email protected]"))
orig, err := NewAccount(ctx, m.PublicId, "alice", WithFullName("Alice Eve Smith"), WithEmail("[email protected]"))
require.NoError(err)
orig2, err := NewAccount(m.PublicId, "bob", WithFullName("Bob Eve Smith"), WithEmail("[email protected]"))
orig2, err := NewAccount(ctx, m.PublicId, "bob", WithFullName("Bob Eve Smith"), WithEmail("[email protected]"))
require.NoError(err)

cp := orig.Clone()
Expand Down
12 changes: 7 additions & 5 deletions internal/auth/oidc/aud_claim.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package oidc

import (
"context"

"github.com/hashicorp/boundary/internal/auth/oidc/store"
"github.com/hashicorp/boundary/internal/errors"
"google.golang.org/protobuf/proto"
Expand Down Expand Up @@ -29,7 +31,7 @@ type AudClaim struct {
//
// For more info on oidc aud claims, see the oidc spec:
// https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
func NewAudClaim(authMethodId string, audClaim string) (*AudClaim, error) {
func NewAudClaim(ctx context.Context, authMethodId string, audClaim string) (*AudClaim, error) {
const op = "oidc.NewAudClaim"

c := &AudClaim{
Expand All @@ -38,19 +40,19 @@ func NewAudClaim(authMethodId string, audClaim string) (*AudClaim, error) {
Aud: audClaim,
},
}
if err := c.validate(op); err != nil {
if err := c.validate(ctx, op); err != nil {
return nil, err // intentionally not wrapped
}
return c, nil
}

// validate the AudClaim. On success, it will return nil.
func (a *AudClaim) validate(caller errors.Op) error {
func (a *AudClaim) validate(ctx context.Context, caller errors.Op) error {
if a.OidcMethodId == "" {
return errors.New(errors.InvalidParameter, caller, "missing oidc auth method id")
return errors.New(ctx, errors.InvalidParameter, caller, "missing oidc auth method id")
}
if a.Aud == "" {
return errors.New(errors.InvalidParameter, caller, "missing aud claim")
return errors.New(ctx, errors.InvalidParameter, caller, "missing aud claim")
}
return nil
}
Expand Down
13 changes: 8 additions & 5 deletions internal/auth/oidc/aud_claim_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

func TestAudClaim_Create(t *testing.T) {
t.Parallel()
ctx := context.TODO()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kmsCache := kms.TestKms(t, conn, wrapper)
Expand Down Expand Up @@ -93,7 +94,7 @@ func TestAudClaim_Create(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert, require := assert.New(t), require.New(t)
got, err := NewAudClaim(tt.args.authMethodId, tt.args.aud)
got, err := NewAudClaim(ctx, tt.args.authMethodId, tt.args.aud)
if tt.wantErr {
require.Error(err)
assert.True(errors.Match(errors.T(tt.wantIsErr), err))
Expand Down Expand Up @@ -121,6 +122,7 @@ func TestAudClaim_Create(t *testing.T) {

func TestAudClaim_Delete(t *testing.T) {
t.Parallel()
ctx := context.TODO()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kmsCache := kms.TestKms(t, conn, wrapper)
Expand All @@ -135,7 +137,7 @@ func TestAudClaim_Delete(t *testing.T) {
WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]), WithAudClaims("alice.com")) // seed an extra callback url to just make sure the delete only gets the right num of rows

testResource := func(authMethodId string, AudClaim string) *AudClaim {
c, err := NewAudClaim(authMethodId, AudClaim)
c, err := NewAudClaim(ctx, authMethodId, AudClaim)
require.NoError(t, err)
return c
}
Expand Down Expand Up @@ -198,6 +200,7 @@ func TestAudClaim_Delete(t *testing.T) {

func TestAudClaim_Clone(t *testing.T) {
t.Parallel()
ctx := context.TODO()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kmsCache := kms.TestKms(t, conn, wrapper)
Expand All @@ -209,7 +212,7 @@ func TestAudClaim_Clone(t *testing.T) {
require.NoError(err)
m := TestAuthMethod(t, conn, databaseWrapper, org.PublicId, InactiveState, "alice_rp", "my-dogs-name",
WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]))
orig, err := NewAudClaim(m.PublicId, "eve.com")
orig, err := NewAudClaim(ctx, m.PublicId, "eve.com")
require.NoError(err)
cp := orig.Clone()
assert.True(proto.Equal(cp.AudClaim, orig.AudClaim))
Expand All @@ -221,9 +224,9 @@ func TestAudClaim_Clone(t *testing.T) {
require.NoError(err)
m := TestAuthMethod(t, conn, databaseWrapper, org.PublicId, InactiveState, "alice_rp", "my-dogs-name",
WithIssuer(TestConvertToUrls(t, "https://alice.com")[0]), WithApiUrl(TestConvertToUrls(t, "https://api.com")[0]))
orig, err := NewAudClaim(m.PublicId, "eve.com")
orig, err := NewAudClaim(ctx, m.PublicId, "eve.com")
require.NoError(err)
orig2, err := NewAudClaim(m.PublicId, "alice.com")
orig2, err := NewAudClaim(ctx, m.PublicId, "alice.com")
require.NoError(err)

cp := orig.Clone()
Expand Down
Loading

0 comments on commit 6b78108

Please sign in to comment.