admindb: InviteService #55
Conversation
|
As these things go, most of the diff is from the new generated code for invites in mockdb and sqlite/models (sqlboiler). |
| // It uses sql-migrate (github.com/rubenv/sql-migrate) for it's schema definition and maintainace. | ||
| // For query construction/ORM it uses SQLBoiler (https://github.com/volatiletech/sqlboiler). | ||
| // | ||
| // The process of updating the schema and ORM can be summarized as follows: |
There was a problem hiding this comment.
I think I never spelled out how updating the sqlite impl was supposed to go. Hope this makes it clearer.
|
I'd prefer if we use godoc for package and function level documentation and READMEs for meta/setup level stuff. I usually use |
| allowList *AllowList | ||
| } | ||
|
|
||
| const tokenLength = 50 |
There was a problem hiding this comment.
magic number? or why is it 50?
There was a problem hiding this comment.
The rooms2 doc currently leaves this open. cc @staltz
There was a problem hiding this comment.
What's the size of normal pub invite codes? Should be the same as this one too, no? Is this something that needs to be spec'd? (I don't see why all the rooms would have to have the same tokenLengths)
There was a problem hiding this comment.
Same for the choice of encoding the tokens. url safe base64 seemed like a better choice then hex.
Also not super sure it needs to be defined strictly since it just needs to be returned and passed back as an opaque string.
There was a problem hiding this comment.
What's the size of normal pub invite codes?
In classic ssb-invites it's a seed for a keypair for ed22519, which is 32bytes. But that protocol is not susceptible to brute forcing an HTTP endpoint, which is why I made it longer.
There was a problem hiding this comment.
👍 👍 👍
edit: supposed to be a comment on make interface assertion comments less scary
| } | ||
|
|
||
| invs = make([]admindb.Invite, len(entries)) | ||
| for i, e := range entries { |
There was a problem hiding this comment.
overloading i Invites with i range count
There was a problem hiding this comment.
yikes. yea, that's bad.
I just checked and noticed the other sqlite sub-service also have inconsistent names.
How about having s for service as the last letter?
So
func (is Invites) ...func (als AllowList) ...func (afs AuthFallback) ..func (as AliasService) ...func (ns Notices) ...
Similar to the package name change I'd prefer this as a follow up, though.
There was a problem hiding this comment.
oops didn't respond: i really like the idea of keeping things cohesive! i'm ambivalent on using s, i don't really know the code well enough yet to have a feeling for how it works. some weird semantic clashes with is, as, ns but 🤷
edit: i think maybe picking good-enough names & then using them consistently is more important than all names conforming to some schema that doesn't work out super duper in practice? bonus points for just tossing them all in a terminology document, so that people can easily look them up (never seen this anywhere, but it could be hecka useful at the earliest stages of getting into a codebase i think)
admindb/sqlite/new.go
Outdated
| go func() { // server might not restart as often | ||
| threeDays := 5 * 24 * time.Hour | ||
| ticker := time.NewTicker(threeDays) | ||
| for range ticker.C { | ||
| err := transact(db, func(tx *sql.Tx) error { | ||
| _, err := models.Invites(qm.Where("active = false")).DeleteAll(context.Background(), tx) | ||
| return err | ||
| }) | ||
| if err != nil { | ||
| // TODO: hook up logging | ||
| log.Printf("admindb: failed to clean up old invites: %s", err.Error()) | ||
| } | ||
| } | ||
| }() |
interface methods: create, consume, list and revoke. SQLite implementation and some light testing. Related changes: * have authfallback.Create return the user id At some point we will need to not assume that authfallback is our users table but that will not become relevant before we start adding moderation roles. * Update package documentation of admindb and admindb/sqlite * remove leftover generated.db now using the roomdb file created by TestSimple Review comments by @cblgh * better documentation of hashed token storage * space between %d and `bytes` * make interface assertion comments less scary
updates #13