feat: implement blocking web hooks #1585

harnash · 2021-07-23T09:28:16Z

This change allows webhooks to control the registration flow by either allowing process to continue or fail with an errors propagated to the UI (mostly validation errors at this point). This is not yet complete but the general idea is here to start any discussions and resolve concerns early.

TODO:

tests
documentation update

Closes #1724

Related issue(s)

#1483

Checklist

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

codecov · 2021-07-23T13:24:07Z

Codecov Report

Merging #1585 (c602790) into master (d8dea01) will increase coverage by 0.02%.
The diff coverage is 67.16%.

❗ Current head c602790 differs from pull request most recent head 63e5706. Consider uploading reports for the commit 63e5706 to get more accurate results

@@            Coverage Diff             @@
##           master    #1585      +/-   ##
==========================================
+ Coverage   75.29%   75.31%   +0.02%     
==========================================
  Files         295      295              
  Lines       16453    16592     +139     
==========================================
+ Hits        12388    12496     +108     
- Misses       3118     3149      +31     
  Partials      947      947

Impacted Files	Coverage Δ
selfservice/flow/flow.go	`100.00% <ø> (ø)`
selfservice/flow/login/flow.go	`94.20% <0.00%> (-2.82%)`	⬇️
selfservice/flow/registration/flow.go	`96.72% <0.00%> (-3.28%)`	⬇️
selfservice/flow/registration/hook.go	`79.62% <0.00%> (-4.69%)`	⬇️
selfservice/flow/settings/flow.go	`91.54% <0.00%> (-2.66%)`	⬇️
selfservice/flow/settings/hook.go	`58.86% <0.00%> (-13.95%)`	⬇️
selfservice/strategy/oidc/strategy_login.go	`68.91% <0.00%> (ø)`
ui/container/container.go	`85.82% <58.33%> (-2.87%)`	⬇️
selfservice/flow/login/handler.go	`78.40% <75.00%> (-0.08%)`	⬇️
selfservice/flow/login/hook.go	`87.50% <80.00%> (-1.64%)`	⬇️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

aeneasr · 2021-07-23T14:48:50Z

Thank you for the PR! I will review it next week!

harnash · 2021-07-23T15:42:29Z

Thank you for the PR! I will review it next week!

No hurry there. I'm off next week so effectively I can resume work on this in two weeks more or less.

aeneasr · 2021-07-30T09:09:38Z

Could you please enable write access for maintainers? 🤔

Thank you! 🙏

aeneasr · 2021-07-30T09:09:55Z

Alternatively, given that this is an org repo, you can also invite me as a collaborator to the fork

aeneasr

Thank you for the PR! This is going in the right direction :)

I have merged the conflicts (as we have changed something in the hooks) which I pushed to a separate branch and made a PR against your fork: Wikia#14

High-level this looks good. To see it in action, we will need some tests. I am in particular interested to see how the error parsing will be working in combination with the UI messages / fields.

We also need to make some decisions on how Ory Kratos detects whether a hook wants to return validation errors or not. I also think we need to rename the feature to better match what it does.

Define a better name for the feature in the config
Come up with a definition how a web hook could tell Ory Kratos that we have a validation error going on
Add functional tests for web hook
Add unit tests for the new helper structs
Add documentation on how to return validation errors for hooks
Add e2e test that tests this feature from start to finish
Remove credentials from hooks

driver/config/.schema/config.schema.json

schema/errors.go

ui/container/container.go

selfservice/hook/web_hook.go

dadrus · 2021-08-03T18:38:23Z

When I was implementing the web hooks, my extension idea was to let the response be verified in a yet another jsonet template to which, as with building of the web hook request, a couple of objects can be passed in, like the response and e.g. the actuall flow objekt. This way the protocol of the web hook is fully open and one can achieve much more compared with the implementation in this pr.
The aforesaid object can e.g. initially contain just one method, to e.g. achieve the behaviour of the implementation given by this PR. Later on one could extend it to support other use cases. What do you think?

harnash · 2021-08-03T18:53:30Z

When I was implementing the web hooks, my extension idea was to let the response be verified in a yet another jsonet template to which, as with building of the web hook request, a couple of objects can be passed in, like the response and the actuall flow objekt. This way the protocol of the web hook is fully open and one can achieve much more compared with the implementation in this pr. What do you think?

Yes I was thinking about this but I am not sure how we would properly propagate those responses up to the UI which requires some concrete structures/data to properly handle validation errors etc. (see: https://github.com/ory/kratos/blob/master/ui/container/container.go#L152). It would be great to come up with some flexible solution that would allow to validate response using Jsonnet and transform it into something Kratos can use. For the scenarion where we have fixed data structures in Kratos and create custom payload for the webhooks Jsonnet works nicely providing the glue betwen Kratos and external services for the other way around it is a bit trickier. There must be some binding point. I will think about it more but right now I have no good solution.

dadrus · 2021-08-03T19:02:51Z

I think this can remain as is implemented by this PR. I mean the implementation right now expects a message with some text in the web hook response. Why not just letting a jennet template call a function on the passed context object, like e.g. ctx.abortFlow("some error description") ."some error description" can then come from the payload received from the Web hook or be just set in the jsonet template based on some conditions which are up to the developer of that template. If there is no error condition, the call to the above flow will not happen. When the template execution is done the remaining code will have everything which is required (information whether to proceed or to abort). The above said function can e.g. have further arguments to let the jsonet developer indicate whether the error is a validation error or not. I think there are a couple of options in this regard. Or do I overlook something?

dadrus · 2021-08-03T19:14:36Z

If you like I can contribute some lines of code 😉 so you can directly see if it fits.

aeneasr · 2021-08-04T09:00:31Z

JsonNet is not really a programming language that executes things. It's for manipulating JSON! Adding functionality like ctx.abortFlow will not work because the control flow is not aborted - you still need to return some JSON. I also think it is not a good idea to have this (potentially untrusted) code be able to call into the internal API of Ory Kratos (as a general pattern, not particularly for this idea).

Keep in mind that regular code is much easier to program, test, and document. JsonNet is not that great when trying to test it. The less logic we have in JsonNet, the better!

aeneasr · 2021-08-23T12:18:58Z

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

harnash · 2021-08-23T12:24:56Z

OK. I had some interruptions lately and I wasn't able to devote much time to this but hopefully I can resume work on this soon.

aeneasr · 2021-08-23T15:06:15Z

No rush! Unmerged PRs give me a bit of anxiety so I tend to try to keep the backlog short 😅

aeneasr · 2021-09-11T07:14:07Z

When this is good for review, mark it as ready!

harnash · 2021-09-30T18:41:04Z

I've just added support for maintaining form/flow data when hook returns error. Only tests to go now I think.

aeneasr · 2021-10-12T05:50:46Z

Let me know when I should take another look :)

hperl · 2022-06-06T09:52:21Z

Thanks for the ping, I'll take a look tomorrow!

hperl · 2022-06-07T14:56:49Z

@harnash thanks for your changes, LGTM!
@aeneasr do you want to take another look as well?

aeneasr · 2022-06-07T16:18:28Z

embedx/config.schema.json

@@ -208,6 +208,11 @@
                }
              ]
            },
+            "can_interrupt": {


What happens if we set this value and response.ignore both to true? Should they be mutually exclusive?

Will take a look.

Not an JSON schema expert but I think I got this working: 98d38c7

aeneasr · 2022-06-07T16:25:33Z

All in all this looks very, very, very good :) I will need to try it out end-to-end, but I don't think that there will be big topics outside of: #1585 (comment) :) :)

harnash · 2022-06-10T10:33:45Z

I think I've addressed all comments @aeneasr + @hperl . We are going to try migrate to v0.10.x in few days and if you say it is in mergable state I will try to incorporate this into our branch and test it on DEV env to see if there are some unexpected issues.

… the same time

aeneasr · 2022-06-20T08:47:26Z

I think I've addressed all comments @aeneasr + @hperl . We are going to try migrate to v0.10.x in few days and if you say it is in mergable state I will try to incorporate this into our branch and test it on DEV env to see if there are some unexpected issues.

Do you have an update on this? :)

harnash · 2022-06-20T12:08:57Z

I think I've addressed all comments @aeneasr + @hperl . We are going to try migrate to v0.10.x in few days and if you say it is in mergable state I will try to incorporate this into our branch and test it on DEV env to see if there are some unexpected issues.

Do you have an update on this? :)

Sadly migration to v0.10 is being postponed a bit. Will let you know as soon as I know more. Currently we have this in backlog and planned for next sprint (~ two weeks).

aeneasr · 2022-06-21T14:36:31Z

Ok, no problem :) I think the code looks good so I would be fine merging it after I do one final round of review

…ooks # Conflicts: # test/e2e/modd.conf # test/e2e/run.sh

aeneasr

It's perfect. Thank you!

I've made two minor changes: I removed the message string and renamed detailed_messages to messages. The reason being that message did not actually have an effect and it was a bit confusing why we need it.

The second thing was that there was a connection leak in the webhook handler as resp.Body.Close() wasn't being called. This is fixed now :)

I've updated the docs also

Overall, great achievement!

…tream_blocking_hooks

harnash · 2022-07-21T21:22:54Z

Thanks!
We are still working on upgrade to v0.10.1 but I think we could test this build soon enough.

feat: implement blocking webhooks (ory#1585) This feature allows webhooks to return validation errors in the registration and login flow from a webhook. This feature enables you to deny sign-ups from a specific domain, for example. A big thank you goes out to the team at Wikia / Fandom for implementing and contributing to this feature! Closes ory#1724 Closes ory#1483

aeneasr requested changes Jul 30, 2021

View reviewed changes

aeneasr added this to the v0.8.0-alpha.1 milestone Aug 1, 2021

harnash requested a review from zepatrik as a code owner August 3, 2021 12:46

harnash force-pushed the upstream_blocking_hooks branch from d32a43a to 6b4c110 Compare August 3, 2021 17:15

aeneasr marked this pull request as draft August 23, 2021 12:19

aeneasr mentioned this pull request Aug 30, 2021

Allow to run non-blocking web-hook #1702

Closed

harnash mentioned this pull request Sep 8, 2021

Setting up a Criteria for self-service registration #1724

Closed

harnash force-pushed the upstream_blocking_hooks branch 3 times, most recently from a4ad5c4 to 035fbaf Compare September 17, 2021 20:42

harnash force-pushed the upstream_blocking_hooks branch 4 times, most recently from 58b057a to d7400b8 Compare September 30, 2021 20:08

hperl previously approved these changes Jun 7, 2022

View reviewed changes

Merge branch 'master' into upstream_blocking_hooks

63165cc

aeneasr reviewed Jun 7, 2022

View reviewed changes

harnash dismissed hperl’s stale review via 97514c2 June 10, 2022 10:30

harnash force-pushed the upstream_blocking_hooks branch from 0ecde72 to 70be256 Compare June 10, 2022 11:42

schema: do not allow for webhooks to be interruptable and blocking at…

98d38c7

… the same time

harnash force-pushed the upstream_blocking_hooks branch from 70be256 to 98d38c7 Compare June 10, 2022 11:43

Merge branch 'master' into upstream_blocking_hooks

0a8523c

aeneasr mentioned this pull request Jun 17, 2022

User invites flow #2525

Open

6 tasks

ory-bot and others added 2 commits July 21, 2022 15:00

Merge remote-tracking branch 'origin/master' into upstream_blocking_h…

1e15019

…ooks # Conflicts: # test/e2e/modd.conf # test/e2e/run.sh

chore: fix minor issues and connection leak

aa67d6f

aeneasr previously approved these changes Jul 21, 2022

View reviewed changes

aeneasr dismissed their stale review via 1b95ba8 July 21, 2022 18:07

u

c602790

aeneasr force-pushed the upstream_blocking_hooks branch from 1b95ba8 to c602790 Compare July 21, 2022 19:31

aeneasr added 2 commits July 21, 2022 23:20

u

b1c3aca

Merge remote-tracking branch 'Wikia/upstream_blocking_hooks' into ups…

d71e58d

…tream_blocking_hooks

aeneasr added 2 commits July 22, 2022 08:16

fix: retries and fast argon2

b153f2b

Merge branch 'master' into upstream_blocking_hooks

63e5706

aeneasr merged commit e48e9fa into ory:master Jul 22, 2022

harnash deleted the upstream_blocking_hooks branch July 22, 2022 15:36

feat: implement blocking web hooks #1585

feat: implement blocking web hooks #1585

Uh oh!

Conversation

harnash commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue(s)

Checklist

Uh oh!

codecov bot commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aeneasr commented Jul 23, 2021

Uh oh!

harnash commented Jul 23, 2021

Uh oh!

aeneasr commented Jul 30, 2021

Uh oh!

aeneasr commented Jul 30, 2021

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dadrus commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harnash commented Aug 3, 2021

Uh oh!

dadrus commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dadrus commented Aug 3, 2021

Uh oh!

aeneasr commented Aug 4, 2021

Uh oh!

aeneasr commented Aug 23, 2021

Uh oh!

harnash commented Aug 23, 2021

Uh oh!

aeneasr commented Aug 23, 2021

Uh oh!

aeneasr commented Sep 11, 2021

Uh oh!

harnash commented Sep 30, 2021

Uh oh!

aeneasr commented Oct 12, 2021

Uh oh!

hperl commented Jun 6, 2022

Uh oh!

hperl commented Jun 7, 2022

Uh oh!

aeneasr Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

harnash Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

harnash Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeneasr commented Jun 7, 2022

Uh oh!

harnash commented Jun 10, 2022

Uh oh!

aeneasr commented Jun 20, 2022

Uh oh!

harnash commented Jun 20, 2022

Uh oh!

harnash commented Jul 23, 2021 •

edited

Loading

codecov bot commented Jul 23, 2021 •

edited

Loading

dadrus commented Aug 3, 2021 •

edited

Loading

dadrus commented Aug 3, 2021 •

edited

Loading

harnash Jun 10, 2022 •

edited

Loading