Bug #16774

keep-web needs to return user-visible errors

Added by Peter Amstutz 5 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Start date:
11/20/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0

Description

I tried to download a file from keep-web with an invalid token. In the browser, without using developer tools, there is no feedback to the user why it didn't work.

  1. There is no error page. For any 4xx or 5xx error it should return a minimal error page
  2. Despite the error is actually that the token is invalid, it returns 404 instead of 401
  3. A 404 can mean either that an item doesn't exist, or that the user doesn't have permission to see it. The error text should reflect that.

Sort of related, I also realized that if keep-web sets a cookie with the API token, there's no way for the user to clear that cookie without going into browser settings.


Subtasks

Task #17027: Review 16774-keep-web-errorsResolvedNico César


Related issues

Related to Arvados Epics - Story #16444: Improved error detection/reportingNew09/30/202110/31/2021

Related to Arvados - Feature #16669: Accept OpenID Connect access tokenFeedback09/24/2020

Associated revisions

Revision c75f2e9b
Added by Peter Amstutz about 2 months ago

Merge branch '16774-keep-web-errors' refs #16774

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz 5 months ago

  • Description updated (diff)

#2 Updated by Peter Amstutz 5 months ago

  • Description updated (diff)

#3 Updated by Peter Amstutz 5 months ago

  • Related to Story #16444: Improved error detection/reporting added

#4 Updated by Peter Amstutz 4 months ago

  • Target version changed from 2020-09-23 Sprint to 2020-10-07 Sprint

#5 Updated by Peter Amstutz 4 months ago

  • Target version changed from 2020-10-07 Sprint to 2020-10-21 Sprint

#6 Updated by Peter Amstutz 3 months ago

  • Target version deleted (2020-10-21 Sprint)

#7 Updated by Peter Amstutz 3 months ago

  • Target version set to 2020-11-04 Sprint

#8 Updated by Peter Amstutz 3 months ago

  • Subject changed from keep-web should return better errors to keep-web needs to return user-visible errors

#9 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#10 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#11 Updated by Peter Amstutz 3 months ago

  • Category set to Keep

#12 Updated by Peter Amstutz 3 months ago

  • Assigned To set to Peter Amstutz

#13 Updated by Peter Amstutz 3 months ago

  • Target version changed from 2020-11-04 Sprint to 2020-11-18

#14 Updated by Peter Amstutz 2 months ago

  • Target version changed from 2020-11-18 to 2020-12-02 Sprint

#15 Updated by Peter Amstutz 2 months ago

  • Story points set to 2.0

#16 Updated by Tom Clegg 2 months ago

#17 Updated by Peter Amstutz about 2 months ago

16774-keep-web-errors @ 34f5aff9a166c0e03564b607981284b6d4af9548

https://ci.arvados.org/view/Developer/job/developer-run-tests/2191/

WebDAV errors are currently plain text but maybe they should follow this https://www.ietf.org/proceedings/50/I-D/webdav-status-00.txt

S3 errors are now XML as expected by S3 clients.

#18 Updated by Tom Clegg about 2 months ago

Might be worth using XML encoder here, in case error messages/paths have "<" chars etc.

#19 Updated by Peter Amstutz about 2 months ago

16774-keep-web-errors @ aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

Fix tests

Use xml.EncodeElement for error response

https://ci.arvados.org/view/Developer/job/developer-run-tests/2194/

#20 Updated by Nico César about 2 months ago

review at aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

services/keep-web/handler.go:

  • Maybe I'm too picky here, but any reason to have "\n" in the errors? Would this cause issues on non-Unix systems that use "\r\n" like Windows? also reading RFC 2046 https://www.ietf.org/rfc/rfc2046.txt section 4.1.1 says: "The canonical form of any MIME "text" subtype MUST always represent a line break as a CRLF sequence. Similarly, any occurrence of CRLF in MIME "text" MUST represent a line break. Use of CR and LF outside of line break sequences is also forbidden."
    This is for Content-Type: text/plain, but I don't know what Content-Type we are using in the errors.
  • as a separate note, I also notice an extra "\n" on the check... as in c.Check(body, check.Equals, notFoundMessage+"\n") any reason why is there?

services/keep-web/s3_test.go:

I noticed the checks are comparing the body only. It will be good to add a comparison of the statusCode too.. since clients could be only checking that.

#21 Updated by Peter Amstutz about 2 months ago

  • Status changed from New to In Progress

#22 Updated by Peter Amstutz about 2 months ago

Nico César wrote:

review at aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

services/keep-web/handler.go:

  • Maybe I'm too picky here, but any reason to have "\n" in the errors? Would this cause issues on non-Unix systems that use "\r\n" like Windows? also reading RFC 2046 https://www.ietf.org/rfc/rfc2046.txt section 4.1.1 says: "The canonical form of any MIME "text" subtype MUST always represent a line break as a CRLF sequence. Similarly, any occurrence of CRLF in MIME "text" MUST represent a line break. Use of CR and LF outside of line break sequences is also forbidden."
    This is for Content-Type: text/plain, but I don't know what Content-Type we are using in the errors.

Sure. I don't think this is a problem in practice but I fixed it anyway.

  • as a separate note, I also notice an extra "\n" on the check... as in c.Check(body, check.Equals, notFoundMessage+"\n") any reason why is there?

The responses use http.Error() which calls Fprintln() which adds a newline to the end of the message:

https://golang.org/src/net/http/server.go?s=63178:63230#L2041

So the string comparison has to add the newline added by Fprintln().

services/keep-web/s3_test.go:

I noticed the checks are comparing the body only. It will be good to add a comparison of the statusCode too.. since clients could be only checking that.

I added checks for the codes in addition to the error messages.

16774-keep-web-errors @ 7d94b1ed55350c01689ad048aee961b261263dd9

https://ci.arvados.org/view/Developer/job/developer-run-tests/2196/

#23 Updated by Nico César about 2 months ago

review @ 7d94b1ed55350c01689ad048aee961b261263dd9

I think that \n or \r\n should be avoided from the message at all leaving as constants since 401 status code will come with them message:

const notFoundMessage = "The requested path was not found, or you do not have permission to access it." 
const unauthorizedMessage = "A valid Arvados token must be provided to access this resource." 

But either way is good to merge.

#24 Updated by Peter Amstutz about 2 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF