Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add api endpoint to list all authorized clients by user #954

Merged
merged 6 commits into from
Aug 6, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions consent/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,11 @@ type swaggerRejectRequest struct {
// in: body
Body RequestDeniedError
}

// A list of handled consent requests.
// swagger:response handledConsentRequestList
type swaggerListHandledConsentRequestsResult struct {
// in: body
// type: array
Body []HandledConsentRequest
}
42 changes: 42 additions & 0 deletions consent/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func (h *Handler) SetRoutes(r *httprouter.Router) {
r.PUT(ConsentPath+"/:challenge/reject", h.RejectConsentRequest)

r.DELETE("/oauth2/auth/sessions/login/:user", h.DeleteLoginSession)
r.GET("/oauth2/auth/sessions/consent/:user", h.GetConsentSessions)
r.DELETE("/oauth2/auth/sessions/consent/:user", h.DeleteUserConsentSession)
r.DELETE("/oauth2/auth/sessions/consent/:user/:client", h.DeleteUserClientConsentSession)
}
Expand Down Expand Up @@ -133,6 +134,47 @@ func (h *Handler) DeleteUserClientConsentSession(w http.ResponseWriter, r *http.
w.WriteHeader(http.StatusNoContent)
}

// swagger:route GET /oauth2/auth/sessions/consent/{user} oAuth2 listUserClientConsentSessions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//
// List all consent sessions of a user
//
// This endpoint lists all user's granted consent sessions, including client and granted scope
//
// Consumes:
// - application/json
//
// Produces:
// - application/json
//
// Schemes: http, https
//
// Responses:
// 200: handledConsentRequestList
// 401: genericError
// 403: genericError
// 500: genericError

func (h *Handler) GetConsentSessions(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
user := ps.ByName("user")
if user == "" {
h.H.WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithDebug("Parameter user is not defined")))
return
}

sessions, err := h.M.FindPreviouslyGrantedConsentRequestsByUser(user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if err != nil {
h.H.WriteError(w, r, err)
return
}

for _, session := range sessions {
session.ConsentRequest.Client = sanitizeClient(session.ConsentRequest.Client)
}

h.H.Write(w, r, sessions)
}

// swagger:route DELETE /oauth2/auth/sessions/login/{user} oAuth2 revokeAuthenticationSession
//
// Invalidates a user's authentication session
Expand Down
1 change: 1 addition & 0 deletions consent/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type Manager interface {

VerifyAndInvalidateConsentRequest(verifier string) (*HandledConsentRequest, error)
FindPreviouslyGrantedConsentRequests(client string, user string) ([]HandledConsentRequest, error)
FindPreviouslyGrantedConsentRequestsByUser(user string) ([]HandledConsentRequest, error)

// Cookie management
GetAuthenticationSession(id string) (*AuthenticationSession, error)
Expand Down
20 changes: 16 additions & 4 deletions consent/manager_memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,22 @@ func (m *MemoryManager) VerifyAndInvalidateConsentRequest(verifier string) (*Han
}

func (m *MemoryManager) FindPreviouslyGrantedConsentRequests(client string, subject string) ([]HandledConsentRequest, error) {
var rs []HandledConsentRequest
filteredByUser, _ := m.FindPreviouslyGrantedConsentRequestsByUser(subject)
for _, c := range filteredByUser {
if client != c.ConsentRequest.Client.GetID() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

continue
}
rs = append(rs, c)
}
if len(rs) == 0 {
return []HandledConsentRequest{}, nil
}

return rs, nil
}

func (m *MemoryManager) FindPreviouslyGrantedConsentRequestsByUser(subject string) ([]HandledConsentRequest, error) {
var rs []HandledConsentRequest
for _, c := range m.handledConsentRequests {
cr, err := m.GetConsentRequest(c.Challenge)
Expand All @@ -171,10 +187,6 @@ func (m *MemoryManager) FindPreviouslyGrantedConsentRequests(client string, subj
return nil, err
}

if client != cr.Client.GetID() {
continue
}

if subject != cr.Subject {
continue
}
Expand Down
31 changes: 28 additions & 3 deletions consent/manager_sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,8 +364,34 @@ WHERE
return nil, sqlcon.HandleError(err)
}

return m.resolveHandledConsentRequests(a)
}

func (m *SQLManager) FindPreviouslyGrantedConsentRequestsByUser(subject string) ([]HandledConsentRequest, error) {
var a []sqlHandledConsentRequest

if err := m.db.Select(&a, m.db.Rebind(`SELECT h.* FROM
hydra_oauth2_consent_request_handled as h
JOIN
hydra_oauth2_consent_request as r ON (h.challenge = r.challenge)
WHERE
r.subject=? AND r.skip=FALSE
AND
(h.error='{}' AND h.remember=TRUE)
`), subject); err != nil {
if err == sql.ErrNoRows {
return nil, errors.WithStack(errNoPreviousConsentFound)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
return nil, sqlcon.HandleError(err)
}

return m.resolveHandledConsentRequests(a)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline

}

func (m *SQLManager) resolveHandledConsentRequests(requests []sqlHandledConsentRequest) ([]HandledConsentRequest, error) {
var aa []HandledConsentRequest
for _, v := range a {
for _, v := range requests {
r, err := m.GetConsentRequest(v.Challenge)
if err != nil {
return nil, err
Expand All @@ -376,12 +402,10 @@ WHERE
if v.RememberFor > 0 && v.RequestedAt.Add(time.Duration(v.RememberFor)*time.Second).Before(time.Now().UTC()) {
continue
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-add newline

va, err := v.toHandledConsentRequest(r)
if err != nil {
return nil, err
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-add newline

aa = append(aa, *va)
}

Expand All @@ -390,4 +414,5 @@ WHERE
}

return aa, nil

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline

}
47 changes: 47 additions & 0 deletions consent/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,53 @@ func TestManagers(t *testing.T) {
})
}
})

t.Run("case=list-handled-consent-requests", func(t *testing.T) {
for k, m := range managers {
cr1, hcr1 := mockConsentRequest("rv1", true, 0, false, false, false)
cr2, hcr2 := mockConsentRequest("rv2", false, 0, false, false, false)
clientManager.CreateClient(cr1.Client)
clientManager.CreateClient(cr2.Client)

require.NoError(t, m.CreateConsentRequest(cr1))
require.NoError(t, m.CreateConsentRequest(cr2))
_, err := m.HandleConsentRequest("challengerv1", hcr1)
require.NoError(t, err)
_, err = m.HandleConsentRequest("challengerv2", hcr2)
require.NoError(t, err)

t.Run("manager="+k, func(t *testing.T) {
for i, tc := range []struct {
subject string
challenges []string
clients []string
}{
{
subject: "subjectrv1",
challenges: []string{"challengerv1"},
clients: []string{"clientrv1"},
},
{
subject: "subjectrv2",
challenges: []string{},
clients: []string{},
},
} {
t.Run(fmt.Sprintf("case=%d/subject=%s", i, tc.subject), func(t *testing.T) {
consents, _ := m.FindPreviouslyGrantedConsentRequestsByUser(tc.subject)

assert.Equal(t, len(tc.challenges), len(consents))

for _, consent := range consents {
assert.Contains(t, tc.challenges, consent.Challenge)
assert.Contains(t, tc.clients, consent.ConsentRequest.Client.ClientID)
}
})
}
})
}

})
}

func compareAuthenticationRequest(t *testing.T, a, b *AuthenticationRequest) {
Expand Down
2 changes: 1 addition & 1 deletion consent/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ type HandledConsentRequest struct {
// authorization will be remembered indefinitely.
RememberFor int `json:"remember_for"`

ConsentRequest *ConsentRequest `json:"-"`
ConsentRequest *ConsentRequest `json:"consent_request"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called it HandledConsentRequestResponse now but other suggestions are very welcome

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about PreviousConsentSession? :)

Error *RequestDeniedError `json:"-"`
Challenge string `json:"-"`
RequestedAt time.Time `json:"-"`
Expand Down
44 changes: 44 additions & 0 deletions docs/api.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,38 @@
}
},
"/oauth2/auth/sessions/consent/{user}": {
"get": {
"description": "This endpoint lists all user's granted consent sessions, including client and granted scope",
"consumes": [
"application/json"
],
"produces": [
"application/json"
],
"schemes": [
"http",
"https"
],
"tags": [
"oAuth2"
],
"summary": "List all consent sessions of a user",
"operationId": "listUserClientConsentSessions",
"responses": {
"200": {
"$ref": "#/responses/handledConsentRequestList"
},
"401": {
"$ref": "#/responses/genericError"
},
"403": {
"$ref": "#/responses/genericError"
},
"500": {
"$ref": "#/responses/genericError"
}
}
},
"delete": {
"description": "This endpoint revokes a user's granted consent sessions and invalidates all associated OAuth 2.0 Access Tokens.",
"consumes": [
Expand Down Expand Up @@ -1925,6 +1957,9 @@
"type": "object",
"title": "The request payload used to accept a consent request.",
"properties": {
"consent_request": {
"$ref": "#/definitions/consentRequest"
},
"grant_scope": {
"description": "GrantScope sets the scope the user authorized the client to use. Should be a subset of `requested_scope`",
"type": "array",
Expand Down Expand Up @@ -3015,6 +3050,15 @@
}
}
},
"handledConsentRequestList": {
"description": "A list of handled consent requests.",
"schema": {
"type": "array",
"items": {
"$ref": "#/definitions/acceptConsentRequest"
}
}
},
"oAuth2ClientList": {
"description": "A list of clients.",
"schema": {
Expand Down