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.

Sign up for GitHub

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

Jump to bottom

Branch protection status code should be 403 #5273

Merged
merged 5 commits into from
Feb 23, 2023
Merged

Branch protection status code should be 403 #5273

merged 5 commits into from
Feb 23, 2023

Conversation

nopcoder
Copy link
Contributor

Close #5272

@nopcoder nopcoder added bug Something isn't working area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog labels Feb 16, 2023
@nopcoder nopcoder self-assigned this Feb 16, 2023
ozkatz
ozkatz requested changes Feb 16, 2023
View reviewed changes
Copy link
Collaborator

@ozkatz ozkatz left a comment

Choose a reason for hiding this comment

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

Thanks! Just a couple of minor things to change:

  1. Create a swagger error type for Forbidden errors (currently the errors are returned with a description of Internal Server Error)
  2. Handle the other error that may rise from committing to a protected branch
  3. Verify that merging could fail due to branch protection? if so, in which case? (i.e. if this is really the case, this requires documentation)

api/swagger.yml Outdated
@@ -2605,6 +2605,8 @@ paths:
$ref: "#/components/responses/ValidationError"
401:
$ref: "#/components/responses/Unauthorized"
403:
$ref: "#/components/responses/ServerError"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be a new error type as this message is still described as an internal server error

api/swagger.yml
@@ -2762,6 +2766,8 @@ paths:
$ref: "#/components/responses/ValidationError"
401:
$ref: "#/components/responses/Unauthorized"
403:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can a merge operation fail due to branch protection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right - I thought we added this feature. Was asked once, on how we can support read-only branch while branch protection commit doesn't stop merge's commit.
I'll keep the status code on the interface unless you think it will be confusion?

pkg/api/controller.go Outdated
@@ -1849,6 +1849,9 @@ func (c *Controller) handleAPIErrorCallback(ctx context.Context, w http.Response
errors.Is(err, kv.ErrNotFound):
cb(w, r, http.StatusNotFound, err)

case errors.Is(err, graveler.ErrWriteToProtectedBranch):
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's also a specific error returned when trying to commit to a protected branch (see ErrCommitToProtectedBranch), which this doesn't cover

clients/java/src/main/java/io/lakefs/clients/api/BranchesApi.java Outdated
@@ -213,6 +213,7 @@ public okhttp3.Call createBranchAsync(String repository, BranchCreation branchCr
<tr><td> Status Code </td><td> Description </td><td> Response Headers </td></tr>
<tr><td> 204 </td><td> branch deleted successfully </td><td> - </td></tr>
<tr><td> 401 </td><td> Unauthorized </td><td> - </td></tr>
<tr><td> 403 </td><td> Internal Server Error </td><td> - </td></tr>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Example of why this is confusing :)

@nopcoder
Copy link
Contributor Author

  • Create a swagger error type for Forbidden errors (currently the errors are returned with a description of Internal Server Error)
  • Handle the other error that may rise from committing to a protected branch
  • Verify that merging could fail due to branch protection? if so, in which case? (i.e. if this is really the case, this requires documentation)

Thanks for the input - did better work in the next commit:

  1. Added Fobidden error
  2. Have common error for both cases of branch protection
  3. Verified - commit protection doesn't block merge into branch. Think it will be helpful to add a "complete" protection option to enable read-only repositories.

@nopcoder nopcoder requested a review from ozkatz February 20, 2023 07:27
@itaiad200 itaiad200 added the team/versioning-engine Team versioning engine label Feb 20, 2023
ozkatz
ozkatz approved these changes Feb 23, 2023
View reviewed changes
Copy link
Collaborator

@ozkatz ozkatz left a comment

Choose a reason for hiding this comment

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

Great! thank you!

@nopcoder nopcoder merged commit 1c8481a into master Feb 23, 2023
@nopcoder nopcoder deleted the fix/protected-status-code branch February 23, 2023 14:04
This was referenced Mar 6, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ozkatz ozkatz ozkatz approved these changes

Assignees

@nopcoder nopcoder

Labels
area/API Improvements or additions to the API bug Something isn't working include-changelog PR description should be included in next release changelog team/versioning-engine Team versioning engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Put object branch protection status code should be 403 Forbidden
3 participants
@nopcoder @ozkatz @itaiad200