stub endpoints should still have working authz + coverage #885
Conversation
|
Depends on #873. |
| /// HTTP "GET" method that is not yet implemented | ||
| /// | ||
| /// This should be a transient state, used only for stub APIs | ||
| GetUnimplemented, |
There was a problem hiding this comment.
do we anticipate needing this for other methods? e.g. PostUnimplemented?
There was a problem hiding this comment.
I don't think so. For everything other than GET, we only make requests that we expect to fail for authn/authz reasons. For GET, do that, but we also make a request with a privileged user that we expect to succeed. GetUnimplemented changes our expectation about this privileged request -- that it will fail with a 500.
There was a problem hiding this comment.
I added a comment to this effect on this variant and the previous variant in c4d8e1f.
| // behavior for unauthenticated and unauthorized users. DO NOT SKIP THIS. | ||
| // Even if you're just adding a stub, see [`Nexus::unimplemented_todo()`]. | ||
| // If you _added_ a test that covered an endpoint from the allowlist -- | ||
| // hooray! Just delete the corresponding line from this file. |
There was a problem hiding this comment.
yes! You might even say:
Why is this not expectorate:::assert_contents? Because this file should only shrink a line at a time; if you find yourself adding something you're probably doing something wrong and you deserve to suffer. Muahahahaha.
There was a problem hiding this comment.
Added a comment about this in c4d8e1f. Thanks!
There was a problem hiding this comment.
Less colorful than I would have preferred... but ok.
Supersedes #809.
This change:
This was more complicated than discussed in chat, mainly because the coverage tests checks more cases than can be handled by a simple
Nexus::unimplemented()function.