Skip to content

Conversation

@dschom
Copy link
Contributor

@dschom dschom commented Sep 20, 2025

Because

  • Just checking the state returned by /session/status often isn't enough

This pull request

  • Adds a new endpoint called /session/status/detailed that exposes more detailed info about the session status state.

Issue that this pull request solves

Closes: FXA-12433

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@dschom dschom requested a review from a team as a code owner September 20, 2025 00:45
Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@dschom Looks like this endpoint was added 11 years ago f7783b3. This was well before there were many different verification states on sessions. It is possible that the original /session/status endpoint does not provide any value over the new endpoint your proposing.

Maybe it makes sense to combine them?

},
{
method: 'GET',
path: '/session/status/detailed',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to combine this into the the /session/status endpoint?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also account profile route that exposes some of this information

Copy link
Contributor Author

@dschom dschom Sep 23, 2025

Choose a reason for hiding this comment

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

It might. I was certainly considering this.

Initially I was concerned that this endpoint now also hits the account table as well as the session table, so it's quite a bit heavier than the /session/status call. I just checked our stats though, and it seems like this endpoint isn't heavily called. Account profile seems like overkill for what I plan to use this for since it's also pulling capabilities.

Let me know what you think. If you think updating /session/status won't break things I'll do that. I was also a little uncertain about who all called /session/status. Seems like it could be a pretty useful endpoint for RPs in general, and I don't know if they respond well to a change in response format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vbudhram I have updated this to just add on to the response of the/session/status endpoint. It seems like a couple integration tests are failing now, so just need circle back on that.

@dschom dschom force-pushed the FXA-12433 branch 2 times, most recently from f353f26 to b21adeb Compare September 26, 2025 23:15
Because:
- Just checking the state returned by `/session/status` often isn't enough
- We need more details about the session status

This Commit:
- Updates /session/status adn that exposes details about the session status state.
@dschom dschom merged commit 27e5133 into main Oct 3, 2025
19 checks passed
@dschom dschom deleted the FXA-12433 branch October 3, 2025 15:56
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