feat: Add profile pictures to OAuth users (#3855)

This supports GitHub and OIDC login for profile pictures!
This commit is contained in:
Kyle Carberry 2022-09-04 11:44:27 -05:00 committed by GitHub
parent 67c4605370
commit 05e2806ff3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
23 changed files with 139 additions and 33 deletions

View File

@ -1859,6 +1859,7 @@ func (q *fakeQuerier) UpdateUserProfile(_ context.Context, arg database.UpdateUs
}
user.Email = arg.Email
user.Username = arg.Username
user.AvatarURL = arg.AvatarURL
q.users[index] = user
return user, nil
}

View File

@ -298,7 +298,8 @@ CREATE TABLE users (
updated_at timestamp with time zone NOT NULL,
status user_status DEFAULT 'active'::public.user_status NOT NULL,
rbac_roles text[] DEFAULT '{}'::text[] NOT NULL,
login_type login_type DEFAULT 'password'::public.login_type NOT NULL
login_type login_type DEFAULT 'password'::public.login_type NOT NULL,
avatar_url character varying(64)
);
CREATE TABLE workspace_agents (

View File

@ -0,0 +1,2 @@
ALTER TABLE users
DROP COLUMN avatar_url;

View File

@ -0,0 +1,2 @@
ALTER TABLE users
ADD COLUMN avatar_url varchar(64);

View File

@ -494,15 +494,16 @@ type TemplateVersion struct {
}
type User struct {
ID uuid.UUID `db:"id" json:"id"`
Email string `db:"email" json:"email"`
Username string `db:"username" json:"username"`
HashedPassword []byte `db:"hashed_password" json:"hashed_password"`
CreatedAt time.Time `db:"created_at" json:"created_at"`
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
Status UserStatus `db:"status" json:"status"`
RBACRoles []string `db:"rbac_roles" json:"rbac_roles"`
LoginType LoginType `db:"login_type" json:"login_type"`
ID uuid.UUID `db:"id" json:"id"`
Email string `db:"email" json:"email"`
Username string `db:"username" json:"username"`
HashedPassword []byte `db:"hashed_password" json:"hashed_password"`
CreatedAt time.Time `db:"created_at" json:"created_at"`
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
Status UserStatus `db:"status" json:"status"`
RBACRoles []string `db:"rbac_roles" json:"rbac_roles"`
LoginType LoginType `db:"login_type" json:"login_type"`
AvatarURL sql.NullString `db:"avatar_url" json:"avatar_url"`
}
type UserLink struct {

View File

@ -2870,7 +2870,7 @@ func (q *sqlQuerier) GetAuthorizationUserRoles(ctx context.Context, userID uuid.
const getUserByEmailOrUsername = `-- name: GetUserByEmailOrUsername :one
SELECT
id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type
id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url
FROM
users
WHERE
@ -2898,13 +2898,14 @@ func (q *sqlQuerier) GetUserByEmailOrUsername(ctx context.Context, arg GetUserBy
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
)
return i, err
}
const getUserByID = `-- name: GetUserByID :one
SELECT
id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type
id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url
FROM
users
WHERE
@ -2926,6 +2927,7 @@ func (q *sqlQuerier) GetUserByID(ctx context.Context, id uuid.UUID) (User, error
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
)
return i, err
}
@ -2946,7 +2948,7 @@ func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) {
const getUsers = `-- name: GetUsers :many
SELECT
id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type
id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url
FROM
users
WHERE
@ -3039,6 +3041,7 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]User,
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
); err != nil {
return nil, err
}
@ -3054,7 +3057,7 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]User,
}
const getUsersByIDs = `-- name: GetUsersByIDs :many
SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type FROM users WHERE id = ANY($1 :: uuid [ ])
SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url FROM users WHERE id = ANY($1 :: uuid [ ])
`
func (q *sqlQuerier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]User, error) {
@ -3076,6 +3079,7 @@ func (q *sqlQuerier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]User
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
); err != nil {
return nil, err
}
@ -3103,7 +3107,7 @@ INSERT INTO
login_type
)
VALUES
($1, $2, $3, $4, $5, $6, $7, $8) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type
($1, $2, $3, $4, $5, $6, $7, $8) RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url
`
type InsertUserParams struct {
@ -3139,6 +3143,7 @@ func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
)
return i, err
}
@ -3168,16 +3173,18 @@ UPDATE
SET
email = $2,
username = $3,
updated_at = $4
avatar_url = $4,
updated_at = $5
WHERE
id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type
id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url
`
type UpdateUserProfileParams struct {
ID uuid.UUID `db:"id" json:"id"`
Email string `db:"email" json:"email"`
Username string `db:"username" json:"username"`
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
ID uuid.UUID `db:"id" json:"id"`
Email string `db:"email" json:"email"`
Username string `db:"username" json:"username"`
AvatarURL sql.NullString `db:"avatar_url" json:"avatar_url"`
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
}
func (q *sqlQuerier) UpdateUserProfile(ctx context.Context, arg UpdateUserProfileParams) (User, error) {
@ -3185,6 +3192,7 @@ func (q *sqlQuerier) UpdateUserProfile(ctx context.Context, arg UpdateUserProfil
arg.ID,
arg.Email,
arg.Username,
arg.AvatarURL,
arg.UpdatedAt,
)
var i User
@ -3198,6 +3206,7 @@ func (q *sqlQuerier) UpdateUserProfile(ctx context.Context, arg UpdateUserProfil
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
)
return i, err
}
@ -3210,7 +3219,7 @@ SET
rbac_roles = ARRAY(SELECT DISTINCT UNNEST($1 :: text[]))
WHERE
id = $2
RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type
RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url
`
type UpdateUserRolesParams struct {
@ -3231,6 +3240,7 @@ func (q *sqlQuerier) UpdateUserRoles(ctx context.Context, arg UpdateUserRolesPar
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
)
return i, err
}
@ -3242,7 +3252,7 @@ SET
status = $2,
updated_at = $3
WHERE
id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type
id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url
`
type UpdateUserStatusParams struct {
@ -3264,6 +3274,7 @@ func (q *sqlQuerier) UpdateUserStatus(ctx context.Context, arg UpdateUserStatusP
&i.Status,
pq.Array(&i.RBACRoles),
&i.LoginType,
&i.AvatarURL,
)
return i, err
}

View File

@ -57,7 +57,8 @@ UPDATE
SET
email = $2,
username = $3,
updated_at = $4
avatar_url = $4,
updated_at = $5
WHERE
id = $1 RETURNING *;

View File

@ -18,6 +18,7 @@ packages:
rename:
api_key: APIKey
avatar_url: AvatarURL
login_type_oidc: LoginTypeOIDC
oauth_access_token: OAuthAccessToken
oauth_expiry: OAuthExpiry

View File

@ -144,6 +144,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
AllowSignups: api.GithubOAuth2Config.AllowSignups,
Email: verifiedEmail.GetEmail(),
Username: ghUser.GetLogin(),
AvatarURL: ghUser.GetAvatarURL(),
})
var httpErr httpError
if xerrors.As(err, &httpErr) {
@ -207,6 +208,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
Email string `json:"email"`
Verified bool `json:"email_verified"`
Username string `json:"preferred_username"`
Picture string `json:"picture"`
}
err = idToken.Claims(&claims)
if err != nil {
@ -256,6 +258,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
AllowSignups: api.OIDCConfig.AllowSignups,
Email: claims.Email,
Username: claims.Username,
AvatarURL: claims.Picture,
})
var httpErr httpError
if xerrors.As(err, &httpErr) {
@ -292,6 +295,7 @@ type oauthLoginParams struct {
AllowSignups bool
Email string
Username string
AvatarURL string
}
type httpError struct {
@ -410,6 +414,15 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
}
}
needsUpdate := false
if user.AvatarURL.String != params.AvatarURL {
user.AvatarURL = sql.NullString{
String: params.AvatarURL,
Valid: true,
}
needsUpdate = true
}
// If the upstream email or username has changed we should mirror
// that in Coder. Many enterprises use a user's email/username as
// security auditing fields so they need to stay synced.
@ -417,6 +430,11 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
// provisioning consequences (updates to usernames may delete persistent
// resources such as user home volumes).
if user.Email != params.Email {
user.Email = params.Email
needsUpdate = true
}
if needsUpdate {
// TODO(JonA): Since we're processing updates to a user's upstream
// email/username, it's possible for a different built-in user to
// have already claimed the username.
@ -425,9 +443,10 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
// user and changes their username.
user, err = tx.UpdateUserProfile(ctx, database.UpdateUserProfileParams{
ID: user.ID,
Email: params.Email,
Email: user.Email,
Username: user.Username,
UpdatedAt: database.Now(),
AvatarURL: user.AvatarURL,
})
if err != nil {
return xerrors.Errorf("update user profile: %w", err)

View File

@ -226,10 +226,11 @@ func TestUserOAuth2Github(t *testing.T) {
},
}}, nil
},
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
AuthenticatedUser: func(ctx context.Context, _ *http.Client) (*github.User, error) {
return &github.User{
Login: github.String("kyle"),
ID: i64ptr(1234),
Login: github.String("kyle"),
ID: i64ptr(1234),
AvatarURL: github.String("/hello-world"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -249,6 +250,7 @@ func TestUserOAuth2Github(t *testing.T) {
require.NoError(t, err)
require.Equal(t, "kyle@coder.com", user.Email)
require.Equal(t, "kyle", user.Username)
require.Equal(t, "/hello-world", user.AvatarURL)
})
t.Run("SignupAllowedTeam", func(t *testing.T) {
t.Parallel()
@ -297,6 +299,7 @@ func TestUserOIDC(t *testing.T) {
AllowSignups bool
EmailDomain string
Username string
AvatarURL string
StatusCode int
}{{
Name: "EmailNotVerified",
@ -357,6 +360,18 @@ func TestUserOIDC(t *testing.T) {
Username: "kyle",
AllowSignups: true,
StatusCode: http.StatusTemporaryRedirect,
}, {
Name: "WithPicture",
Claims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"username": "kyle",
"picture": "/example.png",
},
Username: "kyle",
AllowSignups: true,
AvatarURL: "/example.png",
StatusCode: http.StatusTemporaryRedirect,
}} {
tc := tc
t.Run(tc.Name, func(t *testing.T) {
@ -379,6 +394,13 @@ func TestUserOIDC(t *testing.T) {
require.NoError(t, err)
require.Equal(t, tc.Username, user.Username)
}
if tc.AvatarURL != "" {
client.SessionToken = resp.Cookies()[0].Value
user, err := client.User(ctx, "me")
require.NoError(t, err)
require.Equal(t, tc.AvatarURL, user.AvatarURL)
}
})
}

View File

@ -391,6 +391,7 @@ func (api *API) putUserProfile(rw http.ResponseWriter, r *http.Request) {
updatedUserProfile, err := api.Database.UpdateUserProfile(r.Context(), database.UpdateUserProfileParams{
ID: user.ID,
Email: user.Email,
AvatarURL: user.AvatarURL,
Username: params.Username,
UpdatedAt: database.Now(),
})
@ -1075,6 +1076,7 @@ func convertUser(user database.User, organizationIDs []uuid.UUID) codersdk.User
Status: codersdk.UserStatus(user.Status),
OrganizationIDs: organizationIDs,
Roles: make([]codersdk.Role, 0, len(user.RBACRoles)),
AvatarURL: user.AvatarURL.String,
}
for _, roleName := range user.RBACRoles {

View File

@ -50,6 +50,7 @@ type User struct {
Status UserStatus `json:"status" table:"status"`
OrganizationIDs []uuid.UUID `json:"organization_ids"`
Roles []Role `json:"roles"`
AvatarURL string `json:"avatar_url"`
}
type APIKey struct {

View File

@ -83,6 +83,7 @@ var AuditableResources = auditMap(map[any]map[string]Action{
"status": ActionTrack,
"rbac_roles": ActionTrack,
"login_type": ActionIgnore,
"avatar_url": ActionIgnore,
},
&database.Workspace{}: {
"id": ActionTrack,

View File

@ -458,6 +458,7 @@ export interface User {
readonly status: UserStatus
readonly organization_ids: string[]
readonly roles: Role[]
readonly avatar_url: string
}
// From codersdk/users.go

View File

@ -51,6 +51,7 @@ describe("NavbarView", () => {
const mockUser = {
...MockUser,
username: "bryan",
avatar_url: "",
}
// When

View File

@ -5,8 +5,17 @@ import { firstLetter } from "../../util/firstLetter"
export interface UserAvatarProps {
className?: string
username: string
avatarURL: string
}
export const UserAvatar: FC<UserAvatarProps> = ({ username, className }) => {
return <Avatar className={className}>{firstLetter(username)}</Avatar>
export const UserAvatar: FC<UserAvatarProps> = ({ username, className, avatarURL }) => {
return (
<Avatar className={className}>
{avatarURL ? (
<img alt={`${username}'s Avatar`} src={avatarURL} width="100%" />
) : (
firstLetter(username)
)}
</Avatar>
)
}

View File

@ -13,6 +13,7 @@ export const AuditLogExample = Template.bind({})
AuditLogExample.args = {
Avatar: {
username: MockUser.username,
avatarURL: "",
},
caption: MockUserAgent.ip_address,
primaryText: MockUser.email,
@ -25,6 +26,7 @@ export const AuditLogEmptyUserExample = Template.bind({})
AuditLogEmptyUserExample.args = {
Avatar: {
username: MockUser.username,
avatarURL: "",
},
caption: MockUserAgent.ip_address,
primaryText: "Deleted User",

View File

@ -7,6 +7,7 @@ namespace Helpers {
export const Props: UserCellProps = {
Avatar: {
username: MockUser.username,
avatarURL: "",
},
caption: MockUserAgent.ip_address,
primaryText: MockUser.username,

View File

@ -37,7 +37,7 @@ export const UserDropdown: React.FC<React.PropsWithChildren<UserDropdownProps>>
>
<div className={styles.inner}>
<Badge overlap="circle">
<UserAvatar username={user.username} />
<UserAvatar username={user.username} avatarURL={user.avatar_url} />
</Badge>
{anchorEl ? <CloseDropdown /> : <OpenDropdown />}
</div>

View File

@ -38,7 +38,11 @@ export const UserDropdownContent: FC<UserDropdownContentProps> = ({
<div className={styles.userInfo}>
<div className={styles.root}>
<div className={styles.avatarContainer}>
<UserAvatar className={styles.avatar} username={user.username} />
<UserAvatar
className={styles.avatar}
username={user.username}
avatarURL={user.avatar_url}
/>
</div>
<Typography className={styles.userName}>{user.username}</Typography>
<Typography className={styles.userEmail}>{user.email}</Typography>

View File

@ -72,7 +72,20 @@ export const UsersTableBody: FC<React.PropsWithChildren<UsersTableBodyProps>> =
return (
<TableRow key={user.id}>
<TableCell>
<AvatarData title={user.username} subtitle={user.email} highlightTitle />
<AvatarData
title={user.username}
subtitle={user.email}
highlightTitle
avatar={
user.avatar_url ? (
<img
className={styles.avatar}
alt={`${user.username}'s Avatar`}
src={user.avatar_url}
/>
) : null
}
/>
</TableCell>
<TableCell
className={combineClasses([
@ -139,4 +152,9 @@ const useStyles = makeStyles((theme) => ({
suspended: {
color: theme.palette.text.secondary,
},
avatar: {
width: theme.spacing(4.5),
height: theme.spacing(4.5),
borderRadius: "100%",
},
}))

View File

@ -37,6 +37,7 @@ describe("AccountPage", () => {
status: "active",
organization_ids: ["123"],
roles: [],
avatar_url: "",
...data,
}),
)

View File

@ -70,6 +70,7 @@ export const MockUser: TypesGen.User = {
status: "active",
organization_ids: ["fc0774ce-cc9e-48d4-80ae-88f7a4d4a8b0"],
roles: [MockOwnerRole],
avatar_url: "https://github.com/coder.png",
}
export const MockUserAdmin: TypesGen.User = {
@ -80,6 +81,7 @@ export const MockUserAdmin: TypesGen.User = {
status: "active",
organization_ids: ["fc0774ce-cc9e-48d4-80ae-88f7a4d4a8b0"],
roles: [MockUserAdminRole],
avatar_url: "",
}
export const MockUser2: TypesGen.User = {
@ -90,6 +92,7 @@ export const MockUser2: TypesGen.User = {
status: "active",
organization_ids: ["fc0774ce-cc9e-48d4-80ae-88f7a4d4a8b0"],
roles: [],
avatar_url: "",
}
export const SuspendedMockUser: TypesGen.User = {
@ -100,6 +103,7 @@ export const SuspendedMockUser: TypesGen.User = {
status: "suspended",
organization_ids: ["fc0774ce-cc9e-48d4-80ae-88f7a4d4a8b0"],
roles: [],
avatar_url: "",
}
export const MockOrganization: TypesGen.Organization = {