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

Fix status code for auth failure, and add DepositCompleted exception. #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix status code for auth failure, and add DepositCompleted exception. #21

wants to merge 2 commits into from

Conversation

alexdutton
Copy link
Contributor

This resolves #20, and also makes it easier for sword3common to pick up error definitions from the spec by publishing the CSV.

@alexdutton
Change status code for AuthenticationFailed, and add DepositComplete
8929cb9 
This will still need building and deploying.

Resolves #20.
@alexdutton
Add error-types.csv to the build
4a6c2b6 
sword3common has a script to extract error document types directly from the
spec, currently by parsing the HTML. Once this is merged and deployed it can
grab the CSV specfication directly, allowing a simpler implementation.
@alexdutton alexdutton added the enhancement New feature or request label Jan 31, 2020
@alexdutton alexdutton self-assigned this Jan 31, 2020
@richard-jones
Copy link
Contributor

I don't know if I agree with the status code changes

  • 401 is Unauthorised, and says "The response must include a WWW-Authenticate header field containing a challenge applicable to the requested resource", and we didn't intend to have a request/challenge/re-request cycle here, we're expecting the client to send the auth credentials with every request, and so to immediately receive a 403 if they do not. Perhaps this is wrong of us, but I want to sense check before accepting the change. 403 certainly says "This may be due to the user not having the necessary permissions for a resource" which would make sense for incorrect credentials or missing credentials if we do not issue a challenge.

  • 403 for DepositComplete I'm also not sure about. A completed deposit may still be accessible in some ways (e.g. GET on Object-URL), so a 405 for the specific method feels more appropriate. Perhaps we could have a more informative error document returned with the 405?

So, I'll hold off on merge of these, and we can discuss, and potentially take to the technical advisory group for comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richard-jones richard-jones Awaiting requested review from richard-jones

Assignees

@alexdutton alexdutton

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AuthenticationFailed error response is inconsistent with HTTP spec
2 participants
@alexdutton @richard-jones