fix: Add network specific error message to avoid confusion #2367
Conversation
|
|
lukestoward
changed the title
| @@ -316,6 +318,11 @@ func (m *RegistryBase) ScopeStrategy() fosite.ScopeStrategy { | |||
|
|
|||
| func (m *RegistryBase) newKeyStrategy(key string) (s jwk.JWTStrategy) { | |||
| if err := jwk.EnsureAsymmetricKeypairExists(context.Background(), m.r, new(jwk.RS256Generator), key); err != nil { | |||
| var netError net.Error | |||
| if errors.As(err, &netError) { | |||
There was a problem hiding this comment.
In this case I don't believe so, as net.Error is an interface. It worked when I tested it locally.
There was a problem hiding this comment.
Right, that makes sense! Maybe we could add a small test to ensure we don't regress on this in future iterations?
There was a problem hiding this comment.
Sure, I'll see what I can do.
There was a problem hiding this comment.
@aeneasr i'm having trouble writing tests for this due to the fact that we call .Fatalf as part of the error logging, which inevitably leads to os.Exit(1). Do have any advice/examples on how I might write tests for this? Thanks.
Edit: I can replace ExitFunc on the Logrus instance to prevent the call to os.Exit, however, execution will continue after the log call is made. I don't think it makes sense to explicitly call return.
There was a problem hiding this comment.
It's ok, you can call return explicitly here!
Related issue
#2338
Proposed changes
In the event of a general network error, avoid logging a misleading/confusing message.
Checklist
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.
works.
Further comments