Skip to content

[WPB-21432] app refresh cookie requires password#5129

Merged
fisx merged 6 commits intodevelopfrom
WPB-21432-app-refresh-cookie-requires-password
Mar 20, 2026
Merged

[WPB-21432] app refresh cookie requires password#5129
fisx merged 6 commits intodevelopfrom
WPB-21432-app-refresh-cookie-requires-password

Conversation

@fisx
Copy link
Contributor

@fisx fisx commented Mar 18, 2026

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@fisx fisx requested review from a team as code owners March 18, 2026 09:00
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 18, 2026
@fisx fisx requested a review from battermann March 18, 2026 16:20
@fisx fisx requested a review from battermann March 19, 2026 15:19
Copy link
Contributor

@battermann battermann left a comment

Choose a reason for hiding this comment

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

The swagger docs should to be changed, otherwise good to go.
Approved proforma.

refreshAppCookie u tid appId mbBody = do
req <- baseRequest u Brig Versioned $ joinHttpPath ["teams", tid, "apps", appId, "cookies"]
submit "POST" req
submit "POST" $ req & maybe id addJSON mbBody
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a super nitpick, but I don't think we need to test the case without the body. That is why we use servant, right?
Ideally I would pass a Maybe String and then put it in the body, if it is Just, otherwise "password": null or empty JSON object.

Copy link
Contributor Author

@fisx fisx Mar 20, 2026

Choose a reason for hiding this comment

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

i'm confused now, so i'm going to ignore it :)

Co-authored-by: Leif Battermann <leif.battermann@wire.com>
Comment on lines +1269 to +1272
refreshAppCookie :: (MakesValue u) => u -> String -> String -> Maybe Value -> App Response
refreshAppCookie u tid appId mbBody = do
req <- baseRequest u Brig Versioned $ joinHttpPath ["teams", tid, "apps", appId, "cookies"]
submit "POST" req
submit "POST" $ req & maybe id addJSON mbBody
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant something along these lines:

Suggested change
refreshAppCookie :: (MakesValue u) => u -> String -> String -> Maybe Value -> App Response
refreshAppCookie u tid appId mbBody = do
req <- baseRequest u Brig Versioned $ joinHttpPath ["teams", tid, "apps", appId, "cookies"]
submit "POST" req
submit "POST" $ req & maybe id addJSON mbBody
refreshAppCookie :: (MakesValue u) => u -> String -> String -> Maybe String -> App Response
refreshAppCookie u tid appId mPassword = do
req <- baseRequest u Brig Versioned $ joinHttpPath ["teams", tid, "apps", appId, "cookies"]
submit "POST" $ req & addJSONObject ["password" .= maybe Aeson.Null Aeson.encode mPassword ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hum, maybe i'm not confused but opposed? i had it this way before, but wanted to be more flexible in what we send, to test more corner cases.

thanks, but still ignoring you :)

@fisx fisx merged commit 7c01099 into develop Mar 20, 2026
10 checks passed
@fisx fisx deleted the WPB-21432-app-refresh-cookie-requires-password branch March 20, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants