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

Token from context #176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

FlorianLoch
Copy link
Contributor

@FlorianLoch FlorianLoch commented Mar 4, 2024

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Go Version Update
  • Dependency Update

Description

In order for csrf to play nicely with libraries like huma is seems beneficial to be able to retrieve a token not just from a http.Request but also straight from the context.Context.
This avoids workarounds like creating a dummy request just to pass along the context.
Additionally, it feels more natural to me to pass the actually required value (with is a context.Context) instead of the value wrapped with something else that is not really required for retrieving the token.

Related Tickets & Documents

n. a.

  • Related Issue #
  • Closes #

Added/updated tests?

  • Yes
  • No, and this is why: changes should be covered by existing tests already, adjustment very minor
  • I need help with writing tests

Run verifications and test

  • make verify is passing
  • make test is passing

@FlorianLoch
Copy link
Contributor Author

Requesting review from @gorilla/pull-request-reviewers. :)

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.03%. Comparing base (a009743) to head (b6bb886).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
+ Coverage   90.93%   91.03%   +0.10%     
==========================================
  Files           5        5              
  Lines         353      357       +4     
==========================================
+ Hits          321      325       +4     
  Misses         25       25              
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AlexVulaj
Copy link
Member

@jaitaiwan this looks good to me - mind giving it a quick second look and merging if you're happy with it as well? In summary, it makes it so a token can be fetched directly from a context instead of having to wrap it in an http.Request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants