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

fix bug where other :guardian_ keys are parsed from the conn #476

Merged
merged 2 commits into from
Mar 4, 2018

Conversation

tarzan
Copy link
Contributor

@tarzan tarzan commented Feb 28, 2018

Came accross a problem where other :guardian_ keys where being processed by the Guardian.Plug.Keys module. Other meaning: other than _claims, _token and _resource.

These other keys may, for example, have been put there by another Plug in the pipeline. In particular, this causes problems when working together with guardian_db and revoking sessions.

The test demonstrates this by constructing this:

conn.private == %{
  guardian_default_claims: %{"sub" => "bob", "typ" => "access"},
  guardian_default_resource: %{id: "bob"},
  guardian_default_token: "eyJjbGFpbXMiOnsidHlwIjoiYWNjZXNzIiwic3ViIjoiYm9iIn19",
  guardian_error_handler: Guardian.PlugTest.PipelineImpl.Handler,
  guardian_module: Guardian.PlugTest.PipelineImpl.Impl,
  plug_session: %{
    "guardian_default_token" => "eyJjbGFpbXMiOnsidHlwIjoiYWNjZXNzIiwic3ViIjoiYm9iIn19"
  },
  plug_session_fetch: :done,
  plug_session_info: :renew
}

and then calling Impl.Plug.sign_out which would subsequently not try to clear the key :guardian_error_handler from the session with this fix.

Copy link
Member

@hassox hassox left a comment

Choose a reason for hiding this comment

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

Thanks! Nice catch

@hassox
Copy link
Member

hassox commented Mar 2, 2018

Can you please update the changelog before I merge this.

@tarzan
Copy link
Contributor Author

tarzan commented Mar 4, 2018

Rebased and updated the changelog (added my previous PR to the changelog as well, hope you don't mind).

@hassox
Copy link
Member

hassox commented Mar 4, 2018

Thank you :)

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.

None yet

2 participants