Skip to content

drop usage of userID for names of UserSignup #501

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

Merged
merged 1 commit into from
Jan 31, 2025
Merged

drop usage of userID for names of UserSignup #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 5 additions & 6 deletions pkg/application/service/services.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading