fix: checking hosts properly #2945
Conversation
ssoroka
requested review from
BruceMacD,
dnephin,
jmorganca,
pdevine and
kimskimchi
as code owners
internal/access/signup.go
Outdated
| @@ -71,12 +77,14 @@ func Signup(c *gin.Context, keyExpiresAt time.Time, details SignupDetails) (*mod | |||
| Resource: ResourceInfraAPI, | |||
| CreatedBy: identity.ID, | |||
| }, | |||
| { | |||
| } | |||
| if isFirstOrg { | |||
There was a problem hiding this comment.
don't give support-admin to all orgs!
internal/server/data/data.go
Outdated
| if org == nil { | ||
| panic("missing org id in context") | ||
| } |
There was a problem hiding this comment.
these panics are programmer errors, and should be obvious from tests. If you remove them, it will still panic, but with a worse message.
| } | ||
| } | ||
|
|
||
| func setOrganizationInCtx(srv *Server) gin.HandlerFunc { |
There was a problem hiding this comment.
|
|
||
| if !roots.AppendCertsFromPEM([]byte(opts.CA)) { | ||
| logging.Warnf("failed to load TLS CA, invalid PEM") | ||
| if len(raw) == 0 { |
There was a problem hiding this comment.
added some checks in this file to avoid panics I ran into.
| run(t, setupDB(t, pgsql)) | ||
| db := setupDB(t, pgsql) | ||
| run(t, db) | ||
| db.Rollback() |
There was a problem hiding this comment.
Cool. I just added db.Close() in my own changes here because there will be two connections in the RLS changes (the privileged and the unprivileged connection). Rollback seems better though.
There was a problem hiding this comment.
afaict it shouldn't need to be closed.
| get(a, noAuthn, "/api/settings", a.GetSettings) | ||
| put(a, authn, "/api/settings", a.UpdateSettings) | ||
| add(a, noAuthn, route[api.EmptyRequest, WellKnownJWKResponse]{ | ||
| get(a, noAuthnWithOrg, "/api/providers/:id", a.GetProvider) |
There was a problem hiding this comment.
There's probably no way for someone to guess a provider ID, but would we want to also return fake data here?
There was a problem hiding this comment.
we might need to. Not entirely sure yet. provider IDs are somewhat guessable if you have an unlimited number of guesses. Also I think there might be problems with generating reliably-convincing fake data. especially since we're open source.
| @@ -16,3 +23,15 @@ func (a *API) addRewrites() { | |||
| // addRedirects for API endpoints that have moved to a different path | |||
| func (a *API) addRedirects() { | |||
| } | |||
|
|
|||
| func (a *API) deprecatedRoutes(noAuthnNoOrg *gin.RouterGroup) { | |||
There was a problem hiding this comment.
This feels weird since there would be no way to add deprecated routes to other RouterGroups.
There was a problem hiding this comment.
yeah, you'd have to pass in the other router group as an argument. I could pass in the base router group and let you redefine your own middleware etc, but that felt weird too
Summary
Checklist
Related Issues
Resolves #