Skip to content

Commit 6ccd0c2

Browse files
hiranya911hiranya911
authored
Minor improvements to user management code (#44)
* some changes to auth * Implemented a reusable HTTP client API * Added test cases * Comment clean up * Simplified the usage by adding HTTPClient * Using the old ctx import * Support for arbitrary entity types in the request * Renamed fields; Added documentation * Removing a redundant else case * initial * more integration tests * set custom - still needs guessing the type for unmarshaling. * tests * server * server * . * json * json * move testdata * get * tests * updated to param struct of pointers to call create and update * Comments * cleanup * cleanup * cleanup * cleanup minor test changes * Changing the iteraator pattern * last page iterator test fix * clean up tests next. * make the fetch tidier * fetch cleanup * cc change * custom claims * newline * adding error propagation to makeExportedUser * remove trivial claims map type * list users test data * rename p ptr, and remove the with... options for the iterator * rename p ptr * some unit tests * adding integration tests for custom claims * NO ERROR * unit tests * comments hkj * addressing comments * delete unneeded json file * phone NUMBER * typo * remove cc from create * refactor param structs * remove package ptr * test refactor * cleanup comments * cleanup debug.test * Adding back the default http client * fix httpClient for tests * creds * creds * fix default http client * cleanupPrintfs * disable * Reanme payload vars * Revert newHTTPKeySource function * add back defaultClient in newClient * reenable testNewHTTPClientNoOpts) * reverting keysource tests) * Take the httpClient from the keysource * Removethe +1 second for the token timestamp. * Adding tests * White spaces * Redesign the error validators * prepare returns an error. * cleanup * dissolve * dissolve * clean tests * split integration tests * addressing comments * addressing comments opt branch/ BEFORE hc change * Removing the defaultClient from the NewClient, and extracting the NewClient creation outside of KeySource * closer from echoServer * cleanup + 500 error unit test * unify error messages * Refactor stop side effect for params preparation * +1 to timestamp for flakiness * removing +1 * disallow defaultClient * whitespaces * http default * add TODO * Code clean up and refactoring
1 parent b4bc3d2 commit 6ccd0c2

File tree

2 files changed

+93
-109
lines changed

2 files changed

+93
-109
lines changed

‎auth/user_mgt.go

Lines changed: 76 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -32,18 +32,15 @@ var (
3232
// Order matters only first error per field is reported.
3333
commonValidators = []struct {
3434
fieldName string
35-
testFun func(map[string]interface{}) error
35+
apply func(map[string]interface{}) error
3636
}{
3737
{"disableUser", validateTrue},
38-
{"displayName", validateNonEmptyDisplayName},
39-
{"email", validateNonEmptyEmail},
38+
{"displayName", validateDisplayName},
4039
{"email", validateEmail},
4140
{"emailVerified", validateTrue},
42-
{"phoneNumber", validateNonEmptyPhoneNumber},
4341
{"phoneNumber", validatePhone},
4442
{"password", validatePassword},
45-
{"photoUrl", validateNonEmptyPhotoURL},
46-
{"localId", validateNonEmptyUID},
43+
{"photoUrl", validatePhotoURL},
4744
{"localId", validateUID},
4845
}
4946
)
@@ -292,21 +289,10 @@ func (p *commonParams) set(key string, value interface{}) {
292289
p.payload[key] = value
293290
}
294291

295-
// assumes that payloadName is a string field in p.payload
296-
func processDeletion(p map[string]interface{}, payloadName string) {
297-
deletionSpecs := map[string]struct {
298-
deleteListName string
299-
deleteFieldName string
300-
}{
301-
"displayName": {"deleteAttribute", "DISPLAY_NAME"},
302-
"phoneNumber": {"deleteProvider", "phone"},
303-
"photoUrl": {"deleteAttribute", "PHOTO_URL"},
304-
}
305-
306-
if dn, ok := p[payloadName]; ok && len(dn.(string)) == 0 {
307-
delSpec := deletionSpecs[payloadName]
308-
addToListParam(p, delSpec.deleteListName, delSpec.deleteFieldName)
309-
delete(p, payloadName)
292+
func processDeletion(p map[string]interface{}, field, listKey, listVal string) {
293+
if dn, ok := p[field]; ok && len(dn.(string)) == 0 {
294+
addToListParam(p, listKey, listVal)
295+
delete(p, field)
310296
}
311297
}
312298

@@ -319,35 +305,35 @@ func addToListParam(p map[string]interface{}, listname, param string) {
319305
}
320306

321307
func processClaims(p map[string]interface{}) error {
322-
if _, ok := p["customClaims"]; !ok {
308+
cc, ok := p["customClaims"]
309+
if !ok {
323310
return nil
324311
}
325-
cc := p["customClaims"]
312+
326313
claims, ok := cc.(map[string]interface{})
327314
if !ok {
328-
return fmt.Errorf("CustomClaims(): unexpected type")
315+
return fmt.Errorf("unexpected type for custom claims")
329316
}
330317
for _, key := range reservedClaims {
331318
if _, ok := claims[key]; ok {
332319
return fmt.Errorf("CustomClaims(%q: ...): claim %q is reserved, and must not be set", key, key)
333320
}
334321
}
322+
335323
b, err := json.Marshal(claims)
336324
if err != nil {
337-
return fmt.Errorf("CustomClaims() Marshaling error: %v", err)
325+
return fmt.Errorf("custom claims marshaling error: %v", err)
338326
}
339327
s := string(b)
340328
if s == "null" {
341329
s = "{}"
342330
}
331+
if len(s) > maxLenPayloadCC {
332+
return fmt.Errorf("serialized custom claims must not exceed %d characters", maxLenPayloadCC)
333+
}
334+
343335
p["customAttributes"] = s
344336
delete(p, "customClaims")
345-
wantLength := maxLenPayloadCC
346-
if val, ok := p["customAttributes"]; ok {
347-
if len(val.(string)) > wantLength {
348-
return fmt.Errorf("stringified JSON of CustomClaims must be a string at most %d characters long", wantLength)
349-
}
350-
}
351337
return nil
352338
}
353339

@@ -358,101 +344,111 @@ func validateTrue(p map[string]interface{}) error {
358344
return nil
359345
}
360346

361-
func validateNonEmptyDisplayName(p map[string]interface{}) error {
362-
return validateNonEmpty(p, "displayName")
363-
}
364-
func validateNonEmptyEmail(p map[string]interface{}) error { return validateNonEmpty(p, "email") }
365-
func validateNonEmptyPhoneNumber(p map[string]interface{}) error {
366-
return validateNonEmpty(p, "phoneNumber")
347+
func validateDisplayName(p map[string]interface{}) error {
348+
val, ok := p["displayName"]
349+
if ok && len(val.(string)) == 0 {
350+
return fmt.Errorf("display name must be a non-empty string")
351+
}
352+
return nil
367353
}
368-
func validateNonEmptyPhotoURL(p map[string]interface{}) error { return validateNonEmpty(p, "photoUrl") }
369-
func validateNonEmptyUID(p map[string]interface{}) error { return validateNonEmpty(p, "localId") }
370354

371-
func validateNonEmpty(p map[string]interface{}, fieldName string) error {
372-
if val, ok := p[fieldName]; ok {
373-
if len(val.(string)) == 0 {
374-
return fmt.Errorf("%s must be a non-empty string", fieldName)
375-
}
355+
func validatePhotoURL(p map[string]interface{}) error {
356+
val, ok := p["photoUrl"]
357+
if ok && len(val.(string)) == 0 {
358+
return fmt.Errorf("photo url must be a non-empty string")
376359
}
377360
return nil
378361
}
379362

380363
func validateEmail(p map[string]interface{}) error {
381-
fieldName := "email"
382-
if val, ok := p[fieldName]; ok {
383-
if parts := strings.Split(val.(string), "@"); len(parts) != 2 || len(parts[0]) == 0 || len(parts[1]) == 0 {
384-
return fmt.Errorf("malformed %s string: %q", fieldName, val)
385-
}
364+
val, ok := p["email"]
365+
if !ok {
366+
return nil
367+
}
368+
email := val.(string)
369+
if email == "" {
370+
return fmt.Errorf("email must not be empty")
371+
}
372+
if parts := strings.Split(email, "@"); len(parts) != 2 || len(parts[0]) == 0 || len(parts[1]) == 0 {
373+
return fmt.Errorf("malformed email string: %q", email)
386374
}
387375
return nil
388376
}
389377

390378
func validatePassword(p map[string]interface{}) error {
391-
wantLength := 6
392-
fieldName := "password"
393-
if val, ok := p[fieldName]; ok {
394-
if len(val.(string)) < wantLength {
395-
return fmt.Errorf("%s must be a string at least %d characters long", fieldName, wantLength)
396-
}
379+
val, ok := p["password"]
380+
if ok && len(val.(string)) < 6 {
381+
return fmt.Errorf("password must be a string at least 6 characters long")
397382
}
398383
return nil
399384
}
400385

401386
func validateUID(p map[string]interface{}) error {
402-
fieldName := "localId"
403-
wantLength := 128
404-
if val, ok := p[fieldName]; ok {
405-
if len(val.(string)) > wantLength {
406-
return fmt.Errorf("%s must be a string at most %d characters long", fieldName, wantLength)
407-
}
387+
val, ok := p["localId"]
388+
if !ok {
389+
return nil
390+
}
391+
uid := val.(string)
392+
if uid == "" {
393+
return fmt.Errorf("uid must not be empty")
394+
}
395+
if len(val.(string)) > 128 {
396+
return fmt.Errorf("uid must be a string at most 128 characters long")
408397
}
409398
return nil
410399
}
411400

412401
func validatePhone(p map[string]interface{}) error {
413-
fieldName := "phoneNumber"
414-
if val, ok := p[fieldName]; ok {
415-
if !regexp.MustCompile(`\+.*[0-9A-Za-z]`).MatchString(val.(string)) {
416-
return fmt.Errorf(
417-
"invalid %s %q. Must be a valid, E.164 compliant identifier", fieldName, val)
418-
}
402+
val, ok := p["phoneNumber"]
403+
if !ok {
404+
return nil
405+
}
406+
phone := val.(string)
407+
if phone == "" {
408+
return fmt.Errorf("phone number must not be empty")
409+
}
410+
if !regexp.MustCompile(`\+.*[0-9A-Za-z]`).MatchString(phone) {
411+
return fmt.Errorf("phone number must be a valid, E.164 compliant identifier")
419412
}
420413
return nil
421414
}
422415

423416
func (u *UserToCreate) preparePayload() (map[string]interface{}, error) {
417+
params := map[string]interface{}{}
424418
if u.payload == nil {
425-
return map[string]interface{}{}, nil
419+
return params, nil
426420
}
427-
params := map[string]interface{}{}
421+
428422
for k, v := range u.payload {
429423
params[k] = v
430424
}
431-
for _, test := range commonValidators {
432-
if err := test.testFun(params); err != nil {
425+
for _, validator := range commonValidators {
426+
if err := validator.apply(params); err != nil {
433427
return nil, err
434428
}
435429
}
436430
return params, nil
437431
}
438432

439433
func (u *UserToUpdate) preparePayload() (map[string]interface{}, error) {
434+
params := map[string]interface{}{}
440435
if u.payload == nil {
441-
return nil, fmt.Errorf("update with no params") // This was caught in the caller not here.
436+
return params, nil
442437
}
443-
params := map[string]interface{}{}
438+
444439
for k, v := range u.payload {
445440
params[k] = v
446441
}
447-
processDeletion(params, "displayName")
448-
processDeletion(params, "phoneNumber")
449-
processDeletion(params, "photoUrl")
442+
processDeletion(params, "displayName", "deleteAttribute", "DISPLAY_NAME")
443+
processDeletion(params, "photoUrl", "deleteAttribute", "PHOTO_URL")
444+
processDeletion(params, "phoneNumber", "deleteProvider", "phone")
445+
450446
if err := processClaims(params); err != nil {
451447
return nil, err
452448
}
453449

454-
for _, test := range commonValidators {
455-
if err := test.testFun(params); err != nil {
450+
for _, validator := range commonValidators {
451+
if err := validator.apply(params); err != nil {
456452
return nil, err
457453
}
458454
}
@@ -501,10 +497,10 @@ func (c *Client) getUser(ctx context.Context, params map[string]interface{}) (*U
501497
return nil, err
502498
}
503499
if len(gur.Users) == 0 {
504-
return nil, fmt.Errorf("cannot find user %v", params)
500+
return nil, fmt.Errorf("cannot find user from params: %v", params)
505501
}
506502
if l := len(gur.Users); l > 1 {
507-
return nil, fmt.Errorf("getUser(%v) got %d users; want: one user, ", params, l)
503+
return nil, fmt.Errorf("getUser(%v) = %d users; want = 1 user, ", params, l)
508504
}
509505
eu, err := makeExportedUser(gur.Users[0])
510506
return eu.UserRecord, err

‎auth/user_mgt_test.go

Lines changed: 17 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
"firebase.google.com/go/internal"
3030
"golang.org/x/net/context"
3131
"google.golang.org/api/iterator"
32-
"google.golang.org/api/option"
3332
)
3433

3534
type mockAuthServer struct {
@@ -81,8 +80,7 @@ func TestGetUser(t *testing.T) {
8180
CustomClaims: map[string]interface{}{"admin": true, "package": "gold"},
8281
}
8382
if !reflect.DeepEqual(user, want) {
84-
t.Errorf("GetUser(UID) = %#v, want: %#v", user, want)
85-
testCompareUserRecords("GetUser(UID)", user, want, t)
83+
t.Errorf("GetUser() = %#v; want = %#v", user, want)
8684
}
8785
}
8886

@@ -155,31 +153,31 @@ func TestCreateUserValidatorsFail(t *testing.T) {
155153
}{
156154
{
157155
(&UserToCreate{}).Password("short"),
158-
`password must be a string at least 6 characters long`,
156+
"password must be a string at least 6 characters long",
159157
}, {
160158
(&UserToCreate{}).PhoneNumber(""),
161-
"phoneNumber must be a non-empty string",
159+
"phone number must not be empty",
162160
}, {
163161
(&UserToCreate{}).PhoneNumber("1234"),
164-
`invalid phoneNumber "1234". Must be a valid, E.164 compliant identifier`,
162+
"phone number must be a valid, E.164 compliant identifier",
165163
}, {
166164
(&UserToCreate{}).PhoneNumber("+_!@#$"),
167-
`invalid phoneNumber "+_!@#$". Must be a valid, E.164 compliant identifier`,
165+
"phone number must be a valid, E.164 compliant identifier",
168166
}, {
169167
(&UserToCreate{}).UID(""),
170-
`localId must be a non-empty string`,
168+
"uid must not be empty",
171169
}, {
172170
(&UserToCreate{}).UID(strings.Repeat("a", 129)),
173-
"localId must be a string at most 128 characters long",
171+
"uid must be a string at most 128 characters long",
174172
}, {
175173
(&UserToCreate{}).DisplayName(""),
176-
`displayName must be a non-empty string`,
174+
"display name must be a non-empty string",
177175
}, {
178176
(&UserToCreate{}).PhotoURL(""),
179-
"photoUrl must be a non-empty string",
177+
"photo url must be a non-empty string",
180178
}, {
181179
(&UserToCreate{}).Email(""),
182-
`email must be a non-empty string`,
180+
"email must not be empty",
183181
}, {
184182
(&UserToCreate{}).Email("a"),
185183
`malformed email string: "a"`,
@@ -229,7 +227,7 @@ func TestCreateUserValidatorsPass(t *testing.T) {
229227
// that's how we know the params passed validation
230228
// the second call to GetUser, tries to get the user with the returned ID above, it fails
231229
// with the following expected error
232-
if err.Error() != "cannot find user map[localId:[expectedUserID]]" {
230+
if err.Error() != "cannot find user from params: map[localId:[expectedUserID]]" {
233231
t.Error(err)
234232
}
235233
}
@@ -248,10 +246,10 @@ func TestUpdateParamsValidatorsFail(t *testing.T) {
248246
"params must not be empty for update",
249247
}, {
250248
(&UserToUpdate{}).PhoneNumber("1"),
251-
`invalid phoneNumber "1". Must be a valid, E.164 compliant identifier`,
249+
"phone number must be a valid, E.164 compliant identifier",
252250
}, {
253251
(&UserToUpdate{}).CustomClaims(map[string]interface{}{"a": strings.Repeat("a", 993)}),
254-
fmt.Sprintf("stringified JSON of CustomClaims must be a string at most %d characters long", maxLenPayloadCC),
252+
"serialized custom claims must not exceed 1000 characters",
255253
},
256254
}
257255

@@ -310,7 +308,7 @@ func TestUpdateUserValidatorsPass(t *testing.T) {
310308
// that's how we know the params passed validation
311309
// the second call to GetUser, tries to get the user with the returned ID above, it fails
312310
// with the following expected error
313-
if err.Error() != "cannot find user map[localId:[expectedUserID]]" {
311+
if err.Error() != "cannot find user from params: map[localId:[expectedUserID]]" {
314312
t.Error(err)
315313
}
316314
}
@@ -322,7 +320,7 @@ func TestBadSetCustomClaims(t *testing.T) {
322320
want string
323321
}{{
324322
map[string]interface{}{"a": strings.Repeat("a", 993)},
325-
fmt.Sprintf("stringified JSON of CustomClaims must be a string at most %d characters long", maxLenPayloadCC),
323+
"serialized custom claims must not exceed 1000 characters",
326324
}}
327325

328326
for _, res := range reservedClaims {
@@ -750,12 +748,7 @@ func echoServer(resp interface{}, t *testing.T) *mockAuthServer {
750748

751749
})
752750
s.Srv = httptest.NewServer(handler)
753-
conf := &internal.AuthConfig{
754-
Opts: []option.ClientOption{
755-
option.WithHTTPClient(s.Srv.Client()),
756-
},
757-
}
758-
authClient, err := NewClient(context.Background(), conf)
751+
authClient, err := NewClient(context.Background(), &internal.AuthConfig{})
759752
if err != nil {
760753
t.Fatal()
761754
}
@@ -776,12 +769,7 @@ func badServer(t *testing.T) *mockAuthServer {
776769
w.Header().Set("Content-Type", "application/json")
777770
w.Write([]byte("{}"))
778771
}))
779-
conf := &internal.AuthConfig{
780-
Opts: []option.ClientOption{
781-
option.WithHTTPClient(s.Srv.Client()),
782-
},
783-
}
784-
authClient, err := NewClient(context.Background(), conf)
772+
authClient, err := NewClient(context.Background(), &internal.AuthConfig{})
785773
if err != nil {
786774
t.Fatal()
787775
}

0 commit comments

Comments
 (0)