feat: export token expired error

[crenshaw-dev]

Relates to #327

This allows errors.Is checking while preserving the exact same error messages for those who are doing string comparison.

Preserving the exact same strings means some of the error messages (without context) look a little weird. But I think it's a reasonable tradeoff until it's time for another major version bump.

This PR would let me write unit tests in Argo CD without worrying that a change in go-oidc will break my string comparison checks.

[ericchiang]

Hey @crenshaw-dev,

I don't want to export every possible error. If you've got a specific use case where a typed error would be helpful (e.g. specific HTTP status codes from the provider), happy to add that. Do you have an example of what you're trying to do?

Also, some of these errors arguably shouldn't be used problematically, since they're hit before we validate the signature check on the ID token.

Eric

[crenshaw-dev]

@ericchiang there are two current use cases in Argo CD:

1. We want to know if it's a "token expired" error so the UI can redirect the user immediately to a new login flow: https://github.com/argoproj/argo-cd/blob/a588e63e3cb9db91fd491c0d7371c87867861aa7/util/session/sessionmanager.go#L493-L504
2. We have a regression test to make sure we support algorithms besides RS256 (added support with 2.2.0): https://github.com/argoproj/argo-cd/blob/a588e63e3cb9db91fd491c0d7371c87867861aa7/util/session/sessionmanager_test.go#L581

It's possible that test shouldn't exist. But I think the expired example is a reasonable use case. Would you be interested in a PR that only exports that error?

[ericchiang]

I think a token expired error would be reasonable to add!

That second test probably shouldn't be necessary? Just make sure your RS512 token doesn't return an error.

So sounds like the API we'd want to add it

var ErrTokenExpired = errors.New(...)

(nit: exported error variables generally start with "Err", and error types end with "Error" https://pkg.go.dev/io#pkg-variables, https://pkg.go.dev/net#AddrError)

Kinda a tangent, but I've also been thinking about having an oidctest package that would let you stand up a httptest provider with some basic configuration. Maybe that would be helpful too.

[crenshaw-dev]

Just make sure your RS512 token doesn't return an error.

Yeeeeah that would be proper. I was lazy and didn't want to have to set up a full mock OIDC provider. Maybe that would be easier than I assumed.

(nit: exported error variables generally start with "Err", and error types end with "Error" https://pkg.go.dev/io#pkg-variables, https://pkg.go.dev/net#AddrError)

Good catch, will fix!

Kinda a tangent, but I've also been thinking about having an oidctest package that would let you stand up a httptest provider with some basic configuration. Maybe that would be helpful too.

Yep, that would make some other tests I'm working on a lot easier I think! Maybe I can put some Intuit time towards that eventually.

Updating the PR to be more minimal!

[ericchiang]

(please squash your commits)

[crenshaw-dev]

This may or may not be the proper way to do this. lmk

[ericchiang]

Add a new field and use that instead?

type verificationTest struct {
	wantErr bool
	
	wantErrIs error
}

[crenshaw-dev]

@ericchiang ready for another look

[ericchiang]

Add a new field and use that instead?

type verificationTest struct {
	wantErr bool
	
	wantErrIs error
}

[crenshaw-dev]

@ericchiang updated

[ericchiang]

thanks!