Skip to content

Commit

Permalink
drop usage of userID in names of UserSignups #501 (#501)
Browse files Browse the repository at this point in the history
  • Loading branch information
@MatousJobanek
MatousJobanek authored Jan 31, 2025
1 parent c7f9087 commit 280e4ec
Show file tree
Hide file tree
Showing 15 changed files with 200 additions and 259 deletions.
11 changes: 5 additions & 6 deletions pkg/application/service/services.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,14 @@ import (

type SignupService interface {
Signup(ctx *gin.Context) (*toolchainv1alpha1.UserSignup, error)
GetSignup(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error)
GetUserSignupFromIdentifier(userID, username string) (*toolchainv1alpha1.UserSignup, error)
PhoneNumberAlreadyInUse(userID, username, phoneNumberOrHash string) error
GetSignup(ctx *gin.Context, username string, checkUserSignupCompleted bool) (*signup.Signup, error)
PhoneNumberAlreadyInUse(username, phoneNumberOrHash string) error
}

type VerificationService interface {
InitVerification(ctx *gin.Context, userID, username, e164PhoneNumber, countryCode string) error
VerifyPhoneCode(ctx *gin.Context, userID, username, code string) error
VerifyActivationCode(ctx *gin.Context, userID, username, code string) error
InitVerification(ctx *gin.Context, username, e164PhoneNumber, countryCode string) error
VerifyPhoneCode(ctx *gin.Context, username, code string) error
VerifyActivationCode(ctx *gin.Context, username, code string) error
}

type Services interface {
Expand Down
20 changes: 8 additions & 12 deletions pkg/controller/signup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ func (s *Signup) PostHandler(ctx *gin.Context) {
// invokes the Verification service with an E.164 formatted phone number value derived from the country code and phone number
// provided by the user.
func (s *Signup) InitVerificationHandler(ctx *gin.Context) {
userID := ctx.GetString(context.SubKey)
username := ctx.GetString(context.UsernameKey)

// Read the Body content
Expand All @@ -87,9 +86,9 @@ func (s *Signup) InitVerificationHandler(ctx *gin.Context) {
}

e164Number := phonenumbers.Format(number, phonenumbers.E164)
err = s.app.VerificationService().InitVerification(ctx, userID, username, e164Number, strconv.Itoa(countryCode))
err = s.app.VerificationService().InitVerification(ctx, username, e164Number, strconv.Itoa(countryCode))
if err != nil {
log.Errorf(ctx, err, "Verification for %s could not be sent", userID)
log.Errorf(ctx, err, "Verification for %s could not be sent", username)
e := &crterrors.Error{}
switch {
case errors.As(err, &e):
Expand All @@ -100,24 +99,23 @@ func (s *Signup) InitVerificationHandler(ctx *gin.Context) {
return
}

log.Infof(ctx, "phone verification has been sent for userID %s", userID)
log.Infof(ctx, "phone verification has been sent for username %s", username)
ctx.Status(http.StatusNoContent)
ctx.Writer.WriteHeaderNow()
}

// GetHandler returns the Signup resource
func (s *Signup) GetHandler(ctx *gin.Context) {

// Get the UserSignup resource from the service by the userID
userID := ctx.GetString(context.SubKey)
// Get the UserSignup resource from the service by the username
username := ctx.GetString(context.UsernameKey)
signupResource, err := s.app.SignupService().GetSignup(ctx, userID, username, true)
signupResource, err := s.app.SignupService().GetSignup(ctx, username, true)
if err != nil {
log.Error(ctx, err, "error getting UserSignup resource")
crterrors.AbortWithError(ctx, http.StatusInternalServerError, err, "error getting UserSignup resource")
}
if signupResource == nil {
log.Infof(ctx, "UserSignup resource for userID: %s, username: %s resource not found", userID, username)
log.Infof(ctx, "UserSignup resource for username '%s' resource not found", username)
ctx.AbortWithStatus(http.StatusNotFound)
} else {
ctx.JSON(http.StatusOK, signupResource)
Expand All @@ -134,10 +132,9 @@ func (s *Signup) VerifyPhoneCodeHandler(ctx *gin.Context) {
return
}

userID := ctx.GetString(context.SubKey)
username := ctx.GetString(context.UsernameKey)

err := s.app.VerificationService().VerifyPhoneCode(ctx, userID, username, code)
err := s.app.VerificationService().VerifyPhoneCode(ctx, username, code)
if err != nil {
e := &crterrors.Error{}
switch {
Expand Down Expand Up @@ -167,10 +164,9 @@ func (s *Signup) VerifyActivationCodeHandler(ctx *gin.Context) {
return
}

userID := ctx.GetString(context.SubKey)
username := ctx.GetString(context.UsernameKey)

err := s.app.VerificationService().VerifyActivationCode(ctx, userID, username, code)
err := s.app.VerificationService().VerifyActivationCode(ctx, username, code)
if err != nil {
log.Error(ctx, err, "error validating activation code")
e := &crterrors.Error{}
Expand Down
27 changes: 11 additions & 16 deletions pkg/controller/signup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() {
Reason: "Provisioning",
},
}
svc.MockGetSignup = func(_ *gin.Context, _, name string, _ bool) (*signup.Signup, error) {
svc.MockGetSignup = func(_ *gin.Context, name string, _ bool) (*signup.Signup, error) {
if name == username {
return expected, nil
}
Expand All @@ -220,7 +220,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() {
ctx.Request = req
ctx.Set(context.UsernameKey, username)

svc.MockGetSignup = func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) {
svc.MockGetSignup = func(_ *gin.Context, _ string, _ bool) (*signup.Signup, error) {
return nil, nil
}

Expand All @@ -237,7 +237,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() {
ctx.Request = req
ctx.Set(context.UsernameKey, username)

svc.MockGetSignup = func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) {
svc.MockGetSignup = func(_ *gin.Context, _ string, _ bool) (*signup.Signup, error) {
return nil, errors.New("oopsie woopsie")
}

Expand Down Expand Up @@ -451,7 +451,7 @@ func (s *TestSignupSuite) TestVerifyPhoneCodeHandler() {

require.Equal(s.T(), "Internal Server Error", bodyParams["status"])
require.InDelta(s.T(), float64(500), bodyParams["code"], 0.01)
require.Equal(s.T(), "no user: error retrieving usersignup: ", bodyParams["message"])
require.Equal(s.T(), "no user: error retrieving usersignup with username 'johnny@kubesaw'", bodyParams["message"])
require.Equal(s.T(), "error while verifying phone code", bodyParams["details"])
})

Expand Down Expand Up @@ -724,24 +724,19 @@ func initActivationCodeVerification(t *testing.T, handler gin.HandlerFunc, usern
}

type FakeSignupService struct {
MockGetSignup func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error)
MockSignup func(ctx *gin.Context) (*crtapi.UserSignup, error)
MockGetUserSignupFromIdentifier func(userID, username string) (*crtapi.UserSignup, error)
MockPhoneNumberAlreadyInUse func(userID, username, value string) error
MockGetSignup func(ctx *gin.Context, username string, checkUserSignupComplete bool) (*signup.Signup, error)
MockSignup func(ctx *gin.Context) (*crtapi.UserSignup, error)
MockPhoneNumberAlreadyInUse func(username, value string) error
}

func (m *FakeSignupService) GetSignup(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) {
return m.MockGetSignup(ctx, userID, username, checkUserSignupComplete)
func (m *FakeSignupService) GetSignup(ctx *gin.Context, username string, checkUserSignupComplete bool) (*signup.Signup, error) {
return m.MockGetSignup(ctx, username, checkUserSignupComplete)
}

func (m *FakeSignupService) Signup(ctx *gin.Context) (*crtapi.UserSignup, error) {
return m.MockSignup(ctx)
}

func (m *FakeSignupService) GetUserSignupFromIdentifier(userID, username string) (*crtapi.UserSignup, error) {
return m.MockGetUserSignupFromIdentifier(userID, username)
}

func (m *FakeSignupService) PhoneNumberAlreadyInUse(userID, username, e164phoneNumber string) error {
return m.MockPhoneNumberAlreadyInUse(userID, username, e164phoneNumber)
func (m *FakeSignupService) PhoneNumberAlreadyInUse(username, e164phoneNumber string) error {
return m.MockPhoneNumberAlreadyInUse(username, e164phoneNumber)
}
4 changes: 2 additions & 2 deletions pkg/proxy/handlers/spacelister.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const (

type SpaceLister struct {
namespaced.Client
GetSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error)
GetSignupFunc func(ctx *gin.Context, username string, checkUserSignupCompleted bool) (*signup.Signup, error)
ProxyMetrics *metrics.ProxyMetrics
}

Expand All @@ -43,7 +43,7 @@ func NewSpaceLister(client namespaced.Client, app application.Application, proxy
func (s *SpaceLister) GetProvisionedUserSignup(ctx echo.Context) (*signup.Signup, error) {
username, _ := ctx.Get(context.UsernameKey).(string)

userSignup, err := s.GetSignupFunc(nil, "", username, false)
userSignup, err := s.GetSignupFunc(nil, username, false)
if err != nil {
ctx.Logger().Error(errs.Wrap(err, "error retrieving signup"))
return nil, err
Expand Down
8 changes: 4 additions & 4 deletions pkg/proxy/handlers/spacelister_get_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) {
expectedErr string
expectedErrCode int
expectedWorkspace string
overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error)
overrideSignupFunc func(ctx *gin.Context, username string, checkUserSignupComplete bool) (*signup.Signup, error)
mockFakeClient func(fakeClient *test.FakeClient)
overrideGetMembersFunc func(conditions ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster
overrideMemberClient *test.FakeClient
Expand Down Expand Up @@ -270,7 +270,7 @@ func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) {
expectedWs: nil,
expectedErr: "signup error",
expectedErrCode: 500,
overrideSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) {
overrideSignupFunc: func(_ *gin.Context, _ string, _ bool) (*signup.Signup, error) {
return nil, fmt.Errorf("signup error")
},
expectedWorkspace: "dancelover",
Expand Down Expand Up @@ -620,7 +620,7 @@ func TestGetUserWorkspace(t *testing.T) {
workspaceRequest string
expectedWorkspace func(t *testing.T, fakeClient *test.FakeClient) toolchainv1alpha1.Workspace
mockFakeClient func(fakeClient *test.FakeClient)
overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error)
overrideSignupFunc func(ctx *gin.Context, username string, checkUserSignupComplete bool) (*signup.Signup, error)
}{
"get robin workspace": {
username: "robin",
Expand Down Expand Up @@ -673,7 +673,7 @@ func TestGetUserWorkspace(t *testing.T) {
username: "batman",
workspaceRequest: "batman",
expectedErr: "signup error",
overrideSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) {
overrideSignupFunc: func(_ *gin.Context, _ string, _ bool) (*signup.Signup, error) {
return nil, fmt.Errorf("signup error")
},
expectedWorkspace: nil,
Expand Down
4 changes: 2 additions & 2 deletions pkg/proxy/handlers/spacelister_list_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func TestHandleSpaceListRequest(t *testing.T) {
expectedErr string
expectedErrCode int
expectedWorkspace string
overrideSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error)
overrideSignupFunc func(ctx *gin.Context, username string, checkUserSignupComplete bool) (*signup.Signup, error)
mockFakeClient func(fakeClient *test.FakeClient)
}{
"dancelover lists spaces": {
Expand Down Expand Up @@ -181,7 +181,7 @@ func TestHandleSpaceListRequest(t *testing.T) {
expectedWs: nil,
expectedErr: "signup error",
expectedErrCode: 500,
overrideSignupFunc: func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) {
overrideSignupFunc: func(_ *gin.Context, _ string, _ bool) (*signup.Signup, error) {
return nil, fmt.Errorf("signup error")
},
},
Expand Down
22 changes: 11 additions & 11 deletions pkg/proxy/members.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,19 @@ func NewMemberClusters(client namespaced.Client, signupService service.SignupSer
return si
}

func (s *MemberClusters) GetClusterAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) {
func (s *MemberClusters) GetClusterAccess(username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) {
// if workspace is not provided then return the default space access
if workspace == "" {
return s.getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName)
return s.getClusterAccessForDefaultWorkspace(username, proxyPluginName)
}

return s.getSpaceAccess(userID, username, workspace, proxyPluginName, publicViewerEnabled)
return s.getSpaceAccess(username, workspace, proxyPluginName, publicViewerEnabled)
}

// getSpaceAccess retrieves space access for an user
func (s *MemberClusters) getSpaceAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) {
func (s *MemberClusters) getSpaceAccess(username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) {
// retrieve the user's complaint name
complaintUserName, err := s.getUserSignupComplaintName(userID, username, publicViewerEnabled)
complaintUserName, err := s.getUserSignupComplaintName(username, publicViewerEnabled)
if err != nil {
return nil, err
}
Expand All @@ -65,14 +65,14 @@ func (s *MemberClusters) getSpaceAccess(userID, username, workspace, proxyPlugin
return s.accessForSpace(space, complaintUserName, proxyPluginName)
}

func (s *MemberClusters) getUserSignupComplaintName(userID, username string, publicViewerEnabled bool) (string, error) {
func (s *MemberClusters) getUserSignupComplaintName(username string, publicViewerEnabled bool) (string, error) {
// if PublicViewer is enabled and the requested user is the PublicViewer, than no lookup is required
if publicViewerEnabled && username == toolchainv1alpha1.KubesawAuthenticatedUsername {
return username, nil
}

// retrieve the UserSignup from cache
userSignup, err := s.getSignupFromInformerForProvisionedUser(userID, username)
userSignup, err := s.getSignupFromInformerForProvisionedUser(username)
if err != nil {
return "", err
}
Expand All @@ -81,9 +81,9 @@ func (s *MemberClusters) getUserSignupComplaintName(userID, username string, pub
}

// getClusterAccessForDefaultWorkspace retrieves the cluster for the user's default workspace
func (s *MemberClusters) getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName string) (*access.ClusterAccess, error) {
func (s *MemberClusters) getClusterAccessForDefaultWorkspace(username, proxyPluginName string) (*access.ClusterAccess, error) {
// retrieve the UserSignup from cache
userSignup, err := s.getSignupFromInformerForProvisionedUser(userID, username)
userSignup, err := s.getSignupFromInformerForProvisionedUser(username)
if err != nil {
return nil, err
}
Expand All @@ -92,10 +92,10 @@ func (s *MemberClusters) getClusterAccessForDefaultWorkspace(userID, username, p
return s.accessForCluster(userSignup.APIEndpoint, userSignup.ClusterName, userSignup.CompliantUsername, proxyPluginName)
}

func (s *MemberClusters) getSignupFromInformerForProvisionedUser(userID, username string) (*signup.Signup, error) {
func (s *MemberClusters) getSignupFromInformerForProvisionedUser(username string) (*signup.Signup, error) {
// don't check for usersignup complete status, since it might cause the proxy blocking the request
// and returning an error when quick transitions from ready to provisioning are happening.
userSignup, err := s.SignupService.GetSignup(nil, userID, username, false)
userSignup, err := s.SignupService.GetSignup(nil, username, false)
if err != nil {
return nil, err
}
Expand Down
Loading

0 comments on commit 280e4ec

Please sign in to comment.