feat(selfservice/login): enable reauthentication functionality #248

zepatrik · 2020-02-14T14:31:35Z

Related issue

contributes to #243

Proposed changes

Checklist

I have read the contributing 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 security@ory.sh) 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 necessary documentation within the code base (if appropriate)
I have documented my changes in the developer guide (if appropriate)

Further comments

# Conflicts: # selfservice/flow/login/handler_test.go # selfservice/strategy/password/login_test.go

persistence/sql/persister_login.go

…auth-endpoint

selfservice/flow/login/handler.go

# Conflicts: # persistence/sql/persister_login.go

aeneasr · 2020-02-19T07:38:52Z

selfservice/flow/login/handler.go

@@ -110,6 +111,12 @@ func (h *Handler) NewLoginRequest(w http.ResponseWriter, r *http.Request, redir
 //       500: genericError
 func (h *Handler) initLoginRequest(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
 	if err := h.NewLoginRequest(w, r, func(a *Request) (string, error) {
+		// we assume an error means the user has no session
+		if _, err := h.d.SessionManager().FetchFromRequest(r.Context(), w, r); err == nil && r.URL.Query().Get("prompt") == "true" {


The && doesn't make sense here. Please add a failing test case first.

aeneasr · 2020-02-19T07:39:50Z

selfservice/flow/login/handler_test.go

-	}
+	t.Run("does set reauth flag on authenticated request", func(t *testing.T) {
+		rid := x.NewUUID()
+		req := x.NewTestHTTPRequest(t, "GET", ts.URL+login.BrowserLoginPath, nil)


This is missing ?prompt=true

it gets set here https://github.com/ory/kratos/pull/248/files#diff-318ba7d56b3e558e24dd7d812eb7bfccR80

gotcha 👍

aeneasr · 2020-02-19T07:40:08Z

selfservice/flow/login/handler_test.go

-		{"GET", login.BrowserLoginPath},
+	t.Run("does not set reauth flag on unauthenticated request", func(t *testing.T) {
+		c := ts.Client()
+		res, err := c.Get(ts.URL + login.BrowserLoginPath)


This should work for both prompt=true and prompt=false and no prompt

aeneasr · 2020-02-19T07:41:59Z

selfservice/flow/login/persistence.go

@@ -18,7 +18,8 @@ type (
 	RequestPersister interface {
 		CreateLoginRequest(context.Context, *Request) error
 		GetLoginRequest(context.Context, uuid.UUID) (*Request, error)
-		UpdateLoginRequest(context.Context, uuid.UUID, identity.CredentialsType, *RequestMethod) error
+		UpdateLoginRequestMethod(context.Context, uuid.UUID, identity.CredentialsType, *RequestMethod) error
+		UpdateLoginRequestReauth(ctx context.Context, id uuid.UUID, reauth bool) error


Since this is a one-way method (reauth is false by default and can only be set to true) wouldn't it make sense to name this something like MarkLoginRequestForced? Alternatively UpdateLoginRequest(context.Context, *Request).

aeneasr · 2020-02-19T07:42:43Z

persistence/sql/migrations/20191100000010_reauthentication.up.fizz

@@ -0,0 +1 @@
+add_column("selfservice_login_requests", "is_reauthentication", "bool", {default: false})


I would like to rename this to forced or, alternatively prompt although I think that forced is clearer.

aeneasr · 2020-02-19T07:43:17Z

selfservice/flow/login/request.go

@@ -61,6 +65,9 @@ type Request struct {

 	// CSRFToken contains the anti-csrf token associated with this request.
 	CSRFToken string `json:"-" db:"csrf_token"`
+
+	// IsReauthentication stores whether this login request is a reauthenication request.
+	IsReauthentication bool `json:"is_reauthentication" db:"is_reauthentication"`


I think forced is shorter and as concise :)

aeneasr · 2020-02-19T07:45:47Z

selfservice/strategy/oidc/provider_generic_test.go

+	}, public)
+	c, err := p.OAuth2(context.TODO())
+	require.NoError(t, err)
+	return c.AuthCodeURL("state", p.AddAuthCodeURLOptions(r)...)


I would rename this function to p.AuthCodeURLOptions because you are returning the options, not adding it to something.

aeneasr · 2020-02-19T07:47:22Z

selfservice/strategy/oidc/strategy_test.go

@@ -456,4 +459,41 @@ func TestStrategy(t *testing.T) {
 			aue(t, res, body, "an account with the same identifier (email, phone, username, ...) exists already")
 		})
 	})
+


Nice test cases :)

aeneasr · 2020-02-19T07:48:37Z

selfservice/strategy/password/login_test.go

-	identity.PrivilegedPoolProvider
-	password.HashProvider
-}
+func TestLoginNew(t *testing.T) {


Nice refactoring - this looks so much more readable :)

zepatrik · 2020-02-21T09:58:29Z

ready for review @aeneasr

aeneasr

Good job!

zepatrik added 4 commits February 13, 2020 18:46

u

2480ab2

u

6203e6b

Merge branch 'master' into add-reauth-endpoint

c0400c8

# Conflicts: # selfservice/flow/login/handler_test.go # selfservice/strategy/password/login_test.go

format

a75bd82

zepatrik commented Feb 14, 2020

View reviewed changes

persistence/sql/persister_login.go Outdated Show resolved Hide resolved

zepatrik commented Feb 14, 2020

View reviewed changes

persistence/sql/persister_login.go Outdated Show resolved Hide resolved

zepatrik and others added 5 commits February 14, 2020 16:59

u

f38157b

chore: Regenerate swagger spec and internal client

5c7d151

add tests

88f612e

Merge remote-tracking branch 'origin/add-reauth-endpoint' into add-re…

2633b01

…auth-endpoint

Merge branch 'master' into add-reauth-endpoint

a96cc4b

zepatrik marked this pull request as ready for review February 17, 2020 11:49

zepatrik requested a review from aeneasr February 17, 2020 11:49

zepatrik added 2 commits February 17, 2020 14:52

add tests

e0a973c

format

502ed83

aeneasr requested changes Feb 17, 2020

View reviewed changes

selfservice/flow/login/handler.go Outdated Show resolved Hide resolved

zepatrik added 2 commits February 18, 2020 12:30

Merge branch 'master' into add-reauth-endpoint

f449f0b

# Conflicts: # persistence/sql/persister_login.go

u

56b2e37

zepatrik requested a review from aeneasr February 18, 2020 11:59

aeneasr requested changes Feb 19, 2020

View reviewed changes

u

8492d11

zepatrik requested a review from aeneasr February 19, 2020 12:12

chore: Regenerate swagger spec and internal client

550f1db

zepatrik mentioned this pull request Feb 25, 2020

selfservice/profile: sudo mode for updating auth related stuff #243

Closed

3 tasks

aeneasr approved these changes Feb 28, 2020

View reviewed changes

aeneasr merged commit 344fc9c into master Feb 28, 2020

zepatrik deleted the add-reauth-endpoint branch March 23, 2020 18:47

aeneasr mentioned this pull request Apr 7, 2020

Fix broken redirect in login handler #323

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(selfservice/login): enable reauthentication functionality #248

feat(selfservice/login): enable reauthentication functionality #248

zepatrik commented Feb 14, 2020 •

edited

Loading

aeneasr Feb 19, 2020

aeneasr Feb 19, 2020

zepatrik Feb 19, 2020

aeneasr Feb 28, 2020

aeneasr Feb 19, 2020

aeneasr Feb 19, 2020

aeneasr Feb 19, 2020

zepatrik Feb 19, 2020

aeneasr Feb 19, 2020

aeneasr Feb 19, 2020

aeneasr Feb 19, 2020

aeneasr Feb 19, 2020

zepatrik commented Feb 21, 2020

aeneasr left a comment

		@@ -0,0 +1 @@
		add_column("selfservice_login_requests", "is_reauthentication", "bool", {default: false})

feat(selfservice/login): enable reauthentication functionality #248

feat(selfservice/login): enable reauthentication functionality #248

Conversation

zepatrik commented Feb 14, 2020 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zepatrik commented Feb 21, 2020

aeneasr left a comment

Choose a reason for hiding this comment

zepatrik commented Feb 14, 2020 •

edited

Loading