-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add session-related endpoints #269
Conversation
@@ -76,7 +76,7 @@ http_interactions: | |||
- cloudflare | |||
body: | |||
encoding: ASCII-8BIT | |||
string: '{"user":{"object":"user","id":"user_01H93ZY4F80YZRRS6N59Z2HFVS","email":"test@workos.app","email_verified":false,"first_name":"Lucille","last_name":"Bluth","created_at":"2023-08-30T19:50:13.214Z","updated_at":"2023-08-30T19:50:13.214Z","user_type":"managed","sso_profile_id":"prof_01H93ZTVWYPAT4RKDSPFPPXH0J"}}' | |||
string: '{"user":{"object":"user","id":"user_01H93ZY4F80YZRRS6N59Z2HFVS","email":"test@workos.app","email_verified":false,"first_name":"Lucille","last_name":"Bluth","created_at":"2023-08-30T19:50:13.214Z","updated_at":"2023-08-30T19:50:13.214Z","user_type":"managed","sso_profile_id":"prof_01H93ZTVWYPAT4RKDSPFPPXH0J"},"access_token":"access_token","refresh_token":"refresh_token"}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kind of hate vcr and i manually edited/added all of these responses. this is not technically the right way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i added some code to properly sanitize these.
06e57a3
to
167b246
Compare
167b246
to
94a0fc6
Compare
# The AuthenticationResponse class represents an Authentication Response as well as an corresponding | ||
# response data that can later be appended on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "an corresponding response data that can later be appended on" refer to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhh... this is a complete copy/paste from AuthenticationResponse. i'll come up with better wording 😬
lib/workos/user_management.rb
Outdated
sig do | ||
params( | ||
session_id: String, | ||
).returns(NilClass) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use .void
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh, i knew there must be something like this. will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits but overall LGTM!
config.filter_sensitive_data('<ACCESS_TOKEN>', :token) do |interaction| | ||
JSON.parse(interaction.response.body)['access_token'] | ||
end | ||
config.filter_sensitive_data('<REFRESH_TOKEN>', :token) do |interaction| | ||
JSON.parse(interaction.response.body)['refresh_token'] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if this was the default and test authors didn't need to remember to opt-in, though I don't know the best way to implement that behavior without it also being inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, i agree. i tried just having the handler work without tags but it errors out if you don't return something. (i guess i could return some bogus value?). this seemed to be the "vcr" way to handle this situation.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
==========================================
- Coverage 97.22% 96.79% -0.44%
==========================================
Files 57 58 +1
Lines 1371 1403 +32
==========================================
+ Hits 1333 1358 +25
- Misses 38 45 +7 ☔ View full report in Codecov by Sentry. |
Description
Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.