Improving flows and rules around user creation #1272
Conversation
|
Thanks @nahumcohen! This is looking good, just a couple comments for you. The Code Climate issues also look relatively easy to address (syntax formatting issues). Would you mind taking care of those as well? Thanks! |
|
@nahumcohen, once we get the validation moved into the |
nahumcohen
force-pushed
the
user_creation_improving_flows
branch
from
bf2468a to
9894098
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
nahumcohen
force-pushed
the
user_creation_improving_flows
branch
2 times, most recently
from
1acd160 to
d853ee4
Compare
|
@nahumcohen, thanks for making these updates! This is looking good. These last code climate issues look like they're worth fixing, and then I think this should be good to go! |
nahumcohen
force-pushed
the
user_creation_improving_flows
branch
from
d853ee4 to
7102708
Compare
nahumcohen
force-pushed
the
user_creation_improving_flows
branch
from
7102708 to
af07f86
Compare
| head 204 | ||
| rescue Sequel::ValidationFailed => e | ||
| # render json with unprocessable_entity only for insufficient_password_complexity otherwise, raise an error | ||
| insufficientMsg = Errors::Conjur::InsufficientPasswordComplexity.new.to_s |
There was a problem hiding this comment.
Use snake_case for variable names.
nahumcohen
force-pushed
the
user_creation_improving_flows
branch
from
af07f86 to
35f8594
Compare
nahumcohen
force-pushed
the
user_creation_improving_flows
branch
from
35f8594 to
cb38c3d
Compare
|
Code Climate has analyzed commit cb38c3d and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 88.4% (0.0% change). View more on Code Climate. |
| @@ -63,8 +70,7 @@ def validate | |||
|
|
|||
| validates_presence [ :api_key ] | |||
|
|
|||
| errors.add(:password, 'must not be blank') if @plain_password && @plain_password.empty? | |||
| errors.add(:password, 'cannot contain a newline') if @plain_password && @plain_password.index("\n") | |||
| errors.add(:password, ::Errors::Conjur::InsufficientPasswordComplexity.new.to_s) if @plain_password && (@plain_password.index("\n") || @plain_password !~ VALID_PASSWORD_REGEX) | |||
There was a problem hiding this comment.
Let's use the Sequel Gem validation: https://github.com/jeremyevans/sequel/blob/master/doc/validations.rdoc#validates_format
It would be something like:
validates_format VALID_PASSWORD_REGEX, : plain_password, message: ::Errors::Conjur::InsufficientPasswordComplexity.new.to_s
There was a problem hiding this comment.
The scope of validate method includes both api_key and plain_password. If we add validate_format it will not separate between the two use-cases.
See existing password validation logic is to verify existence of plain_password parameter, and only after, verify complexity, blank & new line.
We also wanted to utilize validates_presence, but neglect the idea for the same reason above.
What does this PR do?
Improves flows and rules around user creation.
How should this be manually tested?
Install appliance once this is merged to master.
Has the Version and Changelog been updated?
The version has not the Changelog has updated.
Questions: