API should respond with 401 not just silently not list embargoed if embargoed are requested

[yarikoptic]

ATM we have odd behavior that even if I request embargoed dandisets explicitly, I would simply not get them listed in the output listing all dandisets, instead of what would be more appropriate IMHO to provide some feedback that authentication is needed to get that information.

❯ curl --silent -X 'GET' 'https://api.dandiarchive.org/api/dandisets/?page_size=1000&draft=true&empty=true&embargoed=true' -H 'accept: application/json' -H "Authorization: token $DANDI_API_KEY" | jq . | grep -e embargo_status | sort | uniq -c
     81       "embargo_status": "EMBARGOED",
    350       "embargo_status": "OPEN",
❯ curl --silent -X 'GET' 'https://api.dandiarchive.org/api/dandisets/?page_size=1000&draft=true&empty=true&embargoed=true' -H 'accept: application/json' | jq . | grep -e embargo_status | sort | uniq -c
    350       "embargo_status": "OPEN",

NB not to say that there is no option in swagger to provide examples with proper header field for that, just some insufficient X-CSRFToken.

IMHO 401 with WWW-Authenticate would be most DX friendly to respond if I do request explicitly embargoed=true. If embargoed=false or not explicitly provided - then it would be fine to not list them.

WDYT @satra @jwodder and @dandi/dandiarchive ?

[waxlamp]

I believe the reason such dandisets aren't listed at all is to prevent the discovery of their existence. That is, if you don't have permission to view such a resource, then even learning of the existence thereof would violate a specific standard of authorization. In a similar manner, such a policy would give a 404 instead of a 401 when attempting to access unauthorized resources by name or ID.

Of course we can do something different here, but this impacts our security model so we need to make the requirements explicit before we make a change.

Or maybe I misunderstood your problem?

[jjnesbitt]

@roni I think the point he's making is that if embargoed=true is included in the URL query parameters, we should respond with a 401, since it's nonsensical to ask for embargoed dandisets while not authenticated in the first place.

[yarikoptic]

I think that behavior should be

• for non-authenticated requests, embargoed=true should be 401 (indicating the need to authenticate)
• for authenticated -- embargoed=true should list only dandisets that user has access to (admins would have to all).

It is not about security concern of accessing a resource (and our IDs has no privacy concerns IMHO since they are just incremental numbers). It is about developer (and API "users") experience, and thus marked as such: it is only "internal knowledge" ATM that we are not to list embargoed datasets at all even if requested if not authorized.

[waxlamp]

Thanks for clarifying, @AlmightyYakob and @yarikoptic.

[waxlamp]

I'd like to revisit this a bit. My model for what embargo=true means is to include whatever embargoed dandisets I have access to. For that reason, I don't believe a 401 response is appropriate in the case of an unauthenticated user making such a request.

Compare the situation of an authenticated user who does not have access to any embargoed dandisets. The argument might run that because they are asking for something that they don't have access to, that too should result in a 401.

For both cases, my approach is that the set of embargoed dandisets each user has access to is the empty set; therefore in both cases, the appropriate response is just the open dandisets.

Part of my thinking here is practical: if we send back a 401 for unauthenticated users in this situation, we are opening ourselves up to a class of frontend bugs where the app receives a 401 (and therefore breaks) in case it allows for sending embargo=true by mistake. And the rest has to do with the open nature of the archive: it seems wrong to send a user a 401 when there were legitimate resources we could have sent them, together with the semantics of only sharing embargoed dandisets they are authorized for (which may well, and often is the empty set).

But this brings me to a zoom-out question: @yarikoptic, I'm wondering why you need this feature. Is it because you have a script that accidentally runs unauthenticated and therefore returns the "wrong" data? In that case I would recommend simply having the script check whether a user is logged in, and erroring out if not. That solves the problem with zero code changes to the archive, which to me is a win. Of course, that may not be the problem you're having...

[waxlamp]

@yarikoptic, an update: I am still interested in your immediate use case, but Jake and I spoke to some of our tech elders here, and we reached a consensus that Jake's design in #1790 is a good one, so I'll withdraw my suggestion that we should close that PR and solve your problem in a different way.

[yarikoptic]

note my comment in that PR.

[waxlamp]

note my comment in that PR.

That's a comment on the web app behavior and how it might change--did you mean to link to something else?

[yarikoptic]

yes, it is on web-app but web-app is part of the dandi-archive repo... wasn't that an idea to join them into a single repo to provide such bundled changes to API and reflected changes to web frontend, or what am I missing?

[dandibot]

🚀 Issue was released in v0.3.70 🚀