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

fix: client update breaks primary key #2150

Merged
merged 8 commits into from
Oct 29, 2020
Merged

fix: client update breaks primary key #2150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4052bd5
fix: client update breaks primary key
zepatrik Oct 28, 2020
04a4a08
Merge branch 'master' into fix-client-update-pk
zepatrik Oct 28, 2020
271060c
chore: format docs
zepatrik Oct 28, 2020
bf7ce96
fix: migration test
zepatrik Oct 29, 2020
c435c0f
Merge branch 'master' into fix-client-update-pk
zepatrik Oct 29, 2020
b791c9d
fix: e2e test
zepatrik Oct 29, 2020
3efb4a7
chore: remove debug statement
zepatrik Oct 29, 2020
b661ace
deps: use gobuffalo replace
zepatrik Oct 29, 2020
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
6 changes: 3 additions & 3 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ import (
//
// swagger:model oAuth2Client
type Client struct {
PK int64 `json:"-" db:"pk"`
ID int64 `json:"-" db:"pk"`

// ID is the id for this client.
ID string `json:"client_id" db:"id"`
OutfacingID string `json:"client_id" db:"id"`

// Name is the human-readable string name of the client to be presented to the
// end-user during authorization.
Expand Down Expand Up @@ -235,7 +235,7 @@ func (c *Client) BeforeSave(_ *pop.Connection) error {
}

func (c *Client) GetID() string {
return c.ID
return c.OutfacingID
}

func (c *Client) GetRedirectURIs() []string {
Expand Down
2 changes: 1 addition & 1 deletion client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var _ fosite.Client = new(Client)

func TestClient(t *testing.T) {
c := &Client{
ID: "foo",
OutfacingID: "foo",
RedirectURIs: []string{"foo"},
Scope: "foo bar",
TokenEndpointAuthMethod: "none",
Expand Down
2 changes: 1 addition & 1 deletion client/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func (h *Handler) Update(w http.ResponseWriter, r *http.Request, ps httprouter.P
secret = c.Secret
}

c.ID = ps.ByName("id")
c.OutfacingID = ps.ByName("id")
if err := h.r.ClientValidator().Validate(&c); err != nil {
h.r.Writer().WriteError(w, r, err)
return
Expand Down
36 changes: 25 additions & 11 deletions client/manager_test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestHelperClientAutoGenerateKey(k string, m Storage) func(t *testing.T) {
return func(t *testing.T) {
ctx := context.TODO()
c := &Client{
ID: "foo",
OutfacingID: "foo",
Secret: "secret",
RedirectURIs: []string{"http://redirect"},
TermsOfServiceURI: "foo",
Expand All @@ -54,7 +54,7 @@ func TestHelperClientAuthenticate(k string, m Manager) func(t *testing.T) {
return func(t *testing.T) {
ctx := context.TODO()
require.NoError(t, m.CreateClient(ctx, &Client{
ID: "1234321",
OutfacingID: "1234321",
Secret: "secret",
RedirectURIs: []string{"http://redirect"},
}))
Expand All @@ -68,14 +68,28 @@ func TestHelperClientAuthenticate(k string, m Manager) func(t *testing.T) {
}
}

func TestHelperUpdateTwoClients(_ string, m Manager) func(t *testing.T) {
return func(t *testing.T) {
c1, c2 := &Client{OutfacingID: "klojdfc", Name: "test client 1"}, &Client{OutfacingID: "jlsdfkj", Name: "test client 2"}

require.NoError(t, m.CreateClient(context.Background(), c1))
require.NoError(t, m.CreateClient(context.Background(), c2))

c1.Name, c2.Name = "updated klojdfc client 1", "updated klojdfc client 2"

assert.NoError(t, m.UpdateClient(context.Background(), c1))
assert.NoError(t, m.UpdateClient(context.Background(), c2))
}
}

func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.T) {
return func(t *testing.T) {
ctx := context.TODO()
_, err := m.GetClient(ctx, "4321")
assert.NotNil(t, err)
ctx := context.Background()
_, err := m.GetClient(ctx, "1234")
require.Error(t, err)

c := &Client{
ID: "1234",
OutfacingID: "1234",
Name: "name",
Secret: "secret",
RedirectURIs: []string{"http://redirect", "http://redirect1"},
Expand Down Expand Up @@ -107,14 +121,14 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.
BackChannelLogoutSessionRequired: true,
}

assert.NoError(t, m.CreateClient(ctx, c))
require.NoError(t, m.CreateClient(ctx, c))
assert.Equal(t, c.GetID(), "1234")
if k != "http" {
assert.NotEmpty(t, c.GetHashedSecret())
}

assert.NoError(t, m.CreateClient(ctx, &Client{
ID: "2-1234",
OutfacingID: "2-1234",
Name: "name",
Secret: "secret",
RedirectURIs: []string{"http://redirect"},
Expand All @@ -130,8 +144,8 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.
ds, err := m.GetClients(ctx, 100, 0)
assert.NoError(t, err)
assert.Len(t, ds, 2)
assert.NotEqual(t, ds[0].ID, ds[1].ID)
assert.NotEqual(t, ds[0].ID, ds[1].ID)
assert.NotEqual(t, ds[0].OutfacingID, ds[1].OutfacingID)
assert.NotEqual(t, ds[0].OutfacingID, ds[1].OutfacingID)
// test if SecretExpiresAt was set properly
assert.Equal(t, ds[0].SecretExpiresAt, 0)
assert.Equal(t, ds[1].SecretExpiresAt, 1)
Expand All @@ -144,7 +158,7 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.
assert.NoError(t, err)

err = m.UpdateClient(ctx, &Client{
ID: "2-1234",
OutfacingID: "2-1234",
Name: "name-new",
Secret: "secret-new",
RedirectURIs: []string{"http://redirect/new"},
Expand Down
2 changes: 1 addition & 1 deletion client/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func NewValidatorWithClient(conf Configuration, client *http.Client) *Validator

func (v *Validator) Validate(c *Client) error {
id := uuid.New()
c.ID = stringsx.Coalesce(c.ID, id)
c.OutfacingID = stringsx.Coalesce(c.OutfacingID, id)

if c.TokenEndpointAuthMethod == "" {
c.TokenEndpointAuthMethod = "client_secret_basic"
Expand Down
36 changes: 18 additions & 18 deletions client/validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,59 +53,59 @@ func TestValidate(t *testing.T) {
{
in: new(Client),
check: func(t *testing.T, c *Client) {
assert.NotEmpty(t, c.ID)
assert.NotEmpty(t, c.OutfacingID)
assert.NotEmpty(t, c.GetID())
assert.Equal(t, c.GetID(), c.ID)
assert.Equal(t, c.GetID(), c.OutfacingID)
},
},
{
in: &Client{ID: "foo"},
in: &Client{OutfacingID: "foo"},
check: func(t *testing.T, c *Client) {
assert.Equal(t, c.GetID(), c.ID)
assert.Equal(t, c.GetID(), c.OutfacingID)
},
},
{
in: &Client{ID: "foo"},
in: &Client{OutfacingID: "foo"},
check: func(t *testing.T, c *Client) {
assert.Equal(t, c.GetID(), c.ID)
assert.Equal(t, c.GetID(), c.OutfacingID)
},
},
{
in: &Client{ID: "foo", UserinfoSignedResponseAlg: "foo"},
in: &Client{OutfacingID: "foo", UserinfoSignedResponseAlg: "foo"},
expectErr: true,
},
{
in: &Client{ID: "foo", TokenEndpointAuthMethod: "private_key_jwt"},
in: &Client{OutfacingID: "foo", TokenEndpointAuthMethod: "private_key_jwt"},
expectErr: true,
},
{
in: &Client{ID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, JSONWebKeysURI: "asdf", TokenEndpointAuthMethod: "private_key_jwt"},
in: &Client{OutfacingID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, JSONWebKeysURI: "asdf", TokenEndpointAuthMethod: "private_key_jwt"},
expectErr: true,
},
{
in: &Client{ID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, TokenEndpointAuthMethod: "private_key_jwt", TokenEndpointAuthSigningAlgorithm: "HS256"},
in: &Client{OutfacingID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, TokenEndpointAuthMethod: "private_key_jwt", TokenEndpointAuthSigningAlgorithm: "HS256"},
expectErr: true,
},
{
in: &Client{ID: "foo", PostLogoutRedirectURIs: []string{"https://bar/"}, RedirectURIs: []string{"https://foo/"}},
in: &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"https://bar/"}, RedirectURIs: []string{"https://foo/"}},
expectErr: true,
},
{
in: &Client{ID: "foo", PostLogoutRedirectURIs: []string{"http://foo/"}, RedirectURIs: []string{"https://foo/"}},
in: &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"http://foo/"}, RedirectURIs: []string{"https://foo/"}},
expectErr: true,
},
{
in: &Client{ID: "foo", PostLogoutRedirectURIs: []string{"https://foo:1234/"}, RedirectURIs: []string{"https://foo/"}},
in: &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"https://foo:1234/"}, RedirectURIs: []string{"https://foo/"}},
expectErr: true,
},
{
in: &Client{ID: "foo", PostLogoutRedirectURIs: []string{"https://foo/"}, RedirectURIs: []string{"https://foo/"}},
in: &Client{OutfacingID: "foo", PostLogoutRedirectURIs: []string{"https://foo/"}, RedirectURIs: []string{"https://foo/"}},
check: func(t *testing.T, c *Client) {
assert.Equal(t, []string{"https://foo/"}, []string(c.PostLogoutRedirectURIs))
},
},
{
in: &Client{ID: "foo"},
in: &Client{OutfacingID: "foo"},
check: func(t *testing.T, c *Client) {
assert.Equal(t, "public", c.SubjectType)
},
Expand All @@ -115,19 +115,19 @@ func TestValidate(t *testing.T) {
viper.Set(configuration.ViperKeySubjectTypesSupported, []string{"pairwise"})
return NewValidator(c)
},
in: &Client{ID: "foo"},
in: &Client{OutfacingID: "foo"},
check: func(t *testing.T, c *Client) {
assert.Equal(t, "pairwise", c.SubjectType)
},
},
{
in: &Client{ID: "foo", SubjectType: "pairwise"},
in: &Client{OutfacingID: "foo", SubjectType: "pairwise"},
check: func(t *testing.T, c *Client) {
assert.Equal(t, "pairwise", c.SubjectType)
},
},
{
in: &Client{ID: "foo", SubjectType: "foo"},
in: &Client{OutfacingID: "foo", SubjectType: "foo"},
expectErr: true,
},
} {
Expand Down
6 changes: 3 additions & 3 deletions consent/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func TestGetLogoutRequest(t *testing.T) {
reg := internal.NewRegistryMemory(t, conf)

if tc.exists {
cl := &client.Client{ID: "client" + key}
cl := &client.Client{OutfacingID: "client" + key}
require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
require.NoError(t, reg.ConsentManager().CreateLogoutRequest(context.TODO(), &LogoutRequest{
Client: cl,
Expand Down Expand Up @@ -147,7 +147,7 @@ func TestGetLoginRequest(t *testing.T) {
reg := internal.NewRegistryMemory(t, conf)

if tc.exists {
cl := &client.Client{ID: "client" + key}
cl := &client.Client{OutfacingID: "client" + key}
require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
require.NoError(t, reg.ConsentManager().CreateLoginRequest(context.Background(), &LoginRequest{
Client: cl,
Expand Down Expand Up @@ -192,7 +192,7 @@ func TestGetConsentRequest(t *testing.T) {
reg := internal.NewRegistryMemory(t, conf)

if tc.exists {
cl := &client.Client{ID: "client" + key}
cl := &client.Client{OutfacingID: "client" + key}
require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
require.NoError(t, reg.ConsentManager().CreateConsentRequest(context.Background(), &ConsentRequest{
Client: cl,
Expand Down
18 changes: 9 additions & 9 deletions consent/manager_test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func MockConsentRequest(key string, remember bool, rememberFor int, hasError boo
UILocales: []string{"fr" + key, "de" + key},
Display: "popup" + key,
},
Client: &client.Client{ID: "fk-client-" + key},
Client: &client.Client{OutfacingID: "fk-client-" + key},
RequestURL: "https://request-url/path" + key,
LoginChallenge: sqlxx.NullString("fk-login-challenge-" + key),
LoginSessionID: sqlxx.NullString("fk-login-session-" + key),
Expand Down Expand Up @@ -103,7 +103,7 @@ func MockLogoutRequest(key string, withClient bool) (c *LogoutRequest) {
var cl *client.Client
if withClient {
cl = &client.Client{
ID: "fk-client-" + key,
OutfacingID: "fk-client-" + key,
}
}
return &LogoutRequest{
Expand All @@ -128,7 +128,7 @@ func MockAuthRequest(key string, authAt bool) (c *LoginRequest, h *HandledLoginR
Display: "popup" + key,
},
RequestedAt: time.Now().UTC().Add(-time.Hour),
Client: &client.Client{ID: "fk-client-" + key},
Client: &client.Client{OutfacingID: "fk-client-" + key},
Subject: "subject" + key,
RequestURL: "https://request-url/path" + key,
Skip: true,
Expand Down Expand Up @@ -262,7 +262,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
return func(t *testing.T) {
t.Run("case=init-fks", func(t *testing.T) {
for _, k := range []string{"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "rv1", "rv2"} {
require.NoError(t, clientManager.CreateClient(context.Background(), &client.Client{ID: fmt.Sprintf("fk-client-%s", k)}))
require.NoError(t, clientManager.CreateClient(context.Background(), &client.Client{OutfacingID: fmt.Sprintf("fk-client-%s", k)}))

require.NoError(t, m.CreateLoginSession(context.Background(), &LoginSession{
ID: fmt.Sprintf("fk-login-session-%s", k),
Expand All @@ -273,7 +273,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
require.NoError(t, m.CreateLoginRequest(context.Background(), &LoginRequest{
ID: fmt.Sprintf("fk-login-challenge-%s", k),
Verifier: fmt.Sprintf("fk-login-verifier-%s", k),
Client: &client.Client{ID: fmt.Sprintf("fk-client-%s", k)},
Client: &client.Client{OutfacingID: fmt.Sprintf("fk-client-%s", k)},
AuthenticatedAt: sqlxx.NullTime(time.Now()),
RequestedAt: time.Now(),
}))
Expand Down Expand Up @@ -646,7 +646,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
require.NoError(t, err)
for _, consent := range consents {
assert.Contains(t, tc.challenges, consent.ID)
assert.Contains(t, tc.clients, consent.ConsentRequest.Client.ID)
assert.Contains(t, tc.clients, consent.ConsentRequest.Client.OutfacingID)
}
}

Expand Down Expand Up @@ -710,7 +710,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
}
require.NoError(t, m.CreateLoginSession(context.Background(), ls))

cl := &client.Client{ID: uuid.New().String()}
cl := &client.Client{OutfacingID: uuid.New().String()}
switch k % 4 {
case 0:
cl.FrontChannelLogoutURI = "http://some-url.com/"
Expand Down Expand Up @@ -746,10 +746,10 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
for _, e := range es {
var found bool
for _, a := range actual {
if e.ID == a.ID {
if e.OutfacingID == a.OutfacingID {
found = true
}
assert.Equal(t, e.ID, a.ID)
assert.Equal(t, e.OutfacingID, a.OutfacingID)
assert.Equal(t, e.FrontChannelLogoutURI, a.FrontChannelLogoutURI)
assert.Equal(t, e.BackChannelLogoutURI, a.BackChannelLogoutURI)
}
Expand Down
2 changes: 1 addition & 1 deletion consent/sdk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func TestSDK(t *testing.T) {
_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject("subject1"))
require.Error(t, err)

_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject(cr4.Subject).WithClient(&cr4.Client.ID))
_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject(cr4.Subject).WithClient(&cr4.Client.OutfacingID))
require.NoError(t, err)

_, err = sdk.Admin.RevokeConsentSessions(admin.NewRevokeConsentSessionsParams().WithSubject("subject1").WithAll(pointerx.Bool(true)))
Expand Down
6 changes: 3 additions & 3 deletions consent/strategy_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ func (s *DefaultStrategy) obfuscateSubjectIdentifier(cl fosite.Client, subject,
if c, ok := cl.(*client.Client); ok && c.SubjectType == "pairwise" {
algorithm, ok := s.r.SubjectIdentifierAlgorithm()[c.SubjectType]
if !ok {
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf(`Subject Identifier Algorithm "%s" was requested by OAuth 2.0 Client "%s", but is not configured.`, c.SubjectType, c.ID)))
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf(`Subject Identifier Algorithm "%s" was requested by OAuth 2.0 Client "%s", but is not configured.`, c.SubjectType, c.OutfacingID)))
}

if len(forcedIdentifier) > 0 {
Expand Down Expand Up @@ -683,7 +683,7 @@ func (s *DefaultStrategy) executeBackChannelLogout(ctx context.Context, r *http.

t, _, err := s.r.OpenIDJWTStrategy().Generate(ctx, jwtgo.MapClaims{
"iss": s.c.IssuerURL().String(),
"aud": []string{c.ID},
"aud": []string{c.OutfacingID},
"iat": time.Now().UTC().Unix(),
"jti": uuid.New(),
"events": map[string]struct{}{"http://schemas.openid.net/event/backchannel-logout": {}},
Expand All @@ -695,7 +695,7 @@ func (s *DefaultStrategy) executeBackChannelLogout(ctx context.Context, r *http.
return err
}

tasks = append(tasks, task{url: c.BackChannelLogoutURI, clientID: c.ID, token: t})
tasks = append(tasks, task{url: c.BackChannelLogoutURI, clientID: c.OutfacingID, token: t})
}

var wg sync.WaitGroup
Expand Down
Loading