use auth0 as our login mechanism #245
Conversation
| func OrDebug(closer io.Closer) { | ||
| FuncOrDebug(closer.Close) | ||
| } | ||
|
|
||
| func FuncOrDebug(closer func() error) { | ||
| if err := closer(); err != nil { | ||
| zap.L().Debug("Failed to close resource", zap.Error(err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Something I've used on side projects which might be nice (adapted to the style of this package):
func OrDebug(o any) {
var err error
switch o := o.(type) {
case io.Closer:
err = o.Close()
case func() error:
err = o()
default:
return
}
if err != nil {
zap.L().Debug("Failed to close resource", zap.Error(err))
}
}
pkg/auth/auth.go
Outdated
| State string | ||
| } | ||
|
|
||
| type Authorizer interface { |
There was a problem hiding this comment.
This interface should be defined in cli where it's used (ie, klothomain) not at definition time. The DefaultIfNil function can be moved there as well (or just inlined). Either that or move local authorizer in this package as a subpackage and "standardAuthorizer" (using the auth server) into another subpackage.
pkg/auth/auth.go
Outdated
| func (s standardAuthorizer) Authorize() (*KlothoClaims, error) { | ||
| return Authorize() | ||
| } |
There was a problem hiding this comment.
Not how I'd have expected this to have been implemented, but I suppose it works.
What I'd expect (and IMO more idiomatic):
type ServiceAuthorizer struct {
AuthUrlBase string
PemUrl string
}
func (s ServiceAuthorizer) Authorize() (*KlothoClaims, error) {
// auth implemented in this function
}Login & Logout similarly implemented as methods and type-checked to call from klotho-main on the authorizer. This would enable other authorizer implementations to also have login/logout functionality.
There was a problem hiding this comment.
I'd prefer to keep this as-is for now, since (a) it works, (b) refactoring it later is unlikely to be more work than refactoring it now, and (c) we currently don't have a need for multiple authorizers.
There was a problem hiding this comment.
(a) sure
(b) makes sense
(c) I'd argue that the local implementation is already multiple authorizers. If not, then there's no need for a struct or interface - just call the global methods from klothomain.
| } | ||
|
|
||
| func CallLoginEndpoint() (string, error) { | ||
| res, err := http.Get(authUrlBase + "/login") |
There was a problem hiding this comment.
nit: this should use an *http.Client and not the default one - though we don't do that a lot of other places so fine for now
| @@ -30,184 +23,49 @@ type Validated struct { | |||
| // located in ~/.klotho/ | |||
| var analyticsFile = "analytics.json" | |||
|
|
|||
| func CreateUser(email string) error { | |||
|
|
|||
| func GetOrCreateAnalyticsFile() AnalyticsFile { | |||
There was a problem hiding this comment.
This feels like it should return an error
There was a problem hiding this comment.
What would you suggest we do with that error?
The code as it stands:
- Tries to get the current analytics.json file
- If that errors (probably because it doesn't exist, but possibly for other reasons) then:
- create a new tracking id (in the form of the
AnalyticsFilestruct); this can never error - try to write that id to analytics.json; this can error, in which case we log a debug and proceed
- send that newly created tracking id up to the caller
- create a new tracking id (in the form of the
If we returned an error from this method, I could imagine doing one of two things with it:
- logging it and proceeding, in which case that's the same as this currently logic
- erroring out to the user
Am I missing an option? If not, do you think we should do option 1 (in which case imo it's just a stylistic question) or 2 (in which case I disagree, but we should have that discussion)?
There was a problem hiding this comment.
Regardless of (1) or (2) it doesn't feel like the analytics package should be the "decider" of that behaviour - so I guess it's a stylistic matter.
ghost
|
Latest push is a rebase plus some fixup squashes. Here's the "diff of diffs" to show how this PR applies to main before and after the rebase: $ diff <(git diff origin/auth main) <(git diff auth main)
42c42
< index 3aadacc..65dab23 100644
---
> index cd163da..65dab23 100644
59c59,61
< @@ -32,13 +31,6 @@ require (
---
> @@ -30,13 +29,6 @@ require (
> sigs.k8s.io/yaml v1.3.0
> )
61,62d62
< replace github.com/smacker/go-tree-sitter => github.com/klothoplatform/go-tree-sitter v0.1.1
<
69a70,71
> replace github.com/smacker/go-tree-sitter => github.com/klothoplatform/go-tree-sitter v0.1.1
>
71,72d72
< github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
< github.com/BurntSushi/toml v1.2.1 // indirect
90,98d89
< @@ -121,7 +112,7 @@ require (
< github.com/shopspring/decimal v1.2.0 // indirect
< github.com/sirupsen/logrus v1.9.0 // indirect
< github.com/spf13/cast v1.4.1 // indirect
< - github.com/spf13/pflag v1.0.5
< + github.com/spf13/pflag v1.0.5 // indirect
< github.com/stretchr/objx v0.5.0 // indirect
< github.com/xeipuuv/gojsonpointer v0.0.0-20180127040702-4e3ac2762d5f // indirect
< github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirectSo the only changes were in go.mod and go.sum. The rebase just pulled in the latest from main (which makes sense). |
This introduces a new Authorizer, which is hookable per KlothoMain. The default one just calls auth.Authorize(), but KlothoMains can provide an alternative, including one that adds flags. resolves #164
Before this, the server's /logintoken API would return instantly, with either the token from auth0 or 404 if it hadn't been set yet from the auth0.com callback. So, we needed a callback not just for intermittent errors, but for the happy path functionality. Now, the /logintoken call hangs until it gets that token (or times out), so the happy path doesn't need a retry. We could still need a retry for intermittent failurs, but that's a more general problem that we should resolve across all http calls, not just for auth. I'm also moving the Login error handler to a callback, just as an organizational tool. We want to send the analytics for any error manually (since the client hasn't been set up yet), but the Login method shouldn't have to know about that intricacy. klothomain.go does, so that's where we should put that logic
I'm doing it as a base (instead of just hostname) so that people can specify http and port (especially for localhost testing).
In some cases, were creating the deferred close after the method would have already returned. Also, we were dropping some errors on the ground; this will now handle them.
- split analytics/client.go:NewClient up, such that its email stuff is
in a new method, AttachAuthorizations().
- rm most of analytics/user.go, and replace it with a tiny method
GetOrCreateAnalyticsFile(). This just upserts the
~/.klotho/analytics.json file. All of the logic around retrieving the
user and validating emails and all that is just gone.
- auth/auth.go:
- Authorize() now returns its claims. We pass these to
AttachAuthorizations(), mentioned above.
- We download our auth0 PEM (and cache it), and use it to verify the
auth0 token. Without this, we can't actually get the claims.
- cli/klothomain.go:
- construct analytics.Client just once, and then later attach the
email from the claims (this resolves #180)
- also move the log hooks up, to follow the client
- use RegisteredClaims instead of StandardClaims. Per the [jwt docs][1]:
> Deprecated: Use RegisteredClaims instead for a forward-compatible way
> to access registered claims in a struct.
[1]: https://pkg.go.dev/github.com/golang-jwt/jwt/v4#StandardClaims
If the `credentials.json` file isn't there at all, then require a login. But if it's there but the login doesn't work for whatever reason, just issue a warning and move on. Meanwhile, in the login path, handle errors by writing an empty `credentials.json` and then ignoring the error. resolves #194 Relatedly, add a workaround for #195. It's very easy to hit it with this new path, so I felt like a quick workaround is in order.
In service of #218
|
Big Honkin' Feature Merge™. Sorry. :-(
This rips out all of the current analytics tracking, and replaces it with an auth0-based approach. There's a description of the overall flow at klothoplatform/klotho-auth-service. If you like a more hands-on approach, you can also create a manual test runbook instance.
There's a lot here, so I'm open to discussing ways we can make it easier to review.
Also curious to see what people think we should do for a merge strategy, since there's a lot of commits and they come from two authors, but they really do want to all land on a unit (or at least, not along the subdivisions that the commits reflect).
This touches several tickets:
Outstanding "Auth0 v1" tickets:
klothoplatform/klotho-server#21Standard checks