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

Security Risk - User Hijacking? #52

Open
keithn opened this issue Oct 26, 2018 · 4 comments
Open

Security Risk - User Hijacking? #52

keithn opened this issue Oct 26, 2018 · 4 comments

Comments

@keithn
Copy link

keithn commented Oct 26, 2018

Not sure, just glancing through the code, but in user Edit it doesn't seem like user names are required to be unique when editing? all the code to deal with uniqueness is on create.

A current user could change their username to another users user name,
Then they could edit again and then the firstordefault will likely give them access to the original user.

@keithn
Copy link
Author

keithn commented Oct 26, 2018

also, seems like there is code between create and edit is duplicated... This doesn't seem like a good idea.

@adamhathcock
Copy link
Collaborator

Pull requests are welcome to fix possible issues.

Without looking at the code, you might be right about duplicattion. However, a little duplication can be better than added complexity to remove the little duped code. As always: it depends

@keithn
Copy link
Author

keithn commented Oct 27, 2018

I'm just having a look more than anything else, I was just curious how security was being handled. I think because of how the user concerns are spread out and are quite noisy it leads to the security problem , the duplication was the hint that should of pointed to more cohesive solution to the rules. I don't think I'd ever endorse having two locations in the code where you set things like passwords.

@adamhathcock
Copy link
Collaborator

Cross cutting concerns can be put in MediatR pipelines if there are any.

Things are organized vertically to illustrate how functionality is usually simple before “concerns” are added in. I’ll admit things aren’t perfect nor fully audited but I see no reason for further abstraction as there aren’t more rules to implement yet.

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

No branches or pull requests

2 participants