Skip to content

Commit 9ac754d

Browse files
authored
MCS use the correct region to authenticate users (#94)
Previous mcs was authenticating all the users agains <empty> region, this was a problem when an admin configure a different region via the configuration page on mcs, now before authenticating a user via credentials or idp mcs will ask minio what's the current region and try to authenticate using that that information. - Login to mcs - Go to the configuration page and change the region, ie: us-west-1 - Logout from mcs - Login to mcs again, you should not get any error
1 parent 646318e commit 9ac754d

File tree

2 files changed

+137
-11
lines changed

2 files changed

+137
-11
lines changed

restapi/user_login.go

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ import (
3131
"github.com/minio/mcs/restapi/operations/user_api"
3232
)
3333

34+
var (
35+
errorGeneric = errors.New("an error occurred, please try again")
36+
)
37+
3438
func registerLoginHandlers(api *operations.McsAPI) {
3539
// get login strategy
3640
api.UserAPILoginDetailHandler = user_api.LoginDetailHandlerFunc(func(params user_api.LoginDetailParams) middleware.Responder {
@@ -74,9 +78,32 @@ func login(credentials MCSCredentials) (*string, error) {
7478
return &jwt, nil
7579
}
7680

81+
func getConfiguredRegion(client MinioAdmin) string {
82+
location := ""
83+
configuration, err := getConfig(client, "region")
84+
if err != nil {
85+
log.Println("error obtaining MinIO region:", err)
86+
return location
87+
}
88+
// region is an array of 1 element
89+
if len(configuration) > 0 {
90+
location = configuration[0].Value
91+
}
92+
return location
93+
}
94+
7795
// getLoginResponse performs login() and serializes it to the handler's output
7896
func getLoginResponse(lr *models.LoginRequest) (*models.LoginResponse, error) {
79-
creds, err := newMcsCredentials(*lr.AccessKey, *lr.SecretKey, "")
97+
mAdmin, err := newSuperMAdminClient()
98+
if err != nil {
99+
log.Println("error creating Madmin Client:", err)
100+
return nil, errorGeneric
101+
}
102+
adminClient := adminClient{client: mAdmin}
103+
// obtain the configured MinIO region
104+
// need it for user authentication
105+
location := getConfiguredRegion(adminClient)
106+
creds, err := newMcsCredentials(*lr.AccessKey, *lr.SecretKey, location)
80107
if err != nil {
81108
log.Println("error login:", err)
82109
return nil, err
@@ -131,27 +158,32 @@ func getLoginOauth2AuthResponse(lr *models.LoginOauth2AuthRequest) (*models.Logi
131158
// initialize new oauth2 client
132159
oauth2Client, err := oauth2.NewOauth2ProviderClient(ctx, nil)
133160
if err != nil {
134-
return nil, err
161+
log.Println("error getting new oauth2 client:", err)
162+
return nil, errorGeneric
135163
}
136164
// initialize new identity provider
137165
identityProvider := &auth.IdentityProvider{Client: oauth2Client}
138166
// Validate user against IDP
139167
identity, err := loginOauth2Auth(ctx, identityProvider, *lr.Code, *lr.State)
140168
if err != nil {
141-
return nil, err
169+
log.Println("error validating user identity against idp:", err)
170+
return nil, errorGeneric
142171
}
143172
mAdmin, err := newSuperMAdminClient()
144173
if err != nil {
145174
log.Println("error creating Madmin Client:", err)
146-
return nil, err
175+
return nil, errorGeneric
147176
}
148177
adminClient := adminClient{client: mAdmin}
149178
accessKey := identity.Email
150179
secretKey := utils.RandomCharString(32)
151-
// Create user in MinIO
180+
// obtain the configured MinIO region
181+
// need it for user authentication
182+
location := getConfiguredRegion(adminClient)
183+
// create user in MinIO
152184
if _, err := addUser(ctx, adminClient, &accessKey, &secretKey, []string{}); err != nil {
153185
log.Println("error adding user:", err)
154-
return nil, err
186+
return nil, errorGeneric
155187
}
156188
// rollback user if there's an error after this point
157189
defer func() {
@@ -164,25 +196,25 @@ func getLoginOauth2AuthResponse(lr *models.LoginOauth2AuthRequest) (*models.Logi
164196
// assign the "mcsAdmin" policy to this user
165197
if err := setPolicy(ctx, adminClient, oauth2.GetIDPPolicyForUser(), accessKey, models.PolicyEntityUser); err != nil {
166198
log.Println("error setting policy:", err)
167-
return nil, err
199+
return nil, errorGeneric
168200
}
169201
// User was created correctly, create a new session/JWT
170-
creds, err := newMcsCredentials(accessKey, secretKey, "")
202+
creds, err := newMcsCredentials(accessKey, secretKey, location)
171203
if err != nil {
172204
log.Println("error login:", err)
173-
return nil, err
205+
return nil, errorGeneric
174206
}
175207
credentials := mcsCredentials{minioCredentials: creds}
176208
jwt, err := login(credentials)
177209
if err != nil {
178210
log.Println("error login:", err)
179-
return nil, err
211+
return nil, errorGeneric
180212
}
181213
// serialize output
182214
loginResponse := &models.LoginResponse{
183215
SessionID: *jwt,
184216
}
185217
return loginResponse, nil
186218
}
187-
return nil, errors.New("an error occurred, please try again")
219+
return nil, errorGeneric
188220
}

restapi/user_login_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ import (
2424
"github.com/minio/mcs/pkg/auth"
2525
"github.com/minio/mcs/pkg/auth/idp/oauth2"
2626
"github.com/minio/minio-go/v6/pkg/credentials"
27+
"github.com/minio/minio/cmd/config"
28+
"github.com/minio/minio/pkg/madmin"
2729
"github.com/stretchr/testify/assert"
2830
)
2931

@@ -101,3 +103,95 @@ func TestLoginOauth2Auth(t *testing.T) {
101103
funcAssert.Equal("error", err.Error())
102104
}
103105
}
106+
107+
func Test_getConfiguredRegion(t *testing.T) {
108+
client := adminClientMock{}
109+
type args struct {
110+
client adminClientMock
111+
}
112+
113+
tests := []struct {
114+
name string
115+
args args
116+
want string
117+
mock func()
118+
}{
119+
// If MinIO returns an error, we return empty region name
120+
{
121+
name: "region",
122+
args: args{
123+
client: client,
124+
},
125+
want: "",
126+
mock: func() {
127+
// mock function response from getConfig()
128+
minioGetConfigKVMock = func(key string) ([]byte, error) {
129+
return nil, errors.New("Invalid config")
130+
}
131+
// mock function response from listConfig()
132+
minioHelpConfigKVMock = func(subSys, key string, envOnly bool) (madmin.Help, error) {
133+
return madmin.Help{}, errors.New("no help")
134+
}
135+
},
136+
},
137+
// MinIO returns an empty region name
138+
{
139+
name: "region",
140+
args: args{
141+
client: client,
142+
},
143+
want: "",
144+
mock: func() {
145+
// mock function response from getConfig()
146+
minioGetConfigKVMock = func(key string) ([]byte, error) {
147+
return []byte("region name= "), nil
148+
}
149+
// mock function response from listConfig()
150+
minioHelpConfigKVMock = func(subSys, key string, envOnly bool) (madmin.Help, error) {
151+
return madmin.Help{
152+
SubSys: config.RegionSubSys,
153+
Description: "label the location of the server",
154+
MultipleTargets: false,
155+
KeysHelp: []madmin.HelpKV{
156+
{
157+
Key: "name",
158+
Description: "name of the location of the server e.g. \"us-west-rack2\"",
159+
Optional: true,
160+
Type: "string",
161+
MultipleTargets: false,
162+
},
163+
{
164+
Key: "comment",
165+
Description: "optionally add a comment to this setting",
166+
Optional: true,
167+
Type: "sentence",
168+
MultipleTargets: false,
169+
},
170+
},
171+
}, nil
172+
}
173+
},
174+
},
175+
// MinIO returns the asia region
176+
{
177+
name: "region",
178+
args: args{
179+
client: client,
180+
},
181+
want: "asia",
182+
mock: func() {
183+
minioGetConfigKVMock = func(key string) ([]byte, error) {
184+
return []byte("region name=asia "), nil
185+
}
186+
},
187+
},
188+
}
189+
for _, tt := range tests {
190+
tt.mock()
191+
t.Run(tt.name, func(t *testing.T) {
192+
if got := getConfiguredRegion(tt.args.client); got != tt.want {
193+
t.Errorf("getConfiguredRegion() = %v, want %v", got, tt.want)
194+
}
195+
})
196+
}
197+
}

0 commit comments

Comments
 (0)