From 30edab9f693216426b2fa1aea7ad1ded114c4bd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Franke?= Date: Mon, 17 Apr 2023 17:03:05 +0200 Subject: [PATCH 1/3] Search users by username instead of by email As some setups don't have email addresses setup or reuse email addresses, the keycloak search has to be done by username as that is guaranteed to always be unique and defined. This PR changes that. --- ocis-pkg/keycloak/client.go | 30 +++++--- ocis-pkg/keycloak/types.go | 4 +- services/graph/pkg/service/v0/personaldata.go | 2 +- services/invitations/pkg/mocks/client.go | 68 +++++++++++++++++-- 4 files changed, 86 insertions(+), 18 deletions(-) diff --git a/ocis-pkg/keycloak/client.go b/ocis-pkg/keycloak/client.go index f88f2b142..e01faa2ce 100644 --- a/ocis-pkg/keycloak/client.go +++ b/ocis-pkg/keycloak/client.go @@ -90,34 +90,46 @@ func (c *ConcreteClient) SendActionsMail(ctx context.Context, realm, userID stri return c.keycloak.ExecuteActionsEmail(ctx, token.AccessToken, realm, params) } -// GetUserByEmail looks up a user by email. -func (c *ConcreteClient) GetUserByEmail(ctx context.Context, realm, mail string) (*libregraph.User, error) { +// GetUserByParams looks up a user by the given parameters. +func (c *ConcreteClient) GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) { token, err := c.getToken(ctx) if err != nil { return nil, err } - users, err := c.keycloak.GetUsers(ctx, token.AccessToken, realm, gocloak.GetUsersParams{ - Email: &mail, - }) + users, err := c.keycloak.GetUsers(ctx, token.AccessToken, realm, params) if err != nil { return nil, err } if len(users) == 0 { - return nil, fmt.Errorf("no users found with mail address %s", mail) + return nil, fmt.Errorf("no users found") } if len(users) > 1 { - return nil, fmt.Errorf("%d users found with mail address %s, expected 1", len(users), mail) + return nil, fmt.Errorf("%d users found", len(users)) } return c.keycloakUserToLibregraph(users[0]), nil } +// GetUserByEmail looks up a user by email. +func (c *ConcreteClient) GetUserByEmail(ctx context.Context, realm, mail string) (*libregraph.User, error) { + return c.GetUserByParams(ctx, realm, gocloak.GetUsersParams{ + Email: &mail, + }) +} + +// GetUserByUsername looks up a user by username. +func (c *ConcreteClient) GetUserByUsername(ctx context.Context, realm, username string) (*libregraph.User, error) { + return c.GetUserByParams(ctx, realm, gocloak.GetUsersParams{ + Username: &username, + }) +} + // GetPIIReport returns a structure with all the PII for the user. -func (c *ConcreteClient) GetPIIReport(ctx context.Context, realm string, email string) (*PIIReport, error) { - u, err := c.GetUserByEmail(ctx, realm, email) +func (c *ConcreteClient) GetPIIReport(ctx context.Context, realm, username string) (*PIIReport, error) { + u, err := c.GetUserByUsername(ctx, realm, username) if err != nil { return nil, err } diff --git a/ocis-pkg/keycloak/types.go b/ocis-pkg/keycloak/types.go index 901893823..f51a583bd 100644 --- a/ocis-pkg/keycloak/types.go +++ b/ocis-pkg/keycloak/types.go @@ -34,6 +34,8 @@ type PIIReport struct { type Client interface { CreateUser(ctx context.Context, realm string, user *libregraph.User, userActions []UserAction) (string, error) SendActionsMail(ctx context.Context, realm, userID string, userActions []UserAction) error + GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) GetUserByEmail(ctx context.Context, realm, email string) (*libregraph.User, error) - GetPIIReport(ctx context.Context, realm string, email string) (*PIIReport, error) + GetUserByUsername(ctx context.Context, realm, username string) (*libregraph.User, error) + GetPIIReport(ctx context.Context, realm, username string) (*PIIReport, error) } diff --git a/services/graph/pkg/service/v0/personaldata.go b/services/graph/pkg/service/v0/personaldata.go index 2017c6031..474e00f78 100644 --- a/services/graph/pkg/service/v0/personaldata.go +++ b/services/graph/pkg/service/v0/personaldata.go @@ -90,7 +90,7 @@ func (g Graph) GatherPersonalData(usr *user.User, ref *provider.Reference, token // Check if we have a keycloak client, and if so, get the keycloak export. if ctx != nil && g.keycloakClient != nil { - kcd, err := g.keycloakClient.GetPIIReport(ctx, g.config.Keycloak.UserRealm, usr.GetMail()) + kcd, err := g.keycloakClient.GetPIIReport(ctx, g.config.Keycloak.UserRealm, usr.GetUsername()) if err != nil { g.logger.Error().Err(err).Str("userID", usr.GetId().GetOpaqueId()).Msg("cannot get keycloak personal data") } diff --git a/services/invitations/pkg/mocks/client.go b/services/invitations/pkg/mocks/client.go index 42efdfbf0..82c9c811e 100644 --- a/services/invitations/pkg/mocks/client.go +++ b/services/invitations/pkg/mocks/client.go @@ -5,9 +5,11 @@ package mocks import ( context "context" - libregraph "github.com/owncloud/libre-graph-api-go" + gocloak "github.com/Nerzal/gocloak/v13" keycloak "github.com/owncloud/ocis/v2/ocis-pkg/keycloak" + libregraph "github.com/owncloud/libre-graph-api-go" + mock "github.com/stretchr/testify/mock" ) @@ -40,17 +42,17 @@ func (_m *Client) CreateUser(ctx context.Context, realm string, user *libregraph return r0, r1 } -// GetPIIReport provides a mock function with given fields: ctx, realm, email -func (_m *Client) GetPIIReport(ctx context.Context, realm string, email string) (*keycloak.PIIReport, error) { - ret := _m.Called(ctx, realm, email) +// GetPIIReport provides a mock function with given fields: ctx, realm, username +func (_m *Client) GetPIIReport(ctx context.Context, realm string, username string) (*keycloak.PIIReport, error) { + ret := _m.Called(ctx, realm, username) var r0 *keycloak.PIIReport var r1 error if rf, ok := ret.Get(0).(func(context.Context, string, string) (*keycloak.PIIReport, error)); ok { - return rf(ctx, realm, email) + return rf(ctx, realm, username) } if rf, ok := ret.Get(0).(func(context.Context, string, string) *keycloak.PIIReport); ok { - r0 = rf(ctx, realm, email) + r0 = rf(ctx, realm, username) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*keycloak.PIIReport) @@ -58,7 +60,7 @@ func (_m *Client) GetPIIReport(ctx context.Context, realm string, email string) } if rf, ok := ret.Get(1).(func(context.Context, string, string) error); ok { - r1 = rf(ctx, realm, email) + r1 = rf(ctx, realm, username) } else { r1 = ret.Error(1) } @@ -92,6 +94,58 @@ func (_m *Client) GetUserByEmail(ctx context.Context, realm string, email string return r0, r1 } +// GetUserByParams provides a mock function with given fields: ctx, realm, params +func (_m *Client) GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) { + ret := _m.Called(ctx, realm, params) + + var r0 *libregraph.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string, gocloak.GetUsersParams) (*libregraph.User, error)); ok { + return rf(ctx, realm, params) + } + if rf, ok := ret.Get(0).(func(context.Context, string, gocloak.GetUsersParams) *libregraph.User); ok { + r0 = rf(ctx, realm, params) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*libregraph.User) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, string, gocloak.GetUsersParams) error); ok { + r1 = rf(ctx, realm, params) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetUserByUsername provides a mock function with given fields: ctx, realm, username +func (_m *Client) GetUserByUsername(ctx context.Context, realm string, username string) (*libregraph.User, error) { + ret := _m.Called(ctx, realm, username) + + var r0 *libregraph.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string, string) (*libregraph.User, error)); ok { + return rf(ctx, realm, username) + } + if rf, ok := ret.Get(0).(func(context.Context, string, string) *libregraph.User); ok { + r0 = rf(ctx, realm, username) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*libregraph.User) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = rf(ctx, realm, username) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // SendActionsMail provides a mock function with given fields: ctx, realm, userID, userActions func (_m *Client) SendActionsMail(ctx context.Context, realm string, userID string, userActions []keycloak.UserAction) error { ret := _m.Called(ctx, realm, userID, userActions) From 81491efdc7207ab46d45fbcb48a5400bc5a9cec5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Franke?= Date: Wed, 19 Apr 2023 10:19:07 +0200 Subject: [PATCH 2/3] Remove GetUserByEmail method. --- ocis-pkg/keycloak/client.go | 7 ------- ocis-pkg/keycloak/types.go | 1 - services/invitations/pkg/mocks/client.go | 26 ------------------------ 3 files changed, 34 deletions(-) diff --git a/ocis-pkg/keycloak/client.go b/ocis-pkg/keycloak/client.go index e01faa2ce..d5c9d5de1 100644 --- a/ocis-pkg/keycloak/client.go +++ b/ocis-pkg/keycloak/client.go @@ -113,13 +113,6 @@ func (c *ConcreteClient) GetUserByParams(ctx context.Context, realm string, para return c.keycloakUserToLibregraph(users[0]), nil } -// GetUserByEmail looks up a user by email. -func (c *ConcreteClient) GetUserByEmail(ctx context.Context, realm, mail string) (*libregraph.User, error) { - return c.GetUserByParams(ctx, realm, gocloak.GetUsersParams{ - Email: &mail, - }) -} - // GetUserByUsername looks up a user by username. func (c *ConcreteClient) GetUserByUsername(ctx context.Context, realm, username string) (*libregraph.User, error) { return c.GetUserByParams(ctx, realm, gocloak.GetUsersParams{ diff --git a/ocis-pkg/keycloak/types.go b/ocis-pkg/keycloak/types.go index f51a583bd..ce3862bb7 100644 --- a/ocis-pkg/keycloak/types.go +++ b/ocis-pkg/keycloak/types.go @@ -35,7 +35,6 @@ type Client interface { CreateUser(ctx context.Context, realm string, user *libregraph.User, userActions []UserAction) (string, error) SendActionsMail(ctx context.Context, realm, userID string, userActions []UserAction) error GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) - GetUserByEmail(ctx context.Context, realm, email string) (*libregraph.User, error) GetUserByUsername(ctx context.Context, realm, username string) (*libregraph.User, error) GetPIIReport(ctx context.Context, realm, username string) (*PIIReport, error) } diff --git a/services/invitations/pkg/mocks/client.go b/services/invitations/pkg/mocks/client.go index 82c9c811e..a5580a39e 100644 --- a/services/invitations/pkg/mocks/client.go +++ b/services/invitations/pkg/mocks/client.go @@ -68,32 +68,6 @@ func (_m *Client) GetPIIReport(ctx context.Context, realm string, username strin return r0, r1 } -// GetUserByEmail provides a mock function with given fields: ctx, realm, email -func (_m *Client) GetUserByEmail(ctx context.Context, realm string, email string) (*libregraph.User, error) { - ret := _m.Called(ctx, realm, email) - - var r0 *libregraph.User - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string, string) (*libregraph.User, error)); ok { - return rf(ctx, realm, email) - } - if rf, ok := ret.Get(0).(func(context.Context, string, string) *libregraph.User); ok { - r0 = rf(ctx, realm, email) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*libregraph.User) - } - } - - if rf, ok := ret.Get(1).(func(context.Context, string, string) error); ok { - r1 = rf(ctx, realm, email) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // GetUserByParams provides a mock function with given fields: ctx, realm, params func (_m *Client) GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) { ret := _m.Called(ctx, realm, params) From a52ebd1f79810a0547f1629f2b04b19ffc945cd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Franke?= Date: Wed, 19 Apr 2023 10:46:50 +0200 Subject: [PATCH 3/3] Make `getUserByParams` private. --- ocis-pkg/keycloak/client.go | 6 ++--- ocis-pkg/keycloak/types.go | 1 - services/invitations/pkg/mocks/client.go | 30 +----------------------- 3 files changed, 4 insertions(+), 33 deletions(-) diff --git a/ocis-pkg/keycloak/client.go b/ocis-pkg/keycloak/client.go index d5c9d5de1..8b191d5dc 100644 --- a/ocis-pkg/keycloak/client.go +++ b/ocis-pkg/keycloak/client.go @@ -90,8 +90,8 @@ func (c *ConcreteClient) SendActionsMail(ctx context.Context, realm, userID stri return c.keycloak.ExecuteActionsEmail(ctx, token.AccessToken, realm, params) } -// GetUserByParams looks up a user by the given parameters. -func (c *ConcreteClient) GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) { +// getUserByParams looks up a user by the given parameters. +func (c *ConcreteClient) getUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) { token, err := c.getToken(ctx) if err != nil { return nil, err @@ -115,7 +115,7 @@ func (c *ConcreteClient) GetUserByParams(ctx context.Context, realm string, para // GetUserByUsername looks up a user by username. func (c *ConcreteClient) GetUserByUsername(ctx context.Context, realm, username string) (*libregraph.User, error) { - return c.GetUserByParams(ctx, realm, gocloak.GetUsersParams{ + return c.getUserByParams(ctx, realm, gocloak.GetUsersParams{ Username: &username, }) } diff --git a/ocis-pkg/keycloak/types.go b/ocis-pkg/keycloak/types.go index ce3862bb7..632d0975c 100644 --- a/ocis-pkg/keycloak/types.go +++ b/ocis-pkg/keycloak/types.go @@ -34,7 +34,6 @@ type PIIReport struct { type Client interface { CreateUser(ctx context.Context, realm string, user *libregraph.User, userActions []UserAction) (string, error) SendActionsMail(ctx context.Context, realm, userID string, userActions []UserAction) error - GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) GetUserByUsername(ctx context.Context, realm, username string) (*libregraph.User, error) GetPIIReport(ctx context.Context, realm, username string) (*PIIReport, error) } diff --git a/services/invitations/pkg/mocks/client.go b/services/invitations/pkg/mocks/client.go index a5580a39e..a83ddafe4 100644 --- a/services/invitations/pkg/mocks/client.go +++ b/services/invitations/pkg/mocks/client.go @@ -5,10 +5,8 @@ package mocks import ( context "context" - gocloak "github.com/Nerzal/gocloak/v13" - keycloak "github.com/owncloud/ocis/v2/ocis-pkg/keycloak" - libregraph "github.com/owncloud/libre-graph-api-go" + keycloak "github.com/owncloud/ocis/v2/ocis-pkg/keycloak" mock "github.com/stretchr/testify/mock" ) @@ -68,32 +66,6 @@ func (_m *Client) GetPIIReport(ctx context.Context, realm string, username strin return r0, r1 } -// GetUserByParams provides a mock function with given fields: ctx, realm, params -func (_m *Client) GetUserByParams(ctx context.Context, realm string, params gocloak.GetUsersParams) (*libregraph.User, error) { - ret := _m.Called(ctx, realm, params) - - var r0 *libregraph.User - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string, gocloak.GetUsersParams) (*libregraph.User, error)); ok { - return rf(ctx, realm, params) - } - if rf, ok := ret.Get(0).(func(context.Context, string, gocloak.GetUsersParams) *libregraph.User); ok { - r0 = rf(ctx, realm, params) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*libregraph.User) - } - } - - if rf, ok := ret.Get(1).(func(context.Context, string, gocloak.GetUsersParams) error); ok { - r1 = rf(ctx, realm, params) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // GetUserByUsername provides a mock function with given fields: ctx, realm, username func (_m *Client) GetUserByUsername(ctx context.Context, realm string, username string) (*libregraph.User, error) { ret := _m.Called(ctx, realm, username)