Allow sdk redirect on settings actions #3827

IniZio · 2024-02-22T03:54:35Z

Initially wanted to apply this derive function on all settings sub-pages. However it will be odd to, for example close the webview on remove an identity in /settings/identity page

pkg/auth/webapp/redirect.go

resources/authgear/templates/en/web/settings_close.html

pkg/auth/handler/webapp/enter_login_id.go

pkg/lib/config/oauth.go

pkg/lib/oauth/handler/resolve.go

IniZio · 2024-02-27T03:01:21Z

@tung2744 @louischan-oursky Updated to reflect latest settings action spec, still adding test cases for new response type and grant type 🙏

pkg/auth/webapp/service2.go

pkg/lib/interaction/nodes/do_ensure_session.go

pkg/lib/interaction/nodes/settings_action_end.go

pkg/lib/oauth/handler/handler_authz.go

IniZio · 2024-02-29T04:43:12Z

Tried to create a stimulus controller that can override history items https://github.com/IniZio/authgear-server/tree/3813-settings-action-redirect-web

The script itself works in other flows, but the consent page will block the history.back(). Tried reauth as well, seems have same issue where pressing back to consent page will show bad request error instead of recovering.

In this pr, I inserted a /settings/close page in history stack instead that redirects user back to original redirect URI.

IniZio · 2024-02-29T10:32:57Z

After discussion, will make web behave similarly as other platforms.

Will guard back-navigation only at consent screen instead. Other screens will still show expired error for now.

Changes:

Update consent screen to hold client_id and redirect_uri. If OAuth session is not found, use redirect_uri instead
Remove x_authgear_sdk_on_finish and close screen

louischan-oursky · 2024-02-29T04:30:26Z

pkg/lib/oauth/oidc/ui.go

+	// Do not set anything.
+	if redirectURI != consentURI {
+		return redirectURI
+	}


Why do we add this?

It's in the original code, was removed in a temp commit for an old approach in this pr. New commit adds it back.

louischan-oursky · 2024-03-01T07:08:02Z

pkg/lib/interaction/intents/change_password.go

+			&nodes.EdgeDoEnsureSession{
+				Mode: nodes.EnsureSessionModeNoop,
+			},
+		}, nil


Why do we need to add EdgeDoEnsureSession here?

Currently it's possible to change password to same as previous value and still return as success. Or should it be treated as fail for settings action?

I think it should be a success, but the developer could enable password history check, which should cause the flow to return api error. That should be handled by password policy already.

Does this work? EdgeDoEnsureSession should be irrelevant here.

case *nodes.NodeChangePasswordEnd: // Password was not changed return []interaction.Edge{&nodes.EdgeSettingsActionEnd{}}, nil case *nodes.NodeDoUpdateAuthenticator: return []interaction.Edge{&nodes.EdgeSettingsActionEnd{}}, nil case *nodes.NodeSettingsActionEnd: // Intent is finished. return nil, nil

Should still need, similar to IntentReauthenticate. Tried removing it results in invalid authentication required

pkg/lib/oauth/handler/handler_authz.go

louischan-oursky · 2024-03-01T07:21:55Z

pkg/lib/oauth/handler/handler_authz.go

@@ -398,7 +428,7 @@ func (h *AuthorizationHandler) doHandle(
 	// Handle prompt!=none
 	// We must return here.
 	if !slice.ContainsString(uiInfo.Prompt, "none") {
-		endpoint, err := h.UIURLBuilder.Build(client, r, oauthSessionEntry)
+		endpoint, err := h.UIURLBuilder.BuildAuthenticationURL(client, r, oauthSessionEntry)


We will remove parseAuthzRedirectURI, and in here, we will choose between BuildAuthenticationURL and BuildSettingsActionURL. In doing this, we preserve the original code flow, and let the reader to reason the code easier.

pkg/lib/oauth/handler/handler_authz.go

louischan-oursky · 2024-03-05T07:08:10Z

pkg/lib/interaction/intents/change_password.go

+			&nodes.EdgeDoEnsureSession{
+				Mode: nodes.EnsureSessionModeNoop,
+			},
+		}, nil


Does this work? EdgeDoEnsureSession should be irrelevant here.

case *nodes.NodeChangePasswordEnd: // Password was not changed return []interaction.Edge{&nodes.EdgeSettingsActionEnd{}}, nil case *nodes.NodeDoUpdateAuthenticator: return []interaction.Edge{&nodes.EdgeSettingsActionEnd{}}, nil case *nodes.NodeSettingsActionEnd: // Intent is finished. return nil, nil

pkg/lib/oauth/handler/handler_authz.go

…se type authgear#3813

We now use implicitly whitelisted response types instead of client config

Return to user app if user visits expired oauth pages e.g. oauth consent. It mostly likely happens when users refresh on expired oauth page or go back after oauth flow has completed e.g. settings action.

…ar#3813

IniZio requested review from louischan-oursky and tung2744 February 22, 2024 03:54

IniZio assigned tung2744 Feb 22, 2024

tung2744 reviewed Feb 22, 2024

View reviewed changes

pkg/auth/webapp/redirect.go Outdated Show resolved Hide resolved

IniZio force-pushed the 3813-settings-action-redirect branch 3 times, most recently from 2cc5ce1 to aa81537 Compare February 23, 2024 06:21

louischan-oursky reviewed Feb 23, 2024

View reviewed changes

resources/authgear/templates/en/web/settings_close.html Outdated Show resolved Hide resolved

resources/authgear/templates/en/web/settings_close.html Outdated Show resolved Hide resolved

IniZio force-pushed the 3813-settings-action-redirect branch 4 times, most recently from db703db to 0bf3a3b Compare February 23, 2024 15:20

louischan-oursky self-requested a review February 26, 2024 03:50

louischan-oursky reviewed Feb 26, 2024

View reviewed changes

IniZio force-pushed the 3813-settings-action-redirect branch 5 times, most recently from 84afb58 to a55babf Compare February 27, 2024 13:34

IniZio changed the title ~~Allow sdk redirect on settings actions~~ [WIP] Allow sdk redirect on settings actions Feb 28, 2024

IniZio force-pushed the 3813-settings-action-redirect branch from a55babf to 7dbc9fe Compare February 28, 2024 03:32

IniZio changed the title ~~[WIP] Allow sdk redirect on settings actions~~ Allow sdk redirect on settings actions Feb 28, 2024

IniZio force-pushed the 3813-settings-action-redirect branch 2 times, most recently from efb78ef to 8a6c22a Compare February 28, 2024 04:04

louischan-oursky reviewed Feb 28, 2024

View reviewed changes

IniZio force-pushed the 3813-settings-action-redirect branch 4 times, most recently from cac1bb5 to 70da13b Compare February 28, 2024 08:52

IniZio force-pushed the 3813-settings-action-redirect branch 4 times, most recently from 211c791 to 28cefe1 Compare February 29, 2024 04:38

IniZio force-pushed the 3813-settings-action-redirect branch from 6d3832f to ba267df Compare February 29, 2024 05:59

IniZio force-pushed the 3813-settings-action-redirect branch from ba267df to 4e7b9b0 Compare February 29, 2024 16:13

louischan-oursky reviewed Mar 1, 2024

View reviewed changes

IniZio changed the title ~~Allow sdk redirect on settings actions~~ [WIP] Allow sdk redirect on settings actions Mar 4, 2024

IniZio force-pushed the 3813-settings-action-redirect branch from 2fbd47d to a82f704 Compare March 4, 2024 07:08

IniZio changed the title ~~[WIP] Allow sdk redirect on settings actions~~ Allow sdk redirect on settings actions Mar 4, 2024

louischan-oursky reviewed Mar 5, 2024

View reviewed changes

IniZio and others added 12 commits March 6, 2024 15:18

Allow sdk redirect on settings actions authgear#3813

4fb7a11

Handle new settings_action response type and grant type authgear#3813

45f7813

Remove x_settings_redirect_uri_origins setting authgear#3813

908fc99

Redirect settings action to consent and update settings action respon…

bd334fb

…se type authgear#3813

Remove outdated response_type test case authgear#3813

19edf45

We now use implicitly whitelisted response types instead of client config

Redirect on oauth session expired authgear#3813

637993c

Return to user app if user visits expired oauth pages e.g. oauth consent. It mostly likely happens when users refresh on expired oauth page or go back after oauth flow has completed e.g. settings action.

Remove client config usage for response types whitelist authgear#3813

6bf810f

Remove variable shadowing for oauth error response return authgear#3813

b7fde1e

Remove parseAuthzRedirectURI function authgear#3813

48b53c8

Use processed consent request for invalid oauth error response authge…

3fc2951

…ar#3813

Add a note on why EdgeDoEnsureSession is needed

3fbd8b5

Revert to use client_id and redirect_uri in consent URL

dadf4dd

louischan-oursky force-pushed the 3813-settings-action-redirect branch from d1fe865 to dadf4dd Compare March 6, 2024 07:18

louischan-oursky added 2 commits March 6, 2024 15:56

Fix code is not added to consent URL

217d4f0

Fix *url.URL generated by Endpoints has no leading /

946ef95

louischan-oursky merged commit 09acbe2 into authgear:main Mar 7, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow sdk redirect on settings actions #3827

Allow sdk redirect on settings actions #3827

IniZio commented Feb 22, 2024

IniZio commented Feb 27, 2024

IniZio commented Feb 29, 2024 •

edited

Loading

IniZio commented Feb 29, 2024 •

edited

Loading

louischan-oursky Feb 29, 2024

IniZio Mar 4, 2024 •

edited

Loading

louischan-oursky Mar 1, 2024

IniZio Mar 4, 2024 •

edited

Loading

tung2744 Mar 4, 2024

louischan-oursky Mar 5, 2024

IniZio Mar 5, 2024

louischan-oursky Mar 1, 2024

louischan-oursky Mar 5, 2024

Allow sdk redirect on settings actions #3827

Allow sdk redirect on settings actions #3827

Conversation

IniZio commented Feb 22, 2024

IniZio commented Feb 27, 2024

IniZio commented Feb 29, 2024 • edited Loading

IniZio commented Feb 29, 2024 • edited Loading

louischan-oursky Feb 29, 2024

Choose a reason for hiding this comment

IniZio Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

louischan-oursky Mar 1, 2024

Choose a reason for hiding this comment

IniZio Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

tung2744 Mar 4, 2024

Choose a reason for hiding this comment

louischan-oursky Mar 5, 2024

Choose a reason for hiding this comment

IniZio Mar 5, 2024

Choose a reason for hiding this comment

louischan-oursky Mar 1, 2024

Choose a reason for hiding this comment

louischan-oursky Mar 5, 2024

Choose a reason for hiding this comment

IniZio commented Feb 29, 2024 •

edited

Loading

IniZio commented Feb 29, 2024 •

edited

Loading

IniZio Mar 4, 2024 •

edited

Loading

IniZio Mar 4, 2024 •

edited

Loading