Skip to content

Add a new "Forbidden" FxA error.#7320

Merged
mhammond merged 1 commit intomozilla:mainfrom
mhammond:push-nmtrrkyuzszx
Apr 16, 2026
Merged

Add a new "Forbidden" FxA error.#7320
mhammond merged 1 commit intomozilla:mainfrom
mhammond:push-nmtrrkyuzszx

Conversation

@mhammond
Copy link
Copy Markdown
Member

@mhammond mhammond commented Apr 15, 2026

This is distinct from Authentication in that there's no implication the account state isn't good, it's just that you aren't allowed to do what you tried. An example of this is asking for an access token you don't have the scopes for.

This has the side effect of fixing a bug in get_access_token() - when that code calls exchange_token_for_scope(), the server responds with a http 403 response - which ends up getting re-thrown as "other error".

It arranges for NoCachedToken errors to turn into this (that was previously Authentication) and for NoSessionToken to be Authentication. It also tweaks get_access_token() to explicitly throw NoSessionToken when there's no session token. That way get_access_token() continues to return Authentication when not logged in, but otherwise will generally return Forbidden.

Technically a breaking change, but both Android and iOS seem fine with new variants here.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

@mhammond mhammond requested review from bendk and jonalmeida April 15, 2026 04:30
@mhammond mhammond force-pushed the push-nmtrrkyuzszx branch from da266a4 to c5e4699 Compare April 15, 2026 04:31
Copy link
Copy Markdown
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me. My only suggestion is maybe we could get rid of NoCachedToken entirely and replace it with more specific errors. Looking through the code I see 2 remaining uses:

If it's easy and makes sense to you, then I'm for adding those changes to this PR. Otherwise, we can land it as-is and keep iterating.

@mhammond
Copy link
Copy Markdown
Member Author

This change looks good to me. My only suggestion is maybe we could get rid of NoCachedToken entirely and replace it with more specific errors.

Yeah, good idea. I agree the first one is literally impossible, so I made that unreachable!(). For the second case I didn't generalize ScopedKeyMissingInServerResponse because that has unfortunately been exposed to the mobile clients, wihch I think is unfortunate because there's really nothing they can do about it and it does imply an expected server response. So I added a new UnexpectedServerResponse for cases like this - eg, where we ask for a token and get a 200 response without a token, etc. I also added a note that in the future we should be able to replace ScopedKeyMissingInServerResponse with this, but that's a larger refactor, both in the clients and in the rust code (ie, we should probably be throwing this error at the time we expected to have got the key, not at a later time when someone finally asks for an access token?

(A problem with UnexpectedServerResponse, which I also added to the comments, is that in many cases this would actually show up as a JSON error - eg, when the shape of the payload is wrong. But that seems OK, and both of them end up as "OtherError" to the consumer.)

@mhammond
Copy link
Copy Markdown
Member Author

(ScopedKeyMissingInServerResponse also overlaps with NoScopedKey!)

@mhammond mhammond force-pushed the push-nmtrrkyuzszx branch from c5e4699 to 239166f Compare April 16, 2026 01:14
This is distinct from Authentication in that there's no implication the account
state isn't good, it's just that you aren't allowed to do what you tried.
An example of this is asking for an access token you don't have the scopes for.

This has the side effect of fixing a bug in `get_access_token()` - when that
code calls `exchange_token_for_scope()`, the server responds with a http 403
response - which ends up getting re-thrown as "other error".

It arranges for `NoCachedToken` errors to turn into this (that was previously
`Authentication`) and for `NoSessionToken` to be `Authentication`. It also tweaks
`get_access_token()` to explicitly throw `NoSessionToken` when there's no session
token. That way `get_access_token()` continues to return `Authentication` when
not logged in, but otherwise will generally return `Forbidden`.

Technically a breaking change, but both Android and iOS seem fine with new
variants here.
@mhammond mhammond force-pushed the push-nmtrrkyuzszx branch from 239166f to 780150a Compare April 16, 2026 01:15
@mhammond mhammond enabled auto-merge April 16, 2026 01:27
@mhammond mhammond added this pull request to the merge queue Apr 16, 2026
Merged via the queue into mozilla:main with commit a322032 Apr 16, 2026
14 checks passed
@mhammond mhammond deleted the push-nmtrrkyuzszx branch April 16, 2026 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants