Skip to content
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.

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

storage/static.go: storage backend should not explicitly lower-case email ids. #1046

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

rithujohn191
Copy link
Contributor

No description provided.

Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test and add a docstring to the passwordsByEmail field?

p.Email = strings.ToLower(p.Email)
passwordsByEmail[p.Email] = p
//Enable case insensitive email comparison.
lowerEmail = strings.ToLower(p.Email)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:=

@rithujohn191 rithujohn191 force-pushed the static-password-fix branch 3 times, most recently from f103da8 to d0dfe26 Compare August 24, 2017 18:10
@rithujohn191
Copy link
Contributor Author

added a test and documentation

if err := s.CreatePassword(p3); err != nil {
return err
}
return s.CreatePassword(p4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to read this value back an ensure the emails match.

Copy link
Contributor Author

@rithujohn191 rithujohn191 Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test makes sure that "Spam@example.com" cannot be created, so it ensures an error is returned

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want me to check if "spam@example.com" gets created with the all lower case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like

p, err := s.GetPassword(strings.ToLower(p4.Email))

p.Email == p4.Email

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

passwordsByEmail[p.Email] = p
//Enable case insensitive email comparison.
lowerEmail := strings.ToLower(p.Email)
passwordsByEmail[lowerEmail] = p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we log or something if the lowered email as already in the map?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would just return an error here. @rithujohn191 what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I only think we can log this error. Erroring out would be backward incompatible, if sensible :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we don't error out we essentially allow the user to overwrite the map. For the following ConfigMap:

staticPasswords:
- email: "admin@example.com"
  # bcrypt hash of the string "password"
  hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W"
  username: "admin"
  userID: "08a8684b-db88-4b73-90a9-3cd1661f5466"
- email: "Admin@example.com"
  # bcrypt hash of the string "password"
  hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W"
  username: "Admin"
  userID: "08a8684b-db88-4b73-90a9-3cd1661f5466"

Only the second password gets registered. And then if the user tries to login with the first email it still succeed but returns the wrong email id in the claims:

"email": "Admin@example.com",

That's not desirable behavior :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rithujohn191 right, but prior to this that config would not have errored, if we error now we might break someone during a dex upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logged the error

@rithujohn191
Copy link
Contributor Author

working on adding the logger

@rithujohn191
Copy link
Contributor Author

Done :)

Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rithujohn191 rithujohn191 merged commit 3445895 into dexidp:master Aug 25, 2017
@rithujohn191 rithujohn191 deleted the static-password-fix branch August 25, 2017 16:30
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
storage/static.go: storage backend should not explicitly lower-case email ids.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants