Add api endpoint to list all authorized clients by user by kingjan1999 · Pull Request #954 · ory/hydra

kingjan1999 · 2018-07-29T14:17:14Z

Resolves #953

As already mentioned in #953, I'm rather beginner in Go, so I'm open to comments and suggestions for improvement and ask for leniency for possible mistakes.

Signed-off-by: Jan Beckmann king-jan1999@hotmail.de

Resolves ory#953 Signed-off-by: Jan Beckmann <king-jan1999@hotmail.de>

aeneasr · 2018-07-30T07:51:33Z

+		return
+	}
+
+	sessions, err := h.M.FindPreviouslyGrantedConsentRequestsByUser(user)


I really think we need pagination here, because this can be a lot! Examples can be found here, here and here.

Thanks, I adapted it. However, I was a bit unsure in consent/manager_memory.go as FindPreviouslyGrantedConsentRequests depends now on the newly created FindPreviouslyGrantedConsentRequestsByUser (which requires a limit and offset now) and does not support limit and offset. As you'll see, I've used a little workaround that I'm personally not 100% satisfied with.

I see, I don't think this solution will work with SQL as they don't support negative offset (I think).

Oh sorry, I didn't see that you duplicated the other method. Then it will obviously work. I agree that it's not the cleanest solution but it does get the job done for now. So from my side, this is fine.

aeneasr · 2018-07-30T07:52:35Z

+	var rs []HandledConsentRequest
+	filteredByUser, _ := m.FindPreviouslyGrantedConsentRequestsByUser(subject)
+	for _, c := range filteredByUser {
+		if client != c.ConsentRequest.Client.GetID() {


You can revert this logic to == and add rs = append(rs, c) in the loop, it is easier to read!

aeneasr · 2018-07-30T08:00:37Z

 	RememberFor int `json:"remember_for"`

-	ConsentRequest  *ConsentRequest     `json:"-"`
+	ConsentRequest  *ConsentRequest     `json:"consent_request"`


The problem with exposing this is that acceptance of the consent request could overwrite this value and potentially mess stuff up. I haven't checked if that's actually the case but it could be an issue in some regression. I think it would be ok to copy this struct 1:1 and there set the consent_request value. If the order of the fields is the same, you can easily convert the type:

var foo HandledConsentRequest var bar = NewTypeWithJSON(HandledConsentRequest)

I called it HandledConsentRequestResponse now but other suggestions are very welcome

How about PreviousConsentSession? :)

aeneasr

This looks very solid already. It is really important though that we have a test for the SDK as well (sdk_test.go) and check that the values are populated properly (and also check that e.g. client secrets are removed). Once that is done and the things are addressed, I think this is good to go!

aeneasr · 2018-08-01T09:39:29Z

I'm pinging you @kingjan1999 so this doesn't get lost in your notifications. This looks already very solid, just a few things which I mentioned during review :)

Signed-off-by: Jan <king-jan1999@hotmail.de>

kingjan1999 · 2018-08-01T17:59:36Z

Thanks for your help @arekkas ! I hope I have implemented all your proposals.

When Swagger generated the SDK, some files (concerning JSONWebKeys) appeared for unknown reasons, which I have now simply commited.

aeneasr · 2018-08-02T13:36:21Z

The only thing left is the naming of the one struct and then this is good to get merged from my side! Thank you so much for your contribution :)

Signed-off-by: Jan <king-jan1999@hotmail.de>

kingjan1999 · 2018-08-02T16:40:57Z

Thanks for your naming suggestion @arekkas ! I've implemented it accordingly.

aeneasr · 2018-08-02T17:10:36Z

Perfect!

aeneasr

I took another pass. Unfortunately, I have to nag you with some minor things (just newlines that were removed/added) and also some naming and error handling things.

Once those are addressed, I truly believe it's good to merge (unless I missed something).

Thank you, you're awesome! :)

aeneasr · 2018-08-02T17:11:36Z


 func (m *MemoryManager) FindPreviouslyGrantedConsentRequests(client string, subject string) ([]HandledConsentRequest, error) {
+	var rs []HandledConsentRequest
+	filteredByUser, _ := m.FindPreviouslyGrantedConsentRequestsByUser(subject, -1, -1)


The error must be handled here!

aeneasr · 2018-08-02T17:12:09Z

+		}
+		return nil, sqlcon.HandleError(err)
+	}
+	aa, err := m.resolveHandledConsentRequests(a)


You can rewrite this to return m.resolveHandledConsentRequests(a)

aeneasr · 2018-08-02T17:12:37Z

 		if err != nil {
 			return nil, err
 		}
-


Re-add newline

aeneasr · 2018-08-02T17:12:38Z

 	}

 	return aa, nil
+


Remove newline

aeneasr · 2018-08-02T17:12:40Z

 		if v.RememberFor > 0 && v.RequestedAt.Add(time.Duration(v.RememberFor)*time.Second).Before(time.Now().UTC()) {
 			continue
 		}
-


Re-add newline

aeneasr · 2018-08-02T17:12:49Z

+	}
+	aa, err := m.resolveHandledConsentRequests(a)
+	return aa, err
+


Remove newline

aeneasr · 2018-08-02T17:16:52Z

+	if limit < 0 && offset < 0 {
+		return rs, nil
+	}
+	start, end := pagination.Index(limit, offset, len(rs))


Add a newline on top of start, end := ..

aeneasr · 2018-08-02T17:17:03Z

 	if len(rs) == 0 {
 		return []HandledConsentRequest{}, nil
 	}
-


Re-add this newline

aeneasr · 2018-08-02T17:18:11Z

 	w.WriteHeader(http.StatusNoContent)
 }

+// swagger:route GET /oauth2/auth/sessions/consent/{user} oAuth2 listUserClientConsentSessions


I think the naming should be listUserConsentSessions. With the current naming, I would expect the method to be like listUserClientConsentSessions(user, client string)

aeneasr · 2018-08-02T17:24:46Z

+LIMIT ? OFFSET ?
+`), subject, limit, offset); err != nil {
+		if err == sql.ErrNoRows {
+			return nil, errors.WithStack(errNoPreviousConsentFound)


So one issue with this is that the HTTP handler function will return errNoPreviousConsentFound which is a "dumb" error (meaning it will return a 500 status code with no context in the API). On the same hand, this error is required to be returned for the consent_strategy to work. Since you duplicated the methods and this particular method is not used in the consent_strategy, you can remove the err == sql.ErrNoRows check and just return return nil, sqlcon.HandleError(err).

Please also add a test for this. It should check that sqlcon.ErrNoRows is returned when we expect that no session exist in the store. This will return a proper 404 error in the API which I think is to be expected here.

In the handler, you may want to choose to ignore the error of type sqlcon.ErrNoRows and instead just write an empty array. This will return [] instead of a 404 which might be a better suited response.

Thanks for your help and your feedback! I have gladly implemented your suggestions.

However, I'm a little unsure at this point and need your help: Does db.Select really return an ErrNoRows error if no rows could be found? According to my understanding of SQLX / Go SQL documentation, this does not happen here. I have not been able to force such behaviour in my experiments.

Or did I misunderstand and the manager should generate and return an sqlcon.ErrNoRows?

Signed-off-by: Jan Beckmann <king-jan1999@hotmail.de>

aeneasr · 2018-08-06T08:30:33Z

Thank you for the updates! The newly generated SDK breaks tests because the methods have been renamed:

consent/sdk_test.go:121:24: sdk.ListUserClientConsentSessions undefined (type *hydra.CodeGenSDK has no field or method ListUserClientConsentSessions)
consent/sdk_test.go:128:23: sdk.ListUserClientConsentSessions undefined (type *hydra.CodeGenSDK has no field or method ListUserClientConsentSessions)

Signed-off-by: Jan Beckmann <king-jan1999@hotmail.de>

aeneasr · 2018-08-06T16:48:17Z

Thank you so much for your hard work!

kingjan1999 force-pushed the list-consents branch from 288bd4d to 46720ba Compare July 29, 2018 14:27

Add api endpoint to list all authorized clients by user

adbcc5f

Resolves ory#953 Signed-off-by: Jan Beckmann <king-jan1999@hotmail.de>

kingjan1999 force-pushed the list-consents branch from 46720ba to adbcc5f Compare July 29, 2018 14:28

aeneasr reviewed Jul 30, 2018

View reviewed changes

aeneasr approved these changes Jul 30, 2018 •

edited

Loading

View reviewed changes

aeneasr reviewed Jul 30, 2018

View reviewed changes

Generate and test sdk for listUserConsentSessions, add pagination

701e490

Signed-off-by: Jan <king-jan1999@hotmail.de>

kingjan1999 force-pushed the list-consents branch from 31d14ba to 701e490 Compare August 1, 2018 17:38

Rename HandledConsentRequestResponse to PreviousConsentSession

6857f36

Signed-off-by: Jan <king-jan1999@hotmail.de>

kingjan1999 force-pushed the list-consents branch from 5234a6d to 6857f36 Compare August 2, 2018 16:30

aeneasr reviewed Aug 2, 2018

View reviewed changes

Remove / add newlines, improve error handling

8225b23

Signed-off-by: Jan Beckmann <king-jan1999@hotmail.de>

kingjan1999 force-pushed the list-consents branch from cb13791 to 8225b23 Compare August 5, 2018 16:13

Generate swagger sdk

9806eb7

Signed-off-by: Jan Beckmann <king-jan1999@hotmail.de>

Update sdk test to reflect rename

d0e4d9b

Signed-off-by: Jan Beckmann <king-jan1999@hotmail.de>

aeneasr merged commit 7aace33 into ory:master Aug 6, 2018

Conversation

kingjan1999 commented Jul 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

aeneasr commented Aug 1, 2018

Uh oh!

kingjan1999 commented Aug 1, 2018

Uh oh!

aeneasr commented Aug 2, 2018

Uh oh!

kingjan1999 commented Aug 2, 2018

Uh oh!

aeneasr commented Aug 2, 2018

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeneasr commented Aug 6, 2018

Uh oh!

aeneasr commented Aug 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants