Fixed delimiter validation in resources endpoint #1998

liavyona · 2021-01-19T15:33:12Z

What does this PR do?

_What's changed? Added delimiter validation to resources endpoint
Are there relevant screenshots you can add to the PR description?

What ticket does this PR close?

Resolves #1997

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

orenbm · 2021-01-19T15:37:35Z

app/models/resource.rb

@@ -105,6 +105,9 @@ def search account: nil, kind: nil, owner: nil, offset: nil, limit: nil, search:
 scope = scope.textsearch(search) if search

 if offset || limit
+ if (offset && !numeric?(offset)) || (limit && !numeric?(limit))
+ raise ApplicationController::UnprocessableEntity, "Delimiter must be an integer if given"


@shulifink can you please review this message?

I don't understand. What do you mean "if given"?

@liavyona Isn't it enough to just say "If you provide a value for Delimiter, it must be an integer greater than or equal to 0".

or simply: "Delimiter must be an integer greater than or equal to 0".

or "... must be an integer", or "must be a positive integer " (depending on what the integer type)

I would go with "Delimiter offset or limit must be an integer greater than or equal to 0"

orenbm

LGTM @liavyona , thanks!
please get a review from @shulifink on the message.

codeclimate · 2021-01-19T15:53:55Z

app/controllers/application_controller.rb

@@ -212,6 +216,11 @@ def bad_request e
 head :bad_request
 end

+ def unprocessable_entity e


ApplicationController#unprocessable_entity has the parameter name 'e'

codeclimate · 2021-01-19T15:53:55Z

app/models/resource.rb

@@ -131,6 +134,10 @@ def textsearch input
 def visible_to role
 from Sequel.function(:visible_resources, role.id).as(:resources)
 end
+
+ def numeric? val


Resource#numeric? doesn't depend on instance state (maybe move it to another class?)

izgeri · 2021-01-19T15:57:51Z

@liavyona please make sure to add a change log message for this! it's definitely a user-impacting change, and we should let people know we've fixed this.

orenbm · 2021-01-19T17:46:29Z

@liavyona the code is fine by me but i would like to see a better commit message. please read this post. To be specific, please change the tense to Fix instead of Fixed and add a description that will explain this change a bit better for the future devs who will see it.

CHANGELOG.md

app/models/resource.rb

codeclimate · 2021-01-20T08:13:29Z

app/models/resource.rb

 case kind
 when "variable"
 response["secrets"] = secrets_dataset.order(:version).as_json
- .map { |h| h.except 'resource' }
+  .map { |h| h.except 'resource' }


Use 2 (not 37) spaces for indenting an expression in an assignment spanning multiple lines.

app/models/resource.rb

sigalsax

Great work! Thanks for jumping on this bug
A couple of things 1. as Oren mentioned please check with Shuli that these logs are ok before merging 2. I tagged you in a codeclimate suggestion so check that out :)

app/models/resource.rb

CHANGELOG.md

sigalsax

LGTM! Thanks for this improvement!

jonahx · 2021-01-21T14:18:16Z

app/models/resource.rb

+ # 'offset' must be an integer greater than or equal to 0 if given
+ if offset && (!numeric?(offset) || offset.to_i.negative?)
+ raise ApplicationController::UnprocessableEntity, "'offset' contains an invalid value. 'offset' must be an integer greater than or equal to 0."
+ end


UnprocessableEntity is an http level concern. That is, we should only be talking about that error inside of controllers, and specifically in code that is mapping an actual domain error into an http status code.

Inside of the model we should be raising a more specific validation error, with a name that reflects what went wrong. InvalidQueryParameter or something like that.

Separate comment: There's a lot of formatting / clean changes here (which is awesome), but please put those into a separate commit with a subject like "Cleanup formatting" -- otherwise the "meat" of this PR (which is actually pretty small and simple) isn't clear at a glance.

@jonahx fixed

InbalZilberman

Looks great @liavyona
Thank you for taking this on.

app/controllers/resources_controller.rb

app/models/resource.rb

orenbm · 2021-01-24T13:43:22Z

app/models/resource.rb

+ raise "'limit' contains an invalid value. 'limit' must be a positive integer."
+ end
+ # 'offset' must be an integer greater than or equal to 0 if given
+ if offset && (!numeric?(offset) || offset.to_i.negative?)
+ raise "'offset' contains an invalid value. 'offset' must be an integer greater than or equal to 0."


@liavyona is it possible to move these errors to errors.rb?
we prefer to have all the user-facing messages there

I think it will be a problem to use a custom exception inside a model class. Ads you can see there is no usage of any error which is defined at errors.rb under app/model. Other model classes use simple raise or ArgumentError. I changed it to use ArgumentError

cucumber/api/features/resource_list.feature

`GET /resources` endpoints takes 2 optionals numeric delimiters as requests parameters: `limit` & `offset`. Previously, no input validation has been made when receiving these optional parameters. Now, we verify that their value is valid if given.

codeclimate · 2021-01-26T10:06:56Z

Code Climate has analyzed commit f8679e1 and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	7
Style	1

The test coverage on the diff in this pull request is 90.9% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.3% (0.0% change).

View more on Code Climate.

liavyona added kind/bug component/conjur labels Jan 19, 2021

liavyona requested review from micahlee, sigalsax, orenbm and shulifink January 19, 2021 15:33

liavyona self-assigned this Jan 19, 2021

liavyona requested a review from a team as a code owner January 19, 2021 15:33

orenbm reviewed Jan 19, 2021

View reviewed changes

orenbm previously approved these changes Jan 19, 2021

View reviewed changes

liavyona requested review from john-odonnell and telday January 19, 2021 15:45

liavyona dismissed orenbm’s stale review via 2335f67 January 19, 2021 15:53

codeclimate bot reviewed Jan 19, 2021

View reviewed changes

liavyona force-pushed the 1997-delimiter-validation branch from 574535e to 01199e5 Compare January 19, 2021 16:19

liavyona requested a review from orenbm January 19, 2021 17:42

sigalsax reviewed Jan 20, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

sigalsax reviewed Jan 20, 2021

View reviewed changes

app/models/resource.rb Outdated Show resolved Hide resolved

liavyona force-pushed the 1997-delimiter-validation branch from 01199e5 to 1faaf94 Compare January 20, 2021 07:49

liavyona requested a review from sigalsax January 20, 2021 07:50

codeclimate bot reviewed Jan 20, 2021

View reviewed changes

sigalsax previously approved these changes Jan 20, 2021

View reviewed changes

liavyona dismissed sigalsax’s stale review via e11474c January 20, 2021 08:53

liavyona force-pushed the 1997-delimiter-validation branch from 1faaf94 to e11474c Compare January 20, 2021 08:53

shulifink suggested changes Jan 21, 2021

View reviewed changes

app/models/resource.rb Outdated Show resolved Hide resolved

app/models/resource.rb Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

liavyona force-pushed the 1997-delimiter-validation branch from e11474c to 6a22304 Compare January 21, 2021 14:08

sigalsax self-requested a review January 21, 2021 14:11

sigalsax previously approved these changes Jan 21, 2021

View reviewed changes

jonahx reviewed Jan 21, 2021

View reviewed changes

liavyona dismissed sigalsax’s stale review via 44c5541 January 21, 2021 15:27

liavyona force-pushed the 1997-delimiter-validation branch 3 times, most recently from 2a78590 to a9b50e7 Compare January 21, 2021 15:43

liavyona requested review from jonahx and sigalsax January 21, 2021 15:44

liavyona force-pushed the 1997-delimiter-validation branch from a9b50e7 to 5549927 Compare January 21, 2021 16:42

jonahx previously approved these changes Jan 21, 2021

View reviewed changes

InbalZilberman reviewed Jan 21, 2021

View reviewed changes

orenbm reviewed Jan 24, 2021

View reviewed changes

app/controllers/resources_controller.rb Outdated Show resolved Hide resolved

orenbm reviewed Jan 24, 2021

View reviewed changes

app/models/resource.rb Show resolved Hide resolved

orenbm reviewed Jan 24, 2021

View reviewed changes

cucumber/api/features/resource_list.feature Show resolved Hide resolved

liavyona dismissed jonahx’s stale review via f06a1df January 26, 2021 08:07

liavyona force-pushed the 1997-delimiter-validation branch from f06a1df to 442015a Compare January 26, 2021 08:14

orenbm previously approved these changes Jan 26, 2021

View reviewed changes

liavyona dismissed orenbm’s stale review via f8679e1 January 26, 2021 09:41

liavyona force-pushed the 1997-delimiter-validation branch from 442015a to f8679e1 Compare January 26, 2021 09:41

orenbm self-requested a review January 26, 2021 09:42

orenbm approved these changes Jan 26, 2021

View reviewed changes

liavyona merged commit 0c4dabd into master Jan 26, 2021

liavyona deleted the 1997-delimiter-validation branch January 26, 2021 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed delimiter validation in resources endpoint #1998

Fixed delimiter validation in resources endpoint #1998

liavyona commented Jan 19, 2021 •

edited

Loading

orenbm Jan 19, 2021

shulifink Jan 20, 2021

shulifink Jan 21, 2021

InbalZilberman Jan 24, 2021

orenbm left a comment

codeclimate bot Jan 19, 2021

codeclimate bot Jan 19, 2021

izgeri commented Jan 19, 2021

orenbm commented Jan 19, 2021

codeclimate bot Jan 20, 2021

sigalsax left a comment

sigalsax left a comment

jonahx Jan 21, 2021

liavyona Jan 21, 2021

InbalZilberman left a comment

orenbm Jan 24, 2021

liavyona Jan 26, 2021

codeclimate bot commented Jan 26, 2021

Fixed delimiter validation in resources endpoint #1998

Fixed delimiter validation in resources endpoint #1998

Conversation

liavyona commented Jan 19, 2021 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm left a comment

Choose a reason for hiding this comment

codeclimate bot Jan 19, 2021

Choose a reason for hiding this comment

codeclimate bot Jan 19, 2021

Choose a reason for hiding this comment

izgeri commented Jan 19, 2021

orenbm commented Jan 19, 2021

codeclimate bot Jan 20, 2021

Choose a reason for hiding this comment

sigalsax left a comment

Choose a reason for hiding this comment

sigalsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InbalZilberman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Jan 26, 2021

liavyona commented Jan 19, 2021 •

edited

Loading