[FS-736] ACME Create DPoP Access Token by battermann · Pull Request #2652 · wireapp/wire-server

battermann · 2022-08-25T15:03:18Z

https://wearezeta.atlassian.net/browse/FS-736

Since this is just the "skeleton" implementation for this feature, and the rust library this feature depends on is not finished either, documentation, exhaustive tests, and release notes will be in a separate PR.

Checklist

The PR Title explains the impact of the change.
The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
If HTTP endpoint paths have been added or renamed, check docs/developer/adding-api-endpoints and follow the steps there.
If configuration options have been added or removed, check docs/developer/adding-config-options and follow the steps there.
I swear that if I have changed internal end-points, I do not implicitly rely on deployment ordering (brig needing to be deployed before galley), i.e. I have not introduced a new internal endpoint in brig and already make use of if in galley, as I'm aware old deployed galleys would throw 500s until all new version have been rolled out and I do not want a few minutes of downtime. Instead, I have thought how to merge brig an galley codebases.
I updated changelog.d subsections with one or more entries with the following bits of information (details)

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

libs/wire-api/src/Wire/API/User/Client/DPoPAccessToken.hs

services/brig/test/integration/API/User/Client.hs

services/brig/src/Brig/API/Public.hs

services/brig/src/Brig/API/Error.hs

fisx · 2022-09-28T10:17:27Z

re 3226c9c: this isn't done, but it should show you what i want to do differently:

there should be one haskell enum type that maps all the error codes that can be returned from rust.
(what i haven't done yet) there should probably also some data SuperError = RustError DPoPTokenGenerationError | OtherSuperError | YetanotherSuperError, where DPoPTokenGenerationError is fully isomorphic to the rust number range (no collapsing of other errors into FfiError).
{from,to}Enum should be used instead of manual enumeration.

I don't insist on this change, but it revealed to me that the error types in haskell do not map to the errors in rust very cleanly, so at least that part I would like to see fixed.

This reverts commit 3226c9c.

fisx

I've reverted my last commit to make this compile again. please cherry-pick it if you decide it's useful.

Also, more comments. :)

fisx · 2022-09-28T10:22:28Z

libs/jwt-tools/src/Data/Jwt/Tools.hs

+    toCStr = liftIO . newCString . toStr
+      where
+        toStr :: a -> String
+        toStr = cs . toByteString'


newCString takes a String?! that's horrible. :')

but ok, then this is fine i suppose... you could have also left things inline and written (cs @ByteString @String) instead of cs.

fisx · 2022-09-28T10:24:22Z

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

  fromUnion (Z (I ())) = Nothing
  fromUnion (S (Z (I (DeletionCodeTimeout t)))) = Just t
-  fromUnion (S (S x)) = case x of {}
+  fromUnion (S (S x)) = case x of


Is this because x :: Void? can't you just skip this case, then? (it's nice to have it there explicitly, just trying to understand.)

Annoyingly, this change was made by my code formatter, but it does not match the formatting that is expected on CI. I reverted the change.

…me-be-create-dpop-access-token

fisx

I added two more commits and merged develop. Please take another look, but otherwise LGTM!

jschaul · 2022-10-06T15:42:14Z

libs/wire-api/src/Wire/API/Asset.hs


-instance AsHeaders '[AssetLocation r] Asset (Asset, AssetLocation r) where
-  toHeaders (asset, loc) = (I loc :* Nil, asset)
-  fromHeaders (I loc :* Nil, asset) = (asset, loc)


What's the motivation for this removal that appears unrelated to this PR?

In this PR a generic version of this is introduced so this instance is obsolete.

battermann temporarily deployed to cachix August 25, 2022 15:03 Inactive

battermann temporarily deployed to cachix August 26, 2022 10:59 Inactive

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 26, 2022

battermann temporarily deployed to cachix August 26, 2022 11:29 Inactive

battermann temporarily deployed to cachix August 26, 2022 11:35 Inactive

battermann temporarily deployed to cachix August 26, 2022 11:38 Inactive

fisx reviewed Aug 26, 2022

View reviewed changes

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs Outdated Show resolved Hide resolved

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs Outdated Show resolved Hide resolved

fisx reviewed Aug 26, 2022

View reviewed changes

battermann force-pushed the SQSERVICES-1722-acme-be-create-dpop-access-token branch from 6eb8d97 to 44309f1 Compare August 29, 2022 11:21

battermann temporarily deployed to cachix August 29, 2022 11:21 Inactive

battermann temporarily deployed to cachix August 29, 2022 11:22 Inactive

battermann temporarily deployed to cachix September 1, 2022 13:20 Inactive

battermann temporarily deployed to cachix September 1, 2022 13:27 Inactive

battermann force-pushed the SQSERVICES-1722-acme-be-create-dpop-access-token branch from f465672 to 501ef68 Compare September 5, 2022 14:46

battermann temporarily deployed to cachix September 5, 2022 14:46 Inactive

battermann temporarily deployed to cachix September 6, 2022 11:43 Inactive

battermann temporarily deployed to cachix September 6, 2022 13:03 Inactive

battermann force-pushed the SQSERVICES-1722-acme-be-create-dpop-access-token branch from 6e4d7f6 to 95ff755 Compare September 7, 2022 10:59

battermann temporarily deployed to cachix September 28, 2022 09:05 Inactive

battermann requested a review from fisx September 28, 2022 09:08

format

0e7e9d8

battermann temporarily deployed to cachix September 28, 2022 09:09 Inactive

cleanup (?)

3226c9c

fisx temporarily deployed to cachix September 28, 2022 10:12 Inactive

Revert "cleanup (?)"

23a74cd

This reverts commit 3226c9c.

fisx temporarily deployed to cachix September 28, 2022 10:27 Inactive

fisx reviewed Sep 28, 2022

View reviewed changes

battermann added 2 commits September 28, 2022 11:07

test for toResult

c2f604c

using bounded enum for error mapping

f8aa6a9

battermann temporarily deployed to cachix September 28, 2022 12:10 Inactive

battermann requested a review from fisx September 28, 2022 12:15

fisx added 3 commits September 28, 2022 14:24

Merge remote-tracking branch 'origin/develop' into SQSERVICES-1722-ac…

0322bd0

…me-be-create-dpop-access-token

whitespace.

6f55e11

Add a note of warning to the swagger description.

3c51a60

fisx temporarily deployed to cachix September 28, 2022 12:38 Inactive

fisx approved these changes Sep 28, 2022

View reviewed changes

battermann merged commit 4ad1b44 into develop Sep 28, 2022

battermann deleted the SQSERVICES-1722-acme-be-create-dpop-access-token branch September 28, 2022 15:18

zebot mentioned this pull request Oct 4, 2022

Release 2022-10-04 - (expected chart version 4.25.0) #2746

Merged

jschaul reviewed Oct 6, 2022

View reviewed changes

fisx mentioned this pull request Mar 10, 2023

[SQSERVICES-1942] Fix DPoP access token error propagation (2/2) #3142

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[FS-736] ACME Create DPoP Access Token#2652

[FS-736] ACME Create DPoP Access Token#2652
battermann merged 73 commits intodevelopfrom
SQSERVICES-1722-acme-be-create-dpop-access-token

battermann commented Aug 25, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fisx commented Sep 28, 2022

Uh oh!

fisx left a comment

Uh oh!

fisx Sep 28, 2022

Uh oh!

fisx Sep 28, 2022

Uh oh!

battermann Sep 28, 2022

Uh oh!

fisx left a comment

Uh oh!

jschaul Oct 6, 2022

Uh oh!

battermann Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

Conversation

battermann commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fisx commented Sep 28, 2022

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

fisx Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

fisx Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

battermann Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

jschaul Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

battermann Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

battermann commented Aug 25, 2022 •

edited

Loading