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

chore: Refresh session on request #520

Merged
merged 13 commits into from
Oct 15, 2024
Merged

Conversation

cmaddox5
Copy link
Contributor

Screenplay is very often left open in people's browser. This will eventually lead to an outdated session failing in our request pipeline and cause a CORS error due to a redirect. I followed this Notion doc to get a sense of what our session best practices are and that helped me find this PR. I followed in that PR's footsteps for the solution to our problem. We now set an idle time of 30 minutes (although the plug will prevent users from exceeding that) and a max session age of 12 hours.

@cmaddox5 cmaddox5 requested a review from a team as a code owner October 10, 2024 17:23
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

If this seems to work correctly on dev-green, looks good to me!

@cmaddox5
Copy link
Contributor Author

@digitalcora Just realized that there is some AuthManager configuration in runtime.exs so compile_env doesn't like how the config is changing after compilation. Might need to move back to the helper functions unless I'm overlooking a better solution.

This reverts commit 879513c.
@digitalcora
Copy link
Contributor

If you express it this way it may work better:

- @idle_time Application.compile_env!(:screenplay, __MODULE__)[:idle_time]
+ @idle_time Application.compile_env!(:screenplay, [__MODULE__, :idle_time])

The difference should be that with the latter, compile_env! can "see" the specific key you're accessing (and that it doesn't change at runtime), instead of only knowing that you're accessing the __MODULE__ group of keys (and then warning you because something in that group is set at runtime).

@cmaddox5 cmaddox5 merged commit 24a7afb into main Oct 15, 2024
3 checks passed
@cmaddox5 cmaddox5 deleted the cm/refresh-session-on-request branch October 15, 2024 17:26
@lemald lemald mentioned this pull request Dec 5, 2024
4 tasks
digitalcora added a commit to mbta/screens that referenced this pull request Jan 13, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 13, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 13, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 16, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 21, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

Also, when an admin's session has expired and they try to load new data
in the admin UI, they will get a message indicating they need to refresh
the page, instead of the generic "an error occurred".
digitalcora added a commit to mbta/screens that referenced this pull request Jan 21, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

Also, when an admin's session has expired and they try to load new data
in the admin UI, they will get a message indicating they need to refresh
the page, instead of the generic "an error occurred".
digitalcora added a commit to mbta/screens that referenced this pull request Jan 21, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

Also, when an admin's session has expired and they try to load new data
in the admin UI, they will get a message indicating they need to refresh
the page, instead of the generic "an error occurred".
digitalcora added a commit to mbta/screens that referenced this pull request Jan 21, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

Also, when an admin's session has expired and they try to load new data
in the admin UI, they will get a message indicating they need to refresh
the page, instead of the generic "an error occurred".
digitalcora added a commit to mbta/screens that referenced this pull request Jan 23, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

This also revises and unifies how data fetching works in the admin UI,
to allow showing admins a message indicating their session has expired
when that happens (regardless of the interaction), rather than a generic
error message.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 23, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

This also revises and unifies how data fetching works in the admin UI,
to allow showing admins a message indicating their session has expired
when that happens (regardless of the interaction), rather than a generic
error message.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 23, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

This also revises and unifies how data fetching works in the admin UI,
to allow showing admins a message indicating their session has expired
when that happens (regardless of the interaction), rather than a generic
error message.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 24, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

This also revises and unifies how data fetching works in the admin UI,
to allow showing admins a message indicating their session has expired
when that happens (regardless of the interaction), rather than a generic
error message.

In support of the above feature being able to distinguish HTML page
requests from API requests, this _also_ disentangles some pipelines in
the router where some API endpoints were piped through both "browser"
and "api", causing their format to be improperly set as HTML.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 24, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

This also revises and unifies how data fetching works in the admin UI,
to allow showing admins a message indicating their session has expired
when that happens (regardless of the interaction), rather than a generic
error message.

In support of the above feature being able to distinguish HTML page
requests from API requests, this _also_ disentangles some pipelines in
the router where some API endpoints were piped through both "browser"
and "api", causing their format to be improperly set as HTML.
digitalcora added a commit to mbta/screens that referenced this pull request Jan 24, 2025
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

This also revises and unifies how data fetching works in the admin UI,
to allow showing admins a message indicating their session has expired
when that happens (regardless of the interaction), rather than a generic
error message.

In support of the above feature being able to distinguish HTML page
requests from API requests, this _also_ disentangles some pipelines in
the router where some API endpoints were piped through both "browser"
and "api", causing their format to be improperly set as HTML.
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