Implements findUserByMe endpoint + trimmings
#19
Conversation
| @@ -26,17 +26,32 @@ func initiativeToOAPI(i *pacta.Initiative) (*api.Initiative, error) { | |||
| }, nil | |||
| } | |||
|
|
|||
| func userToOAPI(user *pacta.User) (*api.User, error) { | |||
| if user == nil { | |||
| return nil, errorInternal(fmt.Errorf("userToOAPI: can't convert nil pointer")) | |||
There was a problem hiding this comment.
I hear your point about keeping all the conv stuff in the pactasrv package so you can access these, but my counterpoint would be that determining the 'type' of an error truly is a concern of the server, and this should just return a fmt.Errorf(...) or some conv-specific error that the srv layer then converts into an errorWhatever
Functionally, it probably doesn't matter, just cleaner abstractions and easier testing
| func userToOAPI(user *pacta.User) (*api.User, error) { | ||
| if user == nil { | ||
| return nil, errorInternal(fmt.Errorf("userToOAPI: can't convert nil pointer")) |
There was a problem hiding this comment.
I remember you had a reason for using *api.Whatever and dereference instead of just using structs directly, but it seems like a lot of contorting to be using pointers, but having to check for errors/disallow them, only to convert them back into not-pointers later
cmd/server/pactasrv/user.go
Outdated
| if authnID == "" { | ||
| return nil, fmt.Errorf("couldn't find authn id in jwt") | ||
| } | ||
| user, err := s.DB.GetOrCreateUserByAuthn(s.DB.NoTxn(ctx), pacta.AuthnMechanism_EmailAndPass, authnID, emails[0]) |
There was a problem hiding this comment.
I would not use a GetOrCreate endpoint for something like this. GetOrCreate-ing a user should happen on a dedicated login or similar endpoint, it should be an error if a user tries to do anything else without an account in the DB.
There's a bunch of reasons to do this, but the main one is to have a single location where we create accounts in the DB.
There was a problem hiding this comment.
What are the other reasons? We can remove the other CreateUser method trivially.
There was a problem hiding this comment.
The other reasons are something like:
- We shouldn't have exported methods that nobody is using, it increases the package's surface area unnecessarily
- Given the way our login system works ('get token, send to server'), the only way of account creation that makes sense is
GetOrCreateUser[...], we shouldn't make it tempting to create accounts any other way
There was a problem hiding this comment.
Wait I might be misunderstanding. When does the token from Azure get sent directly to the server? Aren't we always facilitating that exchange through the Frontend? If so wouldn't just having getOrCreateUser make sense at the FE layer too?
Agreed on the idea of one method at the DB layer. - I'll make CreateUser testonly.
There was a problem hiding this comment.
Copying image from Slack:
There was a problem hiding this comment.
The token that would go to a PACTA /login endpoint would be the RMI-specific token from step 4, and that would get or create the user account in the PACTA database
No description provided.