-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Implement Public Clients #471
Conversation
|
||
err := cli.Metadata.Valid() | ||
if err != nil { | ||
fmt.Println("manager throwing validation error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left over log statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIP Means work in progress :)
304bf66
to
89280b1
Compare
bdbf96c
to
cb207e7
Compare
Got a vet error
|
|
||
err := cli.Metadata.Valid() | ||
if err != nil { | ||
return client.ValidationError{err} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the vet error btw. Needs to be client.ValidationError{Err:err}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I just don't know why mine didn't catch it.
@@ -165,6 +163,12 @@ func (a *AdminAPI) GetConnectors() ([]connector.ConnectorConfig, error) { | |||
} | |||
|
|||
func mapError(e error) error { | |||
switch t := e.(type) { | |||
case client.ValidationError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike type switching for a specific error since it makes the validateClient code really brittle. Would be nice if this was formalized more, such as introducing HTTP specific error behavior. I suppose that's outside of the scope of this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it help if I documented manager.New() as to which errors can be returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put it on ValidationError? Whichever works best. If it's tested that might be enough.
* validation of client moved into its own method and tested * public clients have different validation - must have no redirect URIs and must have a clientName set
* Start Documentation
and update docs
Metadata is not enough these days - we're going to need access to the Public field as well.
* disallow ClientCreds for public clients * clients can only redirect to localhost or OOB
When calling manager.Authenticate, logs now show different error messages.
When "urn:ietf:wg:oauth:2.0:oob" is used as a redirect URI, redirect to an internal dex page where the user is shown the code and instructed to paste it into their app.
fixes #469