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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support UVerb in servant-auth-server #1571

Merged
merged 6 commits into from Feb 24, 2023
Merged

Support UVerb in servant-auth-server #1571

merged 6 commits into from Feb 24, 2023

Conversation

gdeest
Copy link
Contributor

@gdeest gdeest commented Mar 22, 2022

Closes #1570

Caveat: returning a 401 response with UVerb is not equivalent to using throwAll err401: XSRF-TOKEN will be set anyway, because the handler then doesn't fail (it successfully returns an error 馃檭). I can't see a satisfying solution to this problem, so external input is much welcome on this issue (I for one consider this limitation semi-reasonable). This should be fixed.

@gdeest gdeest mentioned this pull request Mar 22, 2022
@gdeest gdeest force-pushed the servant-auth-uverb branch 4 times, most recently from ca3a35e to b4d9cbe Compare March 23, 2022 11:27
@gdeest gdeest marked this pull request as ready for review March 23, 2022 11:28
@gdeest gdeest changed the title [WIP] Implement AddSetCookieApi for UVerb Support UVerb in servant-auth-server Mar 23, 2022
changelog.d/1571 Outdated Show resolved Hide resolved
Ga毛l Deest and others added 3 commits March 23, 2022 12:49
type instance AddSetCookieApi (Verb method stat ctyps a)
= Verb method stat ctyps (AddSetCookieApiVerb a)
#if MIN_VERSION_servant_server(0,18,1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: this is in fact not sufficient, as UVerb support depends on some instances in servant that are provided by this PR and hence not yet in any released version of servant.

@@ -86,6 +87,8 @@ newtype WithStatus (k :: Nat) a = WithStatus a
instance KnownStatus n => HasStatus (WithStatus n a) where
type StatusOf (WithStatus n a) = n

instance HasStatus a => HasStatus (Headers ls a) where
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: this actually brings support to headers in UVerb responses, AFAICT, which probably deserves a changelog entry.

@tchoutri
Copy link
Contributor

@gdeest Is this ready for review? :)

@gdeest
Copy link
Contributor Author

gdeest commented Mar 26, 2022

@tchoutri Modulo the small comments for myself, yes, it is !

@RileyEv
Copy link

RileyEv commented May 30, 2022

@gdeest @tchoutri Any chance of getting this merged? I'd really appreciate it 馃槃

@tchoutri tchoutri merged commit aee1917 into master Feb 24, 2023
@tchoutri tchoutri deleted the servant-auth-uverb branch February 24, 2023 22:59
@tchoutri
Copy link
Contributor

No idea how I missed that one, sorry!

@robwithhair
Copy link

@tchoutri would you mind recreating the branch servant-auth-uverb? Unfortunately due to the way GitHub squashes commits on merged pull requests this has broken my ability to pull this commit in stack and thus my code won't build any longer. I tried building my code on master but there have been several changes and several dependencies since then. I can fork but not recreate the branch once forked.

@RileyEv
Copy link

RileyEv commented Mar 15, 2023

The branch still exists on my fork, which I made while waiting for this to be merged, feel free to take from there, but don't rely on it for long as I might delete the fork now that this has been merged

https://github.com/bank-manager/servant/tree/servant-auth-uverb

@ysangkok ysangkok mentioned this pull request Jun 22, 2023
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.

UVerb HasServer Instance
5 participants