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

feat: allow deletion of an individual OIDC credential #3968

Merged
merged 3 commits into from
Jun 25, 2024
Merged

feat: allow deletion of an individual OIDC credential #3968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e53bcf1
feat: allow deletion of an individual OIDC credential
alnr Jun 21, 2024
b11913f
feat: add TestDeleteCredentialOIDCFromIdentity
alnr Jun 24, 2024
0ee8dea
feat: improve coverage
alnr Jun 24, 2024
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
67 changes: 19 additions & 48 deletions identity/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,69 +910,36 @@
h.r.Writer().Write(w, r, WithCredentialsMetadataAndAdminMetadataInJSON(updatedIdentity))
}

func deletCredentialWebAuthFromIdentity(identity *Identity) (*Identity, error) {
cred, ok := identity.GetCredentials(CredentialsTypeWebAuthn)
if !ok {
// This should never happend as it's checked earlier in the code;
// But we never know...
return nil, errors.WithStack(herodot.ErrNotFound.WithReasonf("You tried to remove a CredentialsTypeWebAuthn but this user have no CredentialsTypeWebAuthn set up."))
}

var cc CredentialsWebAuthnConfig
if err := json.Unmarshal(cred.Config, &cc); err != nil {
// Database has been tampered or the json schema are incompatible (migration issue);
return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to decode identity credentials.").WithDebug(err.Error()))
}

updated := make([]CredentialWebAuthn, 0)
for k, cred := range cc.Credentials {
if cred.IsPasswordless {
updated = append(updated, cc.Credentials[k])
}
}

if len(updated) == 0 {
identity.DeleteCredentialsType(CredentialsTypeWebAuthn)
return identity, nil
}

cc.Credentials = updated
message, err := json.Marshal(cc)
if err != nil {
return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to encode identity credentials.").WithDebug(err.Error()))
}

cred.Config = message
identity.SetCredentials(CredentialsTypeWebAuthn, *cred)
return identity, nil
}

// Delete Credential Parameters
//
// swagger:parameters deleteIdentityCredentials
//
//nolint:deadcode,unused
//lint:ignore U1000 Used to generate Swagger and OpenAPI definitions
type deleteIdentityCredentials struct {
type _ struct {
// ID is the identity's ID.
//
// required: true
// in: path
ID string `json:"id"`

// Type is the type of credentials to be deleted.
// Type is the type of credentials to delete.
//
// required: true
// in: path
Type CredentialsType `json:"type"`

// Identifier is the identifier of the OIDC credential to delete.
// Find the identifier by calling the `GET /admin/identities/{id}?include_credential=oidc` endpoint.
//
// required: false
// in: query
Identifier string `json:"identifier"`
}

// swagger:route DELETE /admin/identities/{id}/credentials/{type} identity deleteIdentityCredentials
//
// # Delete a credential for a specific identity
//
// Delete an [identity](https://www.ory.sh/docs/kratos/concepts/identity-user-model) credential by its type
// You can only delete second factor (aal2) credentials.
// Delete an [identity](https://www.ory.sh/docs/kratos/concepts/identity-user-model) credential by its type.
// You cannot delete password or code auth credentials through this API.
//
// Consumes:
// - application/json
Expand Down Expand Up @@ -1006,14 +973,18 @@
case CredentialsTypeLookup, CredentialsTypeTOTP:
identity.DeleteCredentialsType(cred.Type)
case CredentialsTypeWebAuthn:
identity, err = deletCredentialWebAuthFromIdentity(identity)
if err != nil {
if err = identity.deleteCredentialWebAuthFromIdentity(); err != nil {
h.r.Writer().WriteError(w, r, err)
return
}
case CredentialsTypeOIDC, CredentialsTypePassword, CredentialsTypeCodeAuth:
h.r.Writer().WriteError(w, r, errors.WithStack(herodot.ErrBadRequest.WithReasonf("You can't remove first factor credentials.")))
case CredentialsTypePassword, CredentialsTypeCodeAuth:
h.r.Writer().WriteError(w, r, errors.WithStack(herodot.ErrBadRequest.WithReasonf("You cannot remove first factor credentials.")))
return
case CredentialsTypeOIDC:
if err := identity.deleteCredentialOIDCFromIdentity(r.URL.Query().Get("identifier")); err != nil {
h.r.Writer().WriteError(w, r, err)
return

Check warning on line 986 in identity/handler.go

View check run for this annotation

Codecov / codecov/patch

identity/handler.go#L985-L986 

Added lines #L985 - L986 were not covered by tests
}
default:
h.r.Writer().WriteError(w, r, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Unknown credentials type %s.", cred.Type)))
return
Expand Down
118 changes: 89 additions & 29 deletions identity/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
"github.com/ory/kratos/internal/testhelpers"
"github.com/ory/kratos/schema"
"github.com/ory/kratos/x"
"github.com/ory/x/ioutilx"
"github.com/ory/x/randx"
"github.com/ory/x/snapshotx"
"github.com/ory/x/sqlxx"
"github.com/ory/x/urlx"
Expand Down Expand Up @@ -81,8 +83,9 @@ func TestHandler(t *testing.T) {

res, err := base.Client().Do(req)
require.NoError(t, err)
defer res.Body.Close()

require.EqualValues(t, expectCode, res.StatusCode)
require.EqualValues(t, expectCode, res.StatusCode, "%s", ioutilx.MustReadAll(res.Body))
}

send := func(t *testing.T, base *httptest.Server, method, href string, expectCode int, send interface{}) gjson.Result {
Expand Down Expand Up @@ -1497,15 +1500,15 @@ func TestHandler(t *testing.T) {

t.Run("case=should delete credential of a specific user and no longer be able to retrieve it", func(t *testing.T) {
ignoreDefault := []string{"id", "schema_url", "state_changed_at", "created_at", "updated_at"}
createIdentity := func(identities map[identity.CredentialsType]string) func(t *testing.T) *identity.Identity {
type M = map[identity.CredentialsType]identity.Credentials
createIdentity := func(creds M) func(*testing.T) *identity.Identity {
return func(t *testing.T) *identity.Identity {
i := identity.NewIdentity("")
for ct, config := range identities {
i.SetCredentials(ct, identity.Credentials{
Type: ct,
Config: sqlxx.JSONRawMessage(config),
})
for k, v := range creds {
v.Type = k
creds[k] = v
}
i.Credentials = creds
i.Traits = identity.Traits("{}")
require.NoError(t, reg.Persister().CreateIdentity(context.Background(), i))
return i
Expand All @@ -1516,26 +1519,83 @@ func TestHandler(t *testing.T) {
remove(t, ts, "/identities/"+x.NewUUID().String()+"/credentials/azerty", http.StatusNotFound)
})
t.Run("type=remove unknown type/"+name, func(t *testing.T) {
i := createIdentity(map[identity.CredentialsType]string{
identity.CredentialsTypePassword: `{"secret":"pst"}`,
i := createIdentity(M{
identity.CredentialsTypePassword: {Config: []byte(`{"secret":"pst"}`)},
})(t)
remove(t, ts, "/identities/"+i.ID.String()+"/credentials/azerty", http.StatusNotFound)
})
t.Run("type=remove password type/"+name, func(t *testing.T) {
i := createIdentity(map[identity.CredentialsType]string{
identity.CredentialsTypePassword: `{"secret":"pst"}`,
i := createIdentity(M{
identity.CredentialsTypePassword: {Config: []byte(`{"secret":"pst"}`)},
})(t)
remove(t, ts, "/identities/"+i.ID.String()+"/credentials/password", http.StatusBadRequest)
})
t.Run("type=remove oidc type/"+name, func(t *testing.T) {
i := createIdentity(map[identity.CredentialsType]string{
identity.CredentialsTypeOIDC: `{"id":"pst"}`,
// force ordering among github identifiers
githubSubject := "0" + randx.MustString(7, randx.Numeric)
githubSubject2 := "1" + randx.MustString(7, randx.Numeric)
googleSubject := randx.MustString(8, randx.Numeric)
initialConfig := []byte(fmt.Sprintf(`{
"providers": [
{
"subject": %q,
"provider": "github"
},
{
"subject": %q,
"provider": "github"
},
{
"subject": %q,
"provider": "google"
}
]
}`, githubSubject, githubSubject2, googleSubject))
identifiers := []string{
identity.OIDCUniqueID("github", githubSubject),
identity.OIDCUniqueID("github", githubSubject2),
identity.OIDCUniqueID("google", googleSubject),
}
i := createIdentity(M{
identity.CredentialsTypeOIDC: {
Identifiers: identifiers,
Config: initialConfig,
},
})(t)
remove(t, ts, "/identities/"+i.ID.String()+"/credentials/oidc", http.StatusBadRequest)
res := get(t, ts, "/identities/"+i.ID.String()+"?include_credential=oidc", http.StatusOK)
assert.EqualValues(t, i.ID.String(), res.Get("id").String(), "%s", res.Raw)
assert.Len(t, res.Get("credentials.oidc.identifiers").Array(), 3, "%s", res.Raw)
assert.EqualValues(t, res.Get("credentials.oidc.identifiers.0").String(), identifiers[0], "%s", res.Raw)
assert.EqualValues(t, res.Get("credentials.oidc.identifiers.1").String(), identifiers[1], "%s", res.Raw)
assert.EqualValues(t, res.Get("credentials.oidc.identifiers.2").String(), identifiers[2], "%s", res.Raw)

oidConfig := gjson.Parse(res.Get("credentials.oidc.config").String())
assert.Len(t, res.Get("credentials.oidc.identifiers").Array(), 3, "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.0.provider").String(), "github", "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.0.subject").String(), githubSubject, "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.1.provider").String(), "github", "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.1.subject").String(), githubSubject2, "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.2.provider").String(), "google", "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.2.subject").String(), googleSubject, "%s", res.Raw)

remove(t, ts, "/identities/"+i.ID.String()+"/credentials/oidc?identifier="+identifiers[1], http.StatusNoContent)
res = get(t, ts, "/identities/"+i.ID.String()+"?include_credential=oidc", http.StatusOK)

assert.EqualValues(t, i.ID.String(), res.Get("id").String(), "%s", res.Raw)
assert.Len(t, res.Get("credentials.oidc.identifiers").Array(), 2, "%s", res.Raw)
assert.EqualValues(t, res.Get("credentials.oidc.identifiers.0").String(), identifiers[0], "%s", res.Raw)
assert.EqualValues(t, res.Get("credentials.oidc.identifiers.1").String(), identifiers[2], "%s", res.Raw)

oidConfig = gjson.Parse(res.Get("credentials.oidc.config").String())
assert.Len(t, res.Get("credentials.oidc.identifiers").Array(), 2, "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.0.provider").String(), "github", "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.0.subject").String(), githubSubject, "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.1.provider").String(), "google", "%s", res.Raw)
assert.EqualValues(t, oidConfig.Get("providers.1.subject").String(), googleSubject, "%s", res.Raw)
})
t.Run("type=remove webauthn passwordless type/"+name, func(t *testing.T) {
expected := `{"credentials":[{"id":"THTndqZP5Mjvae1BFvJMaMfEMm7O7HE1ju+7PBaYA7Y=","added_at":"2022-12-16T14:11:55Z","public_key":"pQECAyYgASFYIMJLQhJxQRzhnKPTcPCUODOmxYDYo2obrm9bhp5lvSZ3IlggXjhZvJaPUqF9PXqZqTdWYPR7R+b2n/Wi+IxKKXsS4rU=","display_name":"test","authenticator":{"aaguid":"rc4AAjW8xgpkiwsl8fBVAw==","sign_count":0,"clone_warning":false},"is_passwordless":true,"attestation_type":"none"}],"user_handle":"Ef5JiMpMRwuzauWs/9J0gQ=="}`
i := createIdentity(map[identity.CredentialsType]string{identity.CredentialsTypeWebAuthn: expected})(t)
i := createIdentity(M{identity.CredentialsTypeWebAuthn: {Config: []byte(expected)}})(t)
remove(t, ts, "/identities/"+i.ID.String()+"/credentials/webauthn", http.StatusNoContent)
// Check that webauthn has not been deleted
res := get(t, ts, "/identities/"+i.ID.String(), http.StatusOK)
Expand Down Expand Up @@ -1608,7 +1668,7 @@ func TestHandler(t *testing.T) {
message, err := json.Marshal(config)
require.NoError(t, err)

i := createIdentity(map[identity.CredentialsType]string{identity.CredentialsTypeWebAuthn: string(message)})(t)
i := createIdentity(M{identity.CredentialsTypeWebAuthn: {Config: message}})(t)
remove(t, ts, "/identities/"+i.ID.String()+"/credentials/webauthn", http.StatusNoContent)
// Check that webauthn has not been deleted
res := get(t, ts, "/identities/"+i.ID.String(), http.StatusOK)
Expand All @@ -1618,10 +1678,10 @@ func TestHandler(t *testing.T) {
require.NoError(t, err)
snapshotx.SnapshotT(t, identity.WithCredentialsAndAdminMetadataInJSON(*actual), snapshotx.ExceptNestedKeys(append(ignoreDefault, "hashed_password")...), snapshotx.ExceptPaths("credentials.oidc.identifiers"))
})
for ct, ctConf := range map[identity.CredentialsType]string{
identity.CredentialsTypeLookup: `{"recovery_codes": [{"code": "aaa"}]}`,
identity.CredentialsTypeTOTP: `{"totp_url":"otpauth://totp/test"}`,
identity.CredentialsTypeWebAuthn: `{"credentials":[{"id":"THTndqZP5Mjvae1BFvJMaMfEMm7O7HE1ju+7PBaYA7Y=","added_at":"2022-12-16T14:11:55Z","public_key":"pQECAyYgASFYIMJLQhJxQRzhnKPTcPCUODOmxYDYo2obrm9bhp5lvSZ3IlggXjhZvJaPUqF9PXqZqTdWYPR7R+b2n/Wi+IxKKXsS4rU=","display_name":"test","authenticator":{"aaguid":"rc4AAjW8xgpkiwsl8fBVAw==","sign_count":0,"clone_warning":false},"is_passwordless":false,"attestation_type":"none"}],"user_handle":"Ef5JiMpMRwuzauWs/9J0gQ=="}`,
for ct, ctConf := range map[identity.CredentialsType][]byte{
identity.CredentialsTypeLookup: []byte(`{"recovery_codes": [{"code": "aaa"}]}`),
identity.CredentialsTypeTOTP: []byte(`{"totp_url":"otpauth://totp/test"}`),
identity.CredentialsTypeWebAuthn: []byte(`{"credentials":[{"id":"THTndqZP5Mjvae1BFvJMaMfEMm7O7HE1ju+7PBaYA7Y=","added_at":"2022-12-16T14:11:55Z","public_key":"pQECAyYgASFYIMJLQhJxQRzhnKPTcPCUODOmxYDYo2obrm9bhp5lvSZ3IlggXjhZvJaPUqF9PXqZqTdWYPR7R+b2n/Wi+IxKKXsS4rU=","display_name":"test","authenticator":{"aaguid":"rc4AAjW8xgpkiwsl8fBVAw==","sign_count":0,"clone_warning":false},"is_passwordless":false,"attestation_type":"none"}],"user_handle":"Ef5JiMpMRwuzauWs/9J0gQ=="}`),
} {
t.Run("type=remove "+string(ct)+"/"+name, func(t *testing.T) {
for _, tc := range []struct {
Expand All @@ -1632,25 +1692,25 @@ func TestHandler(t *testing.T) {
{
desc: "with",
exist: true,
setup: createIdentity(map[identity.CredentialsType]string{
identity.CredentialsTypePassword: `{"secret":"pst"}`,
ct: ctConf,
setup: createIdentity(M{
identity.CredentialsTypePassword: {Config: []byte(`{"secret":"pst"}`)},
ct: {Config: ctConf},
}),
},
{
desc: "without",
exist: false,
setup: createIdentity(map[identity.CredentialsType]string{
identity.CredentialsTypePassword: `{"secret":"pst"}`,
setup: createIdentity(M{
identity.CredentialsTypePassword: {Config: []byte(`{"secret":"pst"}`)},
}),
},
{
desc: "multiple",
exist: true,
setup: createIdentity(map[identity.CredentialsType]string{
identity.CredentialsTypePassword: `{"secret":"pst"}`,
identity.CredentialsTypeOIDC: `{"id":"pst"}`,
ct: ctConf,
setup: createIdentity(M{
identity.CredentialsTypePassword: {Config: []byte(`{"secret":"pst"}`)},
identity.CredentialsTypeOIDC: {Config: []byte(`{"id":"pst"}`)},
ct: {Config: ctConf},
}),
},
} {
Expand Down
74 changes: 74 additions & 0 deletions identity/identity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,80 @@
return &ii, nil
}

func (i *Identity) deleteCredentialWebAuthFromIdentity() error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was moved here from identity/handler.go where it had the signature func deletCredentialWebAuthFromIdentity(identity *Identity) (*Identity, error)

cred, ok := i.GetCredentials(CredentialsTypeWebAuthn)
if !ok {
// This should never happend as it's checked earlier in the code;
// But we never know...
return errors.WithStack(herodot.ErrNotFound.WithReasonf("You tried to remove a WebAuthn credential but this user has no such credential set up."))

Check warning on line 515 in identity/identity.go

View check run for this annotation

Codecov / codecov/patch

identity/identity.go#L515 

Added line #L515 was not covered by tests
}

var cc CredentialsWebAuthnConfig
if err := json.Unmarshal(cred.Config, &cc); err != nil {
// Database has been tampered or the json schema are incompatible (migration issue);
return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to decode identity credentials.").WithDebug(err.Error()))

Check warning on line 521 in identity/identity.go

View check run for this annotation

Codecov / codecov/patch

identity/identity.go#L521 

Added line #L521 was not covered by tests
}

updated := make([]CredentialWebAuthn, 0)
for k, cred := range cc.Credentials {
if cred.IsPasswordless {
updated = append(updated, cc.Credentials[k])
}
}

if len(updated) == 0 {
i.DeleteCredentialsType(CredentialsTypeWebAuthn)
return nil
}

cc.Credentials = updated
message, err := json.Marshal(cc)
if err != nil {
return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to encode identity credentials.").WithDebug(err.Error()))

Check warning on line 539 in identity/identity.go

View check run for this annotation

Codecov / codecov/patch

identity/identity.go#L539 

Added line #L539 was not covered by tests
}

cred.Config = message
i.SetCredentials(CredentialsTypeWebAuthn, *cred)
return nil
}

func (i *Identity) deleteCredentialOIDCFromIdentity(identifierToDelete string) error {
if identifierToDelete == "" {
return errors.WithStack(herodot.ErrBadRequest.WithReasonf("You must provide an identifier to delete this credential."))
}
_, hasOIDC := i.GetCredentials(CredentialsTypeOIDC)
if !hasOIDC {
return errors.WithStack(herodot.ErrNotFound.WithReasonf("You tried to remove an OIDC credential but this user has no such credential set up."))
}
var oidcConfig CredentialsOIDC
creds, err := i.ParseCredentials(CredentialsTypeOIDC, &oidcConfig)
if err != nil {
return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to decode identity credentials.").WithDebug(err.Error()))

Check warning on line 558 in identity/identity.go

View check run for this annotation

Codecov / codecov/patch

identity/identity.go#L558 

Added line #L558 was not covered by tests
}

var updatedIdentifiers []string
var updatedProviders []CredentialsOIDCProvider
var found bool
for _, cfg := range oidcConfig.Providers {
if identifierToDelete == OIDCUniqueID(cfg.Provider, cfg.Subject) {
found = true
continue
}
updatedIdentifiers = append(updatedIdentifiers, OIDCUniqueID(cfg.Provider, cfg.Subject))
updatedProviders = append(updatedProviders, cfg)
}
if !found {
return errors.WithStack(herodot.ErrNotFound.WithReasonf("The identifier `%s` was not found among OIDC credentials.", identifierToDelete))
}
creds.Identifiers = updatedIdentifiers
creds.Config, err = json.Marshal(&CredentialsOIDC{Providers: updatedProviders})
if err != nil {
return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to encode identity credentials.").WithDebug(err.Error()))

Check warning on line 578 in identity/identity.go

View check run for this annotation

Codecov / codecov/patch

identity/identity.go#L578 

Added line #L578 was not covered by tests
}
i.Credentials[CredentialsTypeOIDC] = *creds
return nil
}

// Patch Identities Parameters
//
// swagger:parameters batchPatchIdentities
Expand Down
Loading
Loading