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.

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

eyre: explicit 401 instead of implicit downgrade #7076

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Conversation

Fang-
Copy link
Member

@Fang- Fang- commented Oct 7, 2024

With the addition of guest identities (#6561) eyre started granting guest identities to unauthenticated requesters. However, requesters that tried authenticating with invalid credentials (for non-existent or expired sessions) would be granted a guest identity as well, implicitly "downgrading" their request in the process, to appear as if originating from the guest identity.

This behavior is neither correct nor legible. Especially API-level interactions will want to avoid accidentally opening a channel with an unexpected identity.

We improve Eyre's behavior by detecting the "invalid session" case and returning an explicit 401 (unauthorized) response. To make sure that the browser case doesn't stay stuck in a broken session, we include a set-cookie header that expires the invalid cookie instantly.

In practice, this change should only be user-facing in the "log out of all sessions" case, where users may see a 401 page in browsers on their other devices after performing that action. A simple refresh of the page gets them back into the usual, endpoint-defined behavior.

Draft PR because this breaks tests in a non-trivial way. Apparently Eyre silently ignoring unrecognized sessions was load-bearing for the way the tests are structured.
I'm working on a version of the tests that "captures" & remembers the session cookies that come out of Eyre, but I've exceeded the timebox I set for myself on this. Will try to return to this Soon™.

With the addition of guest identities (#6561) eyre started granting
guest identities to unauthenticated requesters. However, requesters that
tried authenticating with _invalid_ credentials (for non-existent or
expired sessions) would be granted a guest identity as well, implicitly
"downgrading" their request in the process, to appear as if originating
from the guest identity.

This behavior is neither correct nor legible. Especially API-level
interactions will want to avoid accidentally opening a channel with an
unexpected identity.

We improve Eyre's behavior by detecting the "invalid session" case and
returning an explicit 401 (unauthorized) response. To make sure that the
browser case doesn't stay stuck in a broken session, we include a
`set-cookie` header that expires the invalid cookie instantly.

In practice, this change _should_ only be user-facing in the "log out of
all sessions" case, where users may see a 401 page in browsers on their
other devices after performing that action. A simple refresh of the page
gets them back into the usual, endpoint-defined behavior.
@Fang- Fang- added the eyre label Oct 7, 2024
In practice, header ordering doesn't actually matter. Needing to specify
the headers in the exact order made testing them more difficult than it
should've been.

Here, we alphabetically sort the headers before testing them. ( ex-204-2
is to be removed in a future commit, so is ignored here.)

We do take care to preserve relative ordering between headers that have
the exact same field-name value, because that case _does_ matter.
Cookies have been the bane of eyre tests for a while. We had resorted to
hard-coding the couple of expected cookie values and being careful about
passing them in the right place, right time. This was very finicky.

Here, we update the state that's passed continuously to include a value
for the "live" cookie. The rest of the testing code and helpers are
updated to keep this value updated according to the responses emitted by
eyre, and to inject the cookie into requests we simulate. This way, we
get the desired behavior of remembering the cookie like a browser would.

We get to remove the hard-coded cookie values from most tests. In the
process, we clean up the helpers a little bit, but unfortunately also
have to add more complex helpers for pulling the cookie from state.

All in all, tests should be more robust/flexible, at a small complexity
cost. The pattern here (moving over operations, updating & accumulating
state) might be better implemented as the state machine core (aka abet)
pattern, but that would be a bigger rewrite.
@Fang- Fang- marked this pull request as ready for review November 4, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants