changed 404 to 401 for unknown client #1707
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
obasajujoshua31
force-pushed
the
fix-get-client-404
branch
from
1c775f4
to
f3e341e
Compare
aeneasr
reviewed
Jan 23, 2020
There was a problem hiding this comment.
Looks good so far, but there are pieces missing:
- Tests
- The memory manager still returns
sql.ErrNoRows/ 404 - We need to update the swagger documentation to be able to handle 401: https://github.com/ory/hydra/blob/master/client/handler.go#L232-L234
This also needs to be done in the memory manager and we should also add a test!
I also think that it would be a better idea to handle this 404->401 conversion in the handler, because we use the manager also in other areas of hydra, and it might break functionality there.
Handler: https://github.com/ory/hydra/blob/master/client/handler.go#L243
client/manager_sql.go
Outdated
| @@ -253,6 +258,13 @@ func (m *SQLManager) CreateSchemas(dbName string) (int, error) { | |||
| func (m *SQLManager) GetConcreteClient(ctx context.Context, id string) (*Client, error) { | |||
| var d sqlData | |||
| if err := m.DB.GetContext(ctx, &d, m.DB.Rebind("SELECT * FROM hydra_client WHERE id=?"), id); err != nil { | |||
| if errorsx.Cause(err) == sql.ErrNoRows { | |||
| return nil, errors.WithStack(&herodot.DefaultError{ | |||
There was a problem hiding this comment.
You can use herodot.ErrUnauthorized.WithReason("The requested OAuth 2.0 client does not exist or you did not provide the necessary credentials.")
obasajujoshua31
force-pushed
the
fix-get-client-404
branch
2 times, most recently
from
2ac9ff2
to
6d78337
Compare
aeneasr
reviewed
Jan 24, 2020
| @@ -230,13 +233,17 @@ func (h *Handler) List(w http.ResponseWriter, r *http.Request, ps httprouter.Par | |||
| // | |||
| // Responses: | |||
| // 200: oAuth2Client | |||
| // 401: genericError | |||
aeneasr
requested changes
Jan 24, 2020
client/handler.go
Outdated
| // 404: genericError | ||
| // 500: genericError | ||
| func (h *Handler) Get(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { | ||
| var id = ps.ByName("id") | ||
|
|
||
| c, err := h.r.ClientManager().GetConcreteClient(r.Context(), id) | ||
| if err != nil { | ||
| if errors.Cause(err).Error() == sqlcon.ErrNoRows.ErrorField { |
There was a problem hiding this comment.
I believe this should be enough:
Suggested change
| if errors.Cause(err).Error() == sqlcon.ErrNoRows.ErrorField { | |
| if errorsx.Cause(err) == sqlcon.ErrNoRows { |
obasajujoshua31
force-pushed
the
fix-get-client-404
branch
3 times, most recently
from
1518568
to
815c500
Compare
obasajujoshua31
force-pushed
the
fix-get-client-404
branch
from
815c500
to
96ecbc7
Compare
DennisPattmann5012
pushed a commit
to DennisPattmann5012/hydra
that referenced
this pull request
Feb 3, 2020
eli-zh
pushed a commit
to eli-zh/hydra
that referenced
this pull request
Mar 22, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue
#1617
Proposed changes
Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
Further comments