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

Read Set-Cookie header options from Session plug's options #187

Conversation

sylph01
Copy link
Contributor

@sylph01 sylph01 commented Dec 8, 2021

Rationale

This is related to #186 (specifically "Add an interface to Antikythera.Plug.Session.load/2"), but it stems from a different purpose, where we needed to explicitly set the expiration period of the session cookie.
This enables the gear to specify the behavior of the session cookie via the keyword list passed to the plug.

Part that I may need help on

  • Type signature of load/2 's opts variable. The keyword list should accept a map (which is passed to make_before_send/3), but this specification might be too broad
    • Note: I have ran mix dialyzer and got no warnings
  • Are there any test codes relating to this specific code?
  • What to do if store is not a cookie?

lib/web/plug/session.ex Outdated Show resolved Hide resolved
lib/web/plug/session.ex Outdated Show resolved Hide resolved
@irisTa56
Copy link
Contributor

irisTa56 commented Dec 8, 2021

Are there any test codes relating to this specific code?

I couldn't find such test in this repository...
but I found related tests here in testgear repository.

What to do if store is not a cookie?

I'm not sure because I don't know the detailed requirement, but
can it be solved by adding a new store module which implements Antikythera.Session.Store?

lib/web/plug/session.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@irisTa56 irisTa56 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@Fumipo-Theta Fumipo-Theta left a comment

Choose a reason for hiding this comment

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

LGTM!

@aYukiSekiguchi
Copy link
Contributor

Are there any test codes relating to this specific code?

I couldn't find such test in this repository...
but I found related tests here in testgear repository.

I don't think you need to write test code for testgear, but I want to confirm that you manually tested this.

@aYukiSekiguchi
Copy link
Contributor

What to do if store is not a cookie?

I'm not sure what Uses cookie store by default means. However, it looks like session data is always stored in cookie, and Antikythera.Session.Cookie just encodes/decodes data to/from JSON. Therefore, your set_cookie_opts looks correct approach.

Copy link
Contributor

@aYukiSekiguchi aYukiSekiguchi left a comment

Choose a reason for hiding this comment

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

LGTM if you have tested this PR

@sylph01
Copy link
Contributor Author

sylph01 commented Jan 14, 2022

please wait while I prepare some evidence 🙇

Copy link
Contributor

@aMasakiTakahashi aMasakiTakahashi left a comment

Choose a reason for hiding this comment

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

LGTM!

@sylph01
Copy link
Contributor Author

sylph01 commented Jan 21, 2022

I got some evidence of how this works.

Prerequisites

  • antikythera_dep = {:antikythera, [github: "sylph01/antikythera", ref: "2479a8d8cad17c63d000782b5c0e432ade970aca"]} in antikythera_instance_example 's mix.exs
    • the commit hash points to the HEAD of this branch
  • then in local testgear, do following to compile/run with specified version of antikythera (change path to your environment accordingly):
ANTIKYTHERA_INSTANCE_DEP='{:antikythera_instance_example, [git: "/home/sylph01/work/antikythera_instance_example"]}' mix deps.get
ANTIKYTHERA_INSTANCE_DEP='{:antikythera_instance_example, [git: "/home/sylph01/work/antikythera_instance_example"]}' mix compile
ANTIKYTHERA_INSTANCE_DEP='{:antikythera_instance_example, [git: "/home/sylph01/work/antikythera_instance_example"]}' iex -S mix

Actual evidence

In testgear's /web/controller/session.ex, I changed the plug line to add max_age option:

Screenshot_20220121_153507

Before

Screenshot_20220121_153134

After

Screenshot_20220121_153305

Note that the new Set-Cookie header has the Max-Age value.

@aYukiSekiguchi
Copy link
Contributor

Great! Thank you!

@aYukiSekiguchi aYukiSekiguchi merged commit d1148d2 into access-company:master Jan 21, 2022
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.

5 participants