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

BadRequest validation added to Kamus API #94

Merged
merged 12 commits into from
Jan 22, 2019
Merged

Conversation

AleF83
Copy link
Contributor

@AleF83 AleF83 commented Jan 21, 2019

Close #93

@omerlh
Copy link
Contributor

omerlh commented Jan 21, 2019

@AleF83 bumb version please

@@ -28,23 +29,28 @@ public EncryptController(IKeyManagement keyManagement)
[Route("api/v1/encrypt")]
public async Task<ActionResult> Encrypt([FromBody]EncryptRequest body)
{
if (!ModelState.IsValid)
{
return BadRequest("One of required fields doesn't present in request body.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Log the validation error please

@@ -35,6 +35,11 @@ public DecryptController(IKubernetes kubernetes, IKeyManagement keyManagement)
[Authorize(AuthenticationSchemes = "kubernetes")]
public async Task<ActionResult> Decrypt([FromBody] DecryptRequest body)
{
if (!ModelState.IsValid)
{
return BadRequest("One of required fields doesn't present in request body.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Log validation error please

@AleF83
Copy link
Contributor Author

AleF83 commented Jan 21, 2019

See last commit. Do you mean that?

@AleF83
Copy link
Contributor Author

AleF83 commented Jan 21, 2019

I want to add some tests to blackbox. Something that isn't happy flow.

@omerlh
Copy link
Contributor

omerlh commented Jan 21, 2019

sounds good, should be easy to do, need help?

omerlh
omerlh previously approved these changes Jan 21, 2019
Copy link
Contributor

@omerlh omerlh left a comment

Choose a reason for hiding this comment

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

LGTM just bump version

Copy link
Contributor

@omerlh omerlh left a comment

Choose a reason for hiding this comment

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

IMO this should and can be a minor

src/encrypt-api/encrypt-api.csproj Outdated Show resolved Hide resolved
src/decrypt-api/decrypt-api.csproj Outdated Show resolved Hide resolved
@AleF83
Copy link
Contributor Author

AleF83 commented Jan 22, 2019

I've added test case for BadRequest. Review once again please

src/decrypt-api/Controllers/DecryptController.cs Outdated Show resolved Hide resolved
src/decrypt-api/Controllers/DecryptController.cs Outdated Show resolved Hide resolved
src/encrypt-api/Controllers/EncryptController.cs Outdated Show resolved Hide resolved
src/encrypt-api/Controllers/EncryptController.cs Outdated Show resolved Hide resolved
public string SerivceAccountName
{
get;
set;
}

[JsonProperty(PropertyName = "namespace", Required = Required.Always)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the required?

tests/blackbox/EncryptControllerTests.cs Outdated Show resolved Hide resolved
omerlh and others added 4 commits January 22, 2019 10:08
Co-Authored-By: AleF83 <AlexanderFux83@gmail.com>
Co-Authored-By: AleF83 <AlexanderFux83@gmail.com>
Co-Authored-By: AleF83 <AlexanderFux83@gmail.com>
Co-Authored-By: AleF83 <AlexanderFux83@gmail.com>
src/decrypt-api/Controllers/DecryptController.cs Outdated Show resolved Hide resolved
@@ -125,5 +125,22 @@ public async Task AnonymousRequestToDecryptEndpointShouldFail()

Assert.Equal(HttpStatusCode.Unauthorized, result.StatusCode);
}

[Fact]
public async Task Encrypt_BadRequest_ShouldFail()
Copy link
Contributor

Choose a reason for hiding this comment

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

What about decryptor tests?

Co-Authored-By: AleF83 <AlexanderFux83@gmail.com>
@AleF83 AleF83 merged commit fd41bce into master Jan 22, 2019
@AleF83 AleF83 deleted the kamus-api-return-400 branch January 22, 2019 09:01
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.

2 participants