From 1017568cf1e9fc3ac3c0d2c1e788ee47a4d5f5fc Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Tue, 20 Jun 2023 14:39:50 +0200 Subject: [PATCH] fix: provide more information in the retrieve idp information (#5927) * fix: provide more information in the retrieve idp information * change raw_information to proto struct * change unmarshal * improve description --- internal/api/grpc/fields.go | 31 ++++++++++ internal/api/grpc/user/v2/user.go | 20 ++++-- .../api/grpc/user/v2/user_integration_test.go | 62 ++++++++++++------- internal/api/grpc/user/v2/user_test.go | 35 ++++++++--- internal/command/idp_intent.go | 2 + internal/command/idp_intent_model.go | 4 ++ internal/command/idp_intent_test.go | 18 +++--- internal/repository/idpintent/intent.go | 6 ++ proto/zitadel/user/v2alpha/idp.proto | 43 ++++++++++--- 9 files changed, 169 insertions(+), 52 deletions(-) diff --git a/internal/api/grpc/fields.go b/internal/api/grpc/fields.go index f3701b21eb..7e83c08f4c 100644 --- a/internal/api/grpc/fields.go +++ b/internal/api/grpc/fields.go @@ -3,9 +3,19 @@ package grpc import ( "testing" + "github.com/stretchr/testify/require" "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/types/known/structpb" ) +var CustomMappers = map[protoreflect.FullName]func(testing.TB, protoreflect.ProtoMessage) any{ + "google.protobuf.Struct": func(t testing.TB, msg protoreflect.ProtoMessage) any { + e, ok := msg.(*structpb.Struct) + require.True(t, ok) + return e.AsMap() + }, +} + // AllFieldsSet recusively checks if all values in a message // have a non-zero value. func AllFieldsSet(t testing.TB, msg protoreflect.Message, ignoreTypes ...protoreflect.FullName) { @@ -36,3 +46,24 @@ func AllFieldsSet(t testing.TB, msg protoreflect.Message, ignoreTypes ...protore } } } + +func AllFieldsEqual(t testing.TB, expected, actual protoreflect.Message, customMappers map[protoreflect.FullName]func(testing.TB, protoreflect.ProtoMessage) any) { + md := expected.Descriptor() + name := md.FullName() + if mapper := customMappers[name]; mapper != nil { + require.Equal(t, mapper(t, expected.Interface()), mapper(t, actual.Interface())) + return + } + + fields := md.Fields() + + for i := 0; i < fields.Len(); i++ { + fd := fields.Get(i) + + if fd.Kind() == protoreflect.MessageKind { + AllFieldsEqual(t, expected.Get(fd).Message(), actual.Get(fd).Message(), customMappers) + } else { + require.Equal(t, expected.Get(fd).Interface(), actual.Get(fd).Interface()) + } + } +} diff --git a/internal/api/grpc/user/v2/user.go b/internal/api/grpc/user/v2/user.go index 97e13e05c5..0a25076247 100644 --- a/internal/api/grpc/user/v2/user.go +++ b/internal/api/grpc/user/v2/user.go @@ -6,6 +6,7 @@ import ( "io" "golang.org/x/text/language" + "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/timestamppb" "github.com/zitadel/zitadel/internal/api/authz" @@ -64,8 +65,8 @@ func addUserRequestToAddHuman(req *user.AddHumanUserRequest) (*command.AddHuman, for i, link := range req.GetIdpLinks() { links[i] = &command.AddLink{ IDPID: link.GetIdpId(), - IDPExternalID: link.GetIdpExternalId(), - DisplayName: link.GetDisplayName(), + IDPExternalID: link.GetUserId(), + DisplayName: link.GetUserName(), } } return &command.AddHuman{ @@ -124,8 +125,8 @@ func (s *Server) AddIDPLink(ctx context.Context, req *user.AddIDPLinkRequest) (_ orgID := authz.GetCtxData(ctx).OrgID details, err := s.command.AddUserIDPLink(ctx, req.UserId, orgID, &domain.UserIDPLink{ IDPConfigID: req.GetIdpLink().GetIdpId(), - ExternalUserID: req.GetIdpLink().GetIdpExternalId(), - DisplayName: req.GetIdpLink().GetDisplayName(), + ExternalUserID: req.GetIdpLink().GetUserId(), + DisplayName: req.GetIdpLink().GetUserName(), }) if err != nil { return nil, err @@ -176,6 +177,12 @@ func intentToIDPInformationPb(intent *command.IDPIntentWriteModel, alg crypto.En return nil, err } } + rawInformation := new(structpb.Struct) + err = rawInformation.UnmarshalJSON(intent.IDPUser) + if err != nil { + return nil, err + } + return &user.RetrieveIdentityProviderInformationResponse{ Details: &object_pb.Details{ Sequence: intent.ProcessedSequence, @@ -189,7 +196,10 @@ func intentToIDPInformationPb(intent *command.IDPIntentWriteModel, alg crypto.En IdToken: idToken, }, }, - IdpInformation: intent.IDPUser, + IdpId: intent.IDPID, + UserId: intent.IDPUserID, + UserName: intent.IDPUserName, + RawInformation: rawInformation, }, }, nil } diff --git a/internal/api/grpc/user/v2/user_integration_test.go b/internal/api/grpc/user/v2/user_integration_test.go index 137b4eabe5..b78cf687cd 100644 --- a/internal/api/grpc/user/v2/user_integration_test.go +++ b/internal/api/grpc/user/v2/user_integration_test.go @@ -15,11 +15,13 @@ import ( "github.com/stretchr/testify/require" "github.com/zitadel/oidc/v2/pkg/oidc" "golang.org/x/oauth2" + "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/timestamppb" "github.com/zitadel/zitadel/internal/api/authz" + "github.com/zitadel/zitadel/internal/api/grpc" "github.com/zitadel/zitadel/internal/command" - "github.com/zitadel/zitadel/internal/idp/providers/oauth" + openid "github.com/zitadel/zitadel/internal/idp/providers/oidc" "github.com/zitadel/zitadel/internal/integration" "github.com/zitadel/zitadel/internal/repository/idp" object "github.com/zitadel/zitadel/pkg/grpc/object/v2alpha" @@ -81,12 +83,15 @@ func createSuccessfulIntent(t *testing.T, idpID string) (string, string, time.Ti intentID := createIntent(t, idpID) writeModel, err := Tester.Commands.GetIntentWriteModel(ctx, intentID, Tester.Organisation.ID) require.NoError(t, err) - idpUser := &oauth.UserMapper{ - RawInfo: map[string]interface{}{ - "id": "id", + idpUser := openid.NewUser( + &oidc.UserInfo{ + Subject: "id", + UserInfoProfile: oidc.UserInfoProfile{ + PreferredUsername: "username", + }, }, - } - idpSession := &oauth.Session{ + ) + idpSession := &openid.Session{ Tokens: &oidc.Tokens[*oidc.IDTokenClaims]{ Token: &oauth2.Token{ AccessToken: "accessToken", @@ -386,9 +391,9 @@ func TestServer_AddHumanUser(t *testing.T) { }, IdpLinks: []*user.IDPLink{ { - IdpId: "idpID", - IdpExternalId: "externalID", - DisplayName: "displayName", + IdpId: "idpID", + UserId: "userID", + UserName: "username", }, }, }, @@ -433,9 +438,9 @@ func TestServer_AddHumanUser(t *testing.T) { }, IdpLinks: []*user.IDPLink{ { - IdpId: idpID, - IdpExternalId: "externalID", - DisplayName: "displayName", + IdpId: idpID, + UserId: "userID", + UserName: "username", }, }, }, @@ -495,9 +500,9 @@ func TestServer_AddIDPLink(t *testing.T) { &user.AddIDPLinkRequest{ UserId: "userID", IdpLink: &user.IDPLink{ - IdpId: idpID, - IdpExternalId: "externalID", - DisplayName: "displayName", + IdpId: idpID, + UserId: "userID", + UserName: "username", }, }, }, @@ -511,9 +516,9 @@ func TestServer_AddIDPLink(t *testing.T) { &user.AddIDPLinkRequest{ UserId: Tester.Users[integration.OrgOwner].ID, IdpLink: &user.IDPLink{ - IdpId: "idpID", - IdpExternalId: "externalID", - DisplayName: "displayName", + IdpId: "idpID", + UserId: "userID", + UserName: "username", }, }, }, @@ -527,9 +532,9 @@ func TestServer_AddIDPLink(t *testing.T) { &user.AddIDPLinkRequest{ UserId: Tester.Users[integration.OrgOwner].ID, IdpLink: &user.IDPLink{ - IdpId: idpID, - IdpExternalId: "externalID", - DisplayName: "displayName", + IdpId: idpID, + UserId: "userID", + UserName: "username", }, }, }, @@ -678,7 +683,17 @@ func TestServer_RetrieveIdentityProviderInformation(t *testing.T) { IdToken: gu.Ptr("idToken"), }, }, - IdpInformation: []byte(`{"RawInfo":{"id":"id"}}`), + IdpId: idpID, + UserId: "id", + UserName: "username", + RawInformation: func() *structpb.Struct { + s, err := structpb.NewStruct(map[string]interface{}{ + "sub": "id", + "preferred_username": "username", + }) + require.NoError(t, err) + return s + }(), }, }, wantErr: false, @@ -693,8 +708,7 @@ func TestServer_RetrieveIdentityProviderInformation(t *testing.T) { require.NoError(t, err) } - require.Equal(t, tt.want.GetDetails(), got.GetDetails()) - require.Equal(t, tt.want.GetIdpInformation(), got.GetIdpInformation()) + grpc.AllFieldsEqual(t, got.ProtoReflect(), tt.want.ProtoReflect(), grpc.CustomMappers) }) } } diff --git a/internal/api/grpc/user/v2/user_test.go b/internal/api/grpc/user/v2/user_test.go index 044099a89a..f6153f6500 100644 --- a/internal/api/grpc/user/v2/user_test.go +++ b/internal/api/grpc/user/v2/user_test.go @@ -9,6 +9,8 @@ import ( "github.com/muhlemmer/gu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/timestamppb" "github.com/zitadel/zitadel/internal/api/grpc" @@ -21,6 +23,8 @@ import ( user "github.com/zitadel/zitadel/pkg/grpc/user/v2alpha" ) +var ignoreTypes = []protoreflect.FullName{"google.protobuf.Duration", "google.protobuf.Struct"} + func Test_hashedPasswordToCommand(t *testing.T) { type args struct { hashed *user.HashedPassword @@ -128,8 +132,10 @@ func Test_intentToIDPInformationPb(t *testing.T) { InstanceID: "instanceID", ChangeDate: time.Date(2019, 4, 1, 1, 1, 1, 1, time.Local), }, - IDPID: "idpID", - IDPUser: []byte(`{"id": "id"}`), + IDPID: "idpID", + IDPUser: []byte(`{"userID": "idpUserID", "username": "username"}`), + IDPUserID: "idpUserID", + IDPUserName: "username", IDPAccessToken: &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -158,8 +164,10 @@ func Test_intentToIDPInformationPb(t *testing.T) { InstanceID: "instanceID", ChangeDate: time.Date(2019, 4, 1, 1, 1, 1, 1, time.Local), }, - IDPID: "idpID", - IDPUser: []byte(`{"id": "id"}`), + IDPID: "idpID", + IDPUser: []byte(`{"userID": "idpUserID", "username": "username"}`), + IDPUserID: "idpUserID", + IDPUserName: "username", IDPAccessToken: &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -184,8 +192,19 @@ func Test_intentToIDPInformationPb(t *testing.T) { Oauth: &user.IDPOAuthAccessInformation{ AccessToken: "accessToken", IdToken: gu.Ptr("idToken"), - }}, - IdpInformation: []byte(`{"id": "id"}`), + }, + }, + IdpId: "idpID", + UserId: "idpUserID", + UserName: "username", + RawInformation: func() *structpb.Struct { + s, err := structpb.NewStruct(map[string]interface{}{ + "userID": "idpUserID", + "username": "username", + }) + require.NoError(t, err) + return s + }(), }, }, err: nil, @@ -196,9 +215,9 @@ func Test_intentToIDPInformationPb(t *testing.T) { t.Run(tt.name, func(t *testing.T) { got, err := intentToIDPInformationPb(tt.args.intent, tt.args.alg) require.ErrorIs(t, err, tt.res.err) - assert.Equal(t, tt.res.resp, got) + grpc.AllFieldsEqual(t, got.ProtoReflect(), tt.res.resp.ProtoReflect(), grpc.CustomMappers) if tt.res.resp != nil { - grpc.AllFieldsSet(t, got.ProtoReflect()) + grpc.AllFieldsSet(t, got.ProtoReflect(), ignoreTypes...) } }) } diff --git a/internal/command/idp_intent.go b/internal/command/idp_intent.go index 1045b77013..a6d64dd78f 100644 --- a/internal/command/idp_intent.go +++ b/internal/command/idp_intent.go @@ -123,6 +123,8 @@ func (c *Commands) SucceedIDPIntent(ctx context.Context, writeModel *IDPIntentWr ctx, &idpintent.NewAggregate(writeModel.AggregateID, writeModel.ResourceOwner).Aggregate, idpInfo, + idpUser.GetID(), + idpUser.GetPreferredUsername(), userID, accessToken, idToken, diff --git a/internal/command/idp_intent_model.go b/internal/command/idp_intent_model.go index cd0cb6e8e0..182b06b3d8 100644 --- a/internal/command/idp_intent_model.go +++ b/internal/command/idp_intent_model.go @@ -16,6 +16,8 @@ type IDPIntentWriteModel struct { FailureURL *url.URL IDPID string IDPUser []byte + IDPUserID string + IDPUserName string IDPAccessToken *crypto.CryptoValue IDPIDToken string UserID string @@ -72,6 +74,8 @@ func (wm *IDPIntentWriteModel) reduceStartedEvent(e *idpintent.StartedEvent) { func (wm *IDPIntentWriteModel) reduceSucceededEvent(e *idpintent.SucceededEvent) { wm.UserID = e.UserID wm.IDPUser = e.IDPUser + wm.IDPUserID = e.IDPUserID + wm.IDPUserName = e.IDPUserName wm.IDPAccessToken = e.IDPAccessToken wm.IDPIDToken = e.IDPIDToken wm.State = domain.IDPIntentStateSucceeded diff --git a/internal/command/idp_intent_test.go b/internal/command/idp_intent_test.go index a0be850fbc..3eb64cb59d 100644 --- a/internal/command/idp_intent_test.go +++ b/internal/command/idp_intent_test.go @@ -439,7 +439,9 @@ func TestCommands_SucceedIDPIntent(t *testing.T) { event, _ := idpintent.NewSucceededEvent( context.Background(), &idpintent.NewAggregate("id", "ro").Aggregate, - []byte(`{"RawInfo":{"id":"id"}}`), + []byte(`{"sub":"id","preferred_username":"username"}`), + "id", + "username", "", &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, @@ -447,7 +449,7 @@ func TestCommands_SucceedIDPIntent(t *testing.T) { KeyID: "id", Crypted: []byte("accessToken"), }, - "", + "idToken", ) return event }(), @@ -458,18 +460,20 @@ func TestCommands_SucceedIDPIntent(t *testing.T) { args{ ctx: context.Background(), writeModel: NewIDPIntentWriteModel("id", "ro"), - idpSession: &oauth.Session{ + idpSession: &openid.Session{ Tokens: &oidc.Tokens[*oidc.IDTokenClaims]{ Token: &oauth2.Token{ AccessToken: "accessToken", }, + IDToken: "idToken", }, }, - idpUser: &oauth.UserMapper{ - RawInfo: map[string]interface{}{ - "id": "id", + idpUser: openid.NewUser(&oidc.UserInfo{ + Subject: "id", + UserInfoProfile: oidc.UserInfoProfile{ + PreferredUsername: "username", }, - }, + }), }, res{ token: "aWQ", diff --git a/internal/repository/idpintent/intent.go b/internal/repository/idpintent/intent.go index eea0bf68f5..52036b8a7f 100644 --- a/internal/repository/idpintent/intent.go +++ b/internal/repository/idpintent/intent.go @@ -69,6 +69,8 @@ type SucceededEvent struct { eventstore.BaseEvent `json:"-"` IDPUser []byte `json:"idpUser"` + IDPUserID string `json:"idpUserId,omitempty"` + IDPUserName string `json:"idpUserName,omitempty"` UserID string `json:"userId,omitempty"` IDPAccessToken *crypto.CryptoValue `json:"idpAccessToken,omitempty"` IDPIDToken string `json:"idpIdToken,omitempty"` @@ -78,6 +80,8 @@ func NewSucceededEvent( ctx context.Context, aggregate *eventstore.Aggregate, idpUser []byte, + idpUserID, + idpUserName, userID string, idpAccessToken *crypto.CryptoValue, idpIDToken string, @@ -89,6 +93,8 @@ func NewSucceededEvent( SucceededEventType, ), IDPUser: idpUser, + IDPUserID: idpUserID, + IDPUserName: idpUserName, UserID: userID, IDPAccessToken: idpAccessToken, IDPIDToken: idpIDToken, diff --git a/proto/zitadel/user/v2alpha/idp.proto b/proto/zitadel/user/v2alpha/idp.proto index ed8f389c36..5996df57aa 100644 --- a/proto/zitadel/user/v2alpha/idp.proto +++ b/proto/zitadel/user/v2alpha/idp.proto @@ -5,14 +5,41 @@ package zitadel.user.v2alpha; option go_package = "github.com/zitadel/zitadel/pkg/grpc/user/v2alpha;user"; import "google/api/field_behavior.proto"; +import "google/protobuf/struct.proto"; import "protoc-gen-openapiv2/options/annotations.proto"; import "validate/validate.proto"; message IDPInformation{ oneof access{ - IDPOAuthAccessInformation oauth = 1; + IDPOAuthAccessInformation oauth = 1 [ + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + description: "OAuth/OIDC access (and id_token) returned by the identity provider" + } + ]; } - bytes idp_information = 2; + string idp_id = 2 [ + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + description: "ID of the identity provider" + example: "\"d654e6ba-70a3-48ef-a95d-37c8d8a7901a\""; + } + ]; + string user_id = 3 [ + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + description: "ID of the user of the identity provider" + example: "\"6516849804890468048461403518\""; + } + ]; + string user_name = 4 [ + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + description: "username of the user of the identity provider" + example: "\"user@external.com\""; + } + ]; + google.protobuf.Struct raw_information = 5 [ + (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { + description: "complete information returned by the identity provider" + } + ]; } message IDPOAuthAccessInformation{ @@ -30,22 +57,22 @@ message IDPLink { example: "\"d654e6ba-70a3-48ef-a95d-37c8d8a7901a\""; } ]; - string idp_external_id = 2 [ + string user_id = 2 [ (validate.rules).string = {min_len: 1, max_len: 200}, (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { - description: "ID of user of the identity provider" + description: "ID of the user of the identity provider" min_length: 1; max_length: 200; - example: "\"d654e6ba-70a3-48ef-a95d-37c8d8a7901a\""; + example: "\"6516849804890468048461403518\""; } ]; - string display_name = 3 [ + string user_name = 3 [ (validate.rules).string = {min_len: 1, max_len: 200}, (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { - description: "Display name of user of the identity provider" + description: "username of the user of the identity provider" min_length: 1; max_length: 200; - example: "\"Firstname Lastname\""; + example: "\"user@external.com\""; } ]; }