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.

Sign up for GitHub

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

Fix/verify sso bug #14253

Merged
merged 15 commits into from
Jul 29, 2024
Merged

Fix/verify sso bug #14253

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
52a9b7d
Delete associated SSO user
melohagan Jul 25, 2024
7a46267
Merge branch 'master' into fix/verify-sso-bug
melohagan Jul 25, 2024
2d673a2
Merge branch 'master' into fix/verify-sso-bug
melohagan Jul 26, 2024
90d1191
Merge branch 'master' into fix/verify-sso-bug
melohagan Jul 26, 2024
f4d4932
Merge branch 'master' into fix/verify-sso-bug
melohagan Jul 26, 2024
ba9bd35
Update reset password screen
melohagan Jul 26, 2024
8752b07
Partial type removed
melohagan Jul 26, 2024
4a80e1d
Merge branch 'fix/verify-sso-bug' of https://github.com/Budibase/budi…
melohagan Jul 26, 2024
c7b6461
lint
melohagan Jul 26, 2024
3fba18e
Split getFirstPlatformUser from getPlatformUsers
melohagan Jul 29, 2024
1f01bb3
lint
melohagan Jul 29, 2024
15b4a7a
Use correct method
melohagan Jul 29, 2024
1094c95
Code review comments
melohagan Jul 29, 2024
534fb1a
Merge branch 'master' into fix/verify-sso-bug
melohagan Jul 29, 2024
3296336
lint
melohagan Jul 29, 2024
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
5 changes: 5 additions & 0 deletions packages/backend-core/src/platform/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ export async function getUserDoc(emailOrId: string): Promise<PlatformUser> {
return db.get(emailOrId)
}

export async function updateUserDoc(platformUser: Partial<PlatformUserById>) {
melohagan marked this conversation as resolved.
Show resolved Hide resolved
const db = getPlatformDB()
await db.put(platformUser)
}

// CREATE

function newUserIdDoc(id: string, tenantId: string): PlatformUserById {
Expand Down
26 changes: 25 additions & 1 deletion packages/backend-core/src/users/db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,17 @@ import {
User,
UserStatus,
UserGroup,
PlatformUserBySsoId,
PlatformUserById,
AnyDocument,
} from "@budibase/types"
import {
getAccountHolderFromUserIds,
isAdmin,
isCreator,
validateUniqueUser,
} from "./utils"
import { searchExistingEmails } from "./lookup"
import { getPlatformUser, searchExistingEmails } from "./lookup"
import { hash } from "../utils"
import { validatePassword } from "../security"

Expand Down Expand Up @@ -446,9 +449,30 @@ export class UserDB {
creator => !!creator
).length

const ssoUsersToDelete: AnyDocument[] = []
for (let user of usersToDelete) {
const platformUser = (await getPlatformUser(
user._id!
)) as PlatformUserById
melohagan marked this conversation as resolved.
Show resolved Hide resolved
const ssoId = platformUser.ssoId
if (ssoId) {
// Need to get the _rev of the SSO user doc to delete it. The view also returns docs that have the ssoId property, so we need to ignore those.
const ssoUsers = (await getPlatformUser(ssoId)) as PlatformUserBySsoId[]
ssoUsers
.filter(user => user.ssoId == null)
.forEach(user => {
ssoUsersToDelete.push({
...user,
_deleted: true,
})
})
}
await bulkDeleteProcessing(user)
}

// Delete any associated SSO user docs
await platform.getPlatformDB().bulkDocs(ssoUsersToDelete)

await UserDB.quotas.removeUsers(toDelete.length, creatorsToDeleteCount)

// Build Response
Expand Down
2 changes: 1 addition & 1 deletion packages/backend-core/src/users/lookup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export async function searchExistingEmails(emails: string[]) {
// lookup, could be email or userId, either will return a doc
export async function getPlatformUser(
identifier: string
): Promise<PlatformUser | null> {
): Promise<PlatformUser[] | PlatformUser | null> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In which case it can return an array? We are casting the result to a single user, and we are passing a single id

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It returns an array if the following Couch view finds more than one document:

const viewJs = `function(doc) {
    if (doc.tenantId) {
      emit(doc._id.toLowerCase(), doc._id)
    }

    if (doc.ssoId) {
      emit(doc.ssoId, doc._id)
    }
  }`

In this case it is these two documents:

Screenshot 2024-07-26 at 15 41 23

The document has the property ssoId, so emit that property as the key

Screenshot 2024-07-26 at 15 41 46

The document has the property tenantId, so emit the _id property as the key

This will result in two documents matching the SSO ID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is the case, we might have all the usages wrong. You are casting the results as PlatformUser, so if it returns an array we will have issues. Should we handle the possible arrays on the usages? (fe. packages/backend-core/src/users/db.ts, line 454)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update the types to just return the array, or call getFirstPlatformUser where appropriate

// use the view here and allow to find anyone regardless of casing
// Use lowercase to ensure email login is case insensitive
return (await dbUtils.queryPlatformView(ViewName.PLATFORM_USERS_LOWERCASE, {
Expand Down
10 changes: 8 additions & 2 deletions packages/backend-core/src/users/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { CloudAccount, ContextUser, User, UserGroup } from "@budibase/types"
import {
CloudAccount,
ContextUser,
PlatformUser,
User,
UserGroup,
} from "@budibase/types"
import * as accountSdk from "../accounts"
import env from "../environment"
import { getPlatformUser } from "./lookup"
Expand Down Expand Up @@ -51,7 +57,7 @@ async function isCreatorByGroupMembership(user?: User | ContextUser) {
export async function validateUniqueUser(email: string, tenantId: string) {
// check budibase users in other tenants
if (env.MULTI_TENANCY) {
const tenantUser = await getPlatformUser(email)
const tenantUser = (await getPlatformUser(email)) as PlatformUser
if (tenantUser != null && tenantUser.tenantId !== tenantId) {
throw new EmailUnavailableError(email)
}
Expand Down
3 changes: 3 additions & 0 deletions packages/types/src/documents/platform/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ export interface PlatformUserByEmail extends Document {
*/
export interface PlatformUserById extends Document {
tenantId: string
email?: string
ssoId?: string
}

/**
Expand All @@ -22,6 +24,7 @@ export interface PlatformUserBySsoId extends Document {
tenantId: string
userId: string
email: string
ssoId?: string
}

export type PlatformUser =
Expand Down
12 changes: 11 additions & 1 deletion packages/worker/src/api/controllers/global/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
LockName,
LockType,
MigrationType,
PlatformUserById,
PlatformUserByEmail,
SaveUserResponse,
SearchUsersRequest,
Expand Down Expand Up @@ -62,7 +63,7 @@ export const addSsoSupport = async (ctx: Ctx<AddSSoUserRequest>) => {
const { email, ssoId } = ctx.request.body
try {
// Status is changed to 404 from getUserDoc if user is not found
let userByEmail = (await platform.users.getUserDoc(
const userByEmail = (await platform.users.getUserDoc(
email
)) as PlatformUserByEmail
await platform.users.addSsoUser(
Expand All @@ -71,6 +72,15 @@ export const addSsoSupport = async (ctx: Ctx<AddSSoUserRequest>) => {
userByEmail.userId,
userByEmail.tenantId
)
// Need to get the _rev of the user doc to update
const userById = (await platform.users.getUserDoc(
userByEmail.userId
)) as PlatformUserById
melohagan marked this conversation as resolved.
Show resolved Hide resolved
await platform.users.updateUserDoc({
...userById,
email,
ssoId,
})
ctx.status = 200
} catch (err: any) {
ctx.throw(err.status || 400, err)
Expand Down
Loading