Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements findUserByMe endpoint + trimmings #19

Merged
merged 7 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/server/pactasrv/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ go_library(
"//db",
"//openapi:pacta_generated",
"//pacta",
"@org_uber_go_zap//:zap",
"@com_github_go_chi_jwtauth_v5//:jwtauth",
],
)
23 changes: 19 additions & 4 deletions cmd/server/pactasrv/conv_pacta_to_oapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +29 to +31
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return &api.User{
Admin: user.Admin,
CanonicalEmail: &user.CanonicalEmail,
EnteredEmail: user.EnteredEmail,
Id: string(user.ID),
Name: user.Name,
PreferredLanguage: api.UserPreferredLanguage(user.PreferredLanguage),
SuperAdmin: user.SuperAdmin,
}, nil
}

func pactaVersionToOAPI(pv *pacta.PACTAVersion) (*api.PactaVersion, error) {
if pv == nil {
return nil, errorInternal(fmt.Errorf("pactaVersionToOAPI: can't convert nil pointer"))
}
return &api.PactaVersion{
CreatedAt: pv.CreatedAt,
Description: pv.Description,
Digest: pv.Digest,
Id: string(pv.ID),
Name: pv.Name,
IsDefault: pv.IsDefault,
Digest: pv.Digest,
Description: pv.Description,
CreatedAt: pv.CreatedAt,
Name: pv.Name,
}, nil
}

Expand Down
3 changes: 3 additions & 0 deletions cmd/server/pactasrv/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,6 @@ func (response apiError) VisitFindUserByIdResponse(w http.ResponseWriter) error
func (response apiError) VisitUpdateUserResponse(w http.ResponseWriter) error {
return response.visit(w)
}
func (response apiError) VisitFindUserByMeResponse(w http.ResponseWriter) error {
return response.visit(w)
}
3 changes: 1 addition & 2 deletions cmd/server/pactasrv/pactasrv.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,9 @@ type DB interface {
CreatePortfolioInitiativeMembership(tx db.Tx, pim *pacta.PortfolioInitiativeMembership) error
DeletePortfolioInitiativeMembership(tx db.Tx, pid pacta.PortfolioID, iid pacta.InitiativeID) error

GetOrCreateUserByAuthn(tx db.Tx, authnMechanism pacta.AuthnMechanism, authnID, enteredEmail, canonicalEmail string) (*pacta.User, error)
User(tx db.Tx, id pacta.UserID) (*pacta.User, error)
UserByAuthn(tx db.Tx, authnMechanism pacta.AuthnMechanism, authnID string) (*pacta.User, error)
Users(tx db.Tx, ids []pacta.UserID) (map[pacta.UserID]*pacta.User, error)
CreateUser(tx db.Tx, u *pacta.User) (pacta.UserID, error)
UpdateUser(tx db.Tx, id pacta.UserID, mutations ...db.UpdateUserFn) error
DeleteUser(tx db.Tx, id pacta.UserID) error
}
Expand Down
50 changes: 50 additions & 0 deletions cmd/server/pactasrv/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ package pactasrv

import (
"context"
"fmt"

api "github.com/RMI/pacta/openapi/pacta"
"github.com/RMI/pacta/pacta"
"github.com/go-chi/jwtauth/v5"
)

// Returns a user by ID
Expand Down Expand Up @@ -50,3 +53,50 @@ func (s *Server) deleteUser(ctx context.Context, request api.DeleteUserRequestOb
// TODO(#12) Implement Authorization
return errorNotImplemented("deleteUser")
}

// Returns the logged in user
// (GET /user/me)
func (s *Server) FindUserByMe(ctx context.Context, request api.FindUserByMeRequestObject) (api.FindUserByMeResponseObject, error) {
user, err := s.findUserByMe(ctx, request)
if err != nil {
return errToAPIError(err)
}
return api.FindUserByMe200JSONResponse(*user), nil
}

func (s *Server) findUserByMe(ctx context.Context, request api.FindUserByMeRequestObject) (*api.User, error) {
token, _, err := jwtauth.FromContext(ctx)
if err != nil {
// TODO(grady) upgrade this to the new error handling strategy after #12
return nil, errorUnauthorized("lookup self", "without authorization token")
}
if token == nil {
return nil, errorUnauthorized("lookup self ", "without authorization token")
}
emailsClaim, ok := token.PrivateClaims()["emails"]
if !ok {
return nil, errorUnauthorized("lookup self", "without email claim in token")
}
emails, ok := emailsClaim.([]string)
if !ok || len(emails) == 0 {
return nil, errorInternal(fmt.Errorf("couldn't parse email claim in token"))
}
// TODO(#18) Handle Multiple Emails in the Token Claims gracefully
if len(emails) > 1 {
return nil, errorBadRequest("jwt token", "multiple emails in token")
}
email := emails[0]
canonical, err := pacta.CanonicalizeEmail(email)
if err != nil {
return nil, errorBadRequest("email", "invalid email on token")
}
authnID := token.Subject()
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, email, canonical)
if err != nil {
return nil, fmt.Errorf("getting user by authn: %w", err)
}
return userToOAPI(user)
}
8 changes: 4 additions & 4 deletions db/sqldb/initiative_user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestCreateInitiativeUserRelationship(t *testing.T) {
PACTAVersion: &pacta.PACTAVersion{ID: pvID},
}
err1 := tdb.CreateInitiative(tx, i)
uid, err2 := tdb.CreateUser(tx, &pacta.User{
uid, err2 := tdb.createUser(tx, &pacta.User{
CanonicalEmail: "canon",
EnteredEmail: "entered",
AuthnMechanism: pacta.AuthnMechanism_EmailAndPass,
Expand Down Expand Up @@ -74,7 +74,7 @@ func TestUpdateInitiativeUserRelationship(t *testing.T) {
PACTAVersion: &pacta.PACTAVersion{ID: pvID},
}
err1 := tdb.CreateInitiative(tx, i)
uid, err2 := tdb.CreateUser(tx, &pacta.User{
uid, err2 := tdb.createUser(tx, &pacta.User{
CanonicalEmail: "canon",
EnteredEmail: "entered",
AuthnMechanism: pacta.AuthnMechanism_EmailAndPass,
Expand Down Expand Up @@ -141,13 +141,13 @@ func TestListInitiativeUserRelationships(t *testing.T) {
PACTAVersion: &pacta.PACTAVersion{ID: pvID},
}
err2 := tdb.CreateInitiative(tx, i2)
u1, err3 := tdb.CreateUser(tx, &pacta.User{
u1, err3 := tdb.createUser(tx, &pacta.User{
CanonicalEmail: "canon",
EnteredEmail: "entered",
AuthnMechanism: pacta.AuthnMechanism_EmailAndPass,
AuthnID: "A",
})
u2, err4 := tdb.CreateUser(tx, &pacta.User{
u2, err4 := tdb.createUser(tx, &pacta.User{
CanonicalEmail: "canon2",
EnteredEmail: "entered2",
AuthnMechanism: pacta.AuthnMechanism_EmailAndPass,
Expand Down
37 changes: 35 additions & 2 deletions db/sqldb/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (d *DB) User(tx db.Tx, id pacta.UserID) (*pacta.User, error) {
return exactlyOne("user", id, us)
}

func (d *DB) UserByAuthn(tx db.Tx, authnMechanism pacta.AuthnMechanism, authnID string) (*pacta.User, error) {
func (d *DB) userByAuthn(tx db.Tx, authnMechanism pacta.AuthnMechanism, authnID string) (*pacta.User, error) {
rows, err := d.query(tx, `
SELECT `+userSelectColumns+`
FROM pacta_user
Expand All @@ -52,6 +52,39 @@ func (d *DB) UserByAuthn(tx db.Tx, authnMechanism pacta.AuthnMechanism, authnID
return exactlyOne("user", fmt.Sprintf("%s:%s", authnMechanism, authnID), us)
}

func (d *DB) GetOrCreateUserByAuthn(tx db.Tx, authnMechanism pacta.AuthnMechanism, authnID, enteredEmail, canonicalEmail string) (*pacta.User, error) {
var user *pacta.User
err := d.RunOrContinueTransaction(tx, func(tx db.Tx) error {
u, err := d.userByAuthn(tx, authnMechanism, authnID)
if err == nil {
user = u
return nil
}
if !db.IsNotFound(err) {
return fmt.Errorf("looking up user by authn: %w", err)
}
uID, err := d.createUser(tx, &pacta.User{
CanonicalEmail: canonicalEmail,
EnteredEmail: enteredEmail,
AuthnMechanism: authnMechanism,
AuthnID: authnID,
})
if err != nil {
return fmt.Errorf("creating user: %w", err)
}
u, err = d.User(tx, uID)
if err != nil {
return fmt.Errorf("reading back created user: %w", err)
}
user = u
return nil
})
if err != nil {
return nil, fmt.Errorf("running get_or_create_user txn: %w", err)
}
return user, nil
}

func (d *DB) Users(tx db.Tx, ids []pacta.UserID) (map[pacta.UserID]*pacta.User, error) {
ids = dedupeIDs(ids)
rows, err := d.query(tx, `
Expand All @@ -72,7 +105,7 @@ func (d *DB) Users(tx db.Tx, ids []pacta.UserID) (map[pacta.UserID]*pacta.User,
return result, nil
}

func (d *DB) CreateUser(tx db.Tx, u *pacta.User) (pacta.UserID, error) {
func (d *DB) createUser(tx db.Tx, u *pacta.User) (pacta.UserID, error) {
if err := validateUserForCreation(u); err != nil {
return "", fmt.Errorf("validating user for creation: %w", err)
}
Expand Down
30 changes: 15 additions & 15 deletions db/sqldb/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"github.com/google/go-cmp/cmp/cmpopts"
)

func TestCreateUser(t *testing.T) {
func TestcreateUser(t *testing.T) {
ctx := context.Background()
tdb := createDBForTesting(t)
tx := tdb.NoTxn(ctx)
Expand All @@ -23,7 +23,7 @@ func TestCreateUser(t *testing.T) {
CanonicalEmail: "canonical-email",
Name: "User's Name",
}
userID, err := tdb.CreateUser(tx, u)
userID, err := tdb.createUser(tx, u)
if err != nil {
t.Fatalf("creating user: %v", err)
}
Expand All @@ -40,7 +40,7 @@ func TestCreateUser(t *testing.T) {
}

// Read by Authn
actual, err = tdb.UserByAuthn(tx, u.AuthnMechanism, u.AuthnID)
actual, err = tdb.userByAuthn(tx, u.AuthnMechanism, u.AuthnID)
if err != nil {
t.Fatalf("getting user by authn: %w", err)
}
Expand All @@ -63,7 +63,7 @@ func TestCreateUser(t *testing.T) {
u2 := u.Clone()
u2.EnteredEmail = "entered email 2"
u2.CanonicalEmail = "canonical email 2"
_, err = tdb.CreateUser(tx, u2)
_, err = tdb.createUser(tx, u2)
if err == nil {
t.Fatalf("expected error, got nil")
}
Expand All @@ -72,7 +72,7 @@ func TestCreateUser(t *testing.T) {
u3 := u.Clone()
u3.EnteredEmail = "entered email 3"
u3.AuthnID = "AUthn id 3"
_, err = tdb.CreateUser(tx, u3)
_, err = tdb.createUser(tx, u3)
if err == nil {
t.Fatalf("expected error, got nil")
}
Expand All @@ -81,7 +81,7 @@ func TestCreateUser(t *testing.T) {
u4 := u.Clone()
u4.AuthnID = "authn id 3"
u4.CanonicalEmail = "canonical email 4"
_, err = tdb.CreateUser(tx, u4)
_, err = tdb.createUser(tx, u4)
if err == nil {
t.Fatalf("expected error, got nil")
}
Expand All @@ -91,7 +91,7 @@ func TestCreateUser(t *testing.T) {
u5.EnteredEmail = "entered email 5"
u5.AuthnID = "AUthn id 5"
u5.CanonicalEmail = "canonical email 5"
_, err = tdb.CreateUser(tx, u5)
_, err = tdb.createUser(tx, u5)
if err != nil {
t.Fatal("expected success but got: %w", err)
}
Expand All @@ -108,7 +108,7 @@ func TestUpdateUser(t *testing.T) {
CanonicalEmail: "canonical-email",
Name: "User's Name",
}
userID, err0 := tdb.CreateUser(tx, u)
userID, err0 := tdb.createUser(tx, u)
noErrDuringSetup(t, err0)
u.CreatedAt = time.Now()
u.ID = userID
Expand Down Expand Up @@ -175,13 +175,13 @@ func TestListUsers(t *testing.T) {
CanonicalEmail: "cannnnon",
EnteredEmail: "enter3",
}
userIDA, err0 := tdb.CreateUser(tx, userA)
userIDA, err0 := tdb.createUser(tx, userA)
userA.ID = userIDA
userA.CreatedAt = time.Now()
userIDB, err1 := tdb.CreateUser(tx, userB)
userIDB, err1 := tdb.createUser(tx, userB)
userB.ID = userIDB
userB.CreatedAt = time.Now()
userIDC, err2 := tdb.CreateUser(tx, userC)
userIDC, err2 := tdb.createUser(tx, userC)
userC.ID = userIDC
userC.CreatedAt = time.Now()
err3 := tdb.UpdateUser(tx, userIDA, db.SetUserName(nameA))
Expand Down Expand Up @@ -215,7 +215,7 @@ func TestDeleteUser(t *testing.T) {
CanonicalEmail: "canonical-email",
Name: "User's Name",
}
userID, err0 := tdb.CreateUser(tx, u)
userID, err0 := tdb.createUser(tx, u)
noErrDuringSetup(t, err0)

err := tdb.DeleteUser(tx, userID)
Expand All @@ -230,7 +230,7 @@ func TestDeleteUser(t *testing.T) {
}

// Read by Authn
_, err = tdb.UserByAuthn(tx, u.AuthnMechanism, u.AuthnID)
_, err = tdb.userByAuthn(tx, u.AuthnMechanism, u.AuthnID)
if err == nil {
t.Fatalf("expected error, got nil")
}
Expand Down Expand Up @@ -286,7 +286,7 @@ func testUserEnumConvertability[E comparable](t *testing.T, writeE func(E, *pact
u.CanonicalEmail = fmt.Sprintf("canonical-email-%d", iteration)
writeE(e, u)
iteration++
id2, err := tdb.CreateUser(tx, u)
id2, err := tdb.createUser(tx, u)
id = id2
return err
}
Expand Down Expand Up @@ -332,7 +332,7 @@ func userForTestingWithKey(t *testing.T, tdb *DB, key string) *pacta.User {
}
ctx := context.Background()
tx := tdb.NoTxn(ctx)
uid, err := tdb.CreateUser(tx, u)
uid, err := tdb.createUser(tx, u)
if err != nil {
t.Fatalf("creating user: %v", err)
}
Expand Down
7 changes: 7 additions & 0 deletions deps.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,13 @@ def go_dependencies():
sum = "h1:vV6w1AhK4VMnhBno/TPVCoK9U/LP0PkLCS9tbxHdi/U=",
version = "v1.1.1",
)
# Re-review this package if upgraded.
go_repository(
name = "com_github_dimuska139_go_email_normalizer",
importpath = "github.com/dimuska139/go-email-normalizer",
sum = "h1:pJNZnU7uS9MRoYqpoir05B+bCYXrS9sPGE4G1o9EDA8=",
version = "v1.2.1",
)
gbdubs marked this conversation as resolved.
Show resolved Hide resolved

go_repository(
name = "com_github_docker_distribution",
Expand Down
2 changes: 1 addition & 1 deletion frontend/components/standard/Content.vue
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
margin: 0.5rem 0;
}

min-height: calc(100vh - 9rem - 4px);
min-height: calc(100vh - 9.25rem - 4px);
justify-content: center;
}
</style>
Loading
Loading