-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
feat: Domain-wide delegation for Google Calendar and Google Meet #16622
base: main
Are you sure you want to change the base?
Conversation
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
1700348
to
151c3be
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
e123b08
to
6c94a0c
Compare
6c94a0c
to
02529ca
Compare
E2E results are ready! |
02529ca
to
ddade88
Compare
…n-google-calendar
1ef9b76
to
60a930c
Compare
everything seems fine in this pr regarding compatibility with platform atoms |
@@ -90,6 +90,7 @@ function DelegationListItemActions({ | |||
{ | |||
id: "delete", | |||
label: t("delete"), | |||
disabled: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not allow deletion of DWD as of now. We would enable it later.
const metadata = eventTypeMetaDataSchemaWithTypedApps.parse(eventType?.metadata); | ||
const eventManager = new EventManager(user, metadata?.apps); | ||
const eventManager = new EventManager({ ...user, credentials: allCredentials }, metadata?.apps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to provide DWD credentials here as well.
@@ -185,7 +185,7 @@ async function verifyCredentialsAndGetId({ | |||
currentCredentialId: number | null; | |||
}) { | |||
if (parsedBody.integration && parsedBody.externalId) { | |||
const calendarCredentials = getCalendarCredentials(userCredentials); | |||
const calendarCredentials = getCalendarCredentialsWithoutDwd(userCredentials); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No support for DWD in v1
const domainWideDelegationRepository = await DomainWideDelegation.init(user.id, organizationId); | ||
|
||
const createdDelegation = await domainWideDelegationRepository.create({ | ||
const createdDelegation = await DomainWideDelegationRepository.create({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the need of feature repository for DWD as we already have DWD toggle on organization level
userLevelSelectedCalendars: TCalendar[]; | ||
}; | ||
|
||
export function withSelectedCalendars< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move in its own module
@@ -39,6 +39,16 @@ export class CalendarsService { | |||
private readonly selectedCalendarsRepository: SelectedCalendarsRepository | |||
) {} | |||
|
|||
private buildNonDwdCredentials<TCredential>(credentials: TCredential[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't import from domainWideDelegation due to build issues. So, created a local copy here.
disabled={shouldDisableInstallation} | ||
color="primary" | ||
size="base" | ||
{...props}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was causing props.disabled to override the disabled property above that in turn caused Install button to be not disabled.
cfa7d7b
to
d9425a3
Compare
d9425a3
to
bfd156b
Compare
bfd156b
to
0d0ca15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Tested few scenarios and seems to be working fine.
will fully review on monday |
bb194f7
to
4079e31
Compare
We need to encrypt the serviceAccountKey as suggsted by Keith. I will start with the implementation and we can merge only after that, should be quick though. |
What does this PR do?
Demo - https://www.loom.com/share/1f9e7c79697847a8bb37490e9873fb26
For feature details refer to README
Detailed Changes
getCalendar
now requires value of typeCredentialForCalendarService
which needs an explicit value fordelegatedTo
. It makes it clear to CalendarService how that Credential should be used.delegatedTo=null
.delegatedTo={ serviceAccountKey: { client_email: string; private_key: string; client_id: string;}}
google-calendar/CalendarService
now supports impersonating a user using the ServiceAccount and thus is capable of taking actions on any of the organization member's behalf on the Google Calendar.Calendar Cache support is part of #18619
Release Plan
- Observe errors in Sentry and Axiom.
Automated Tests
Slots Availability - getSchedule.test.ts
Google Calendar Service - Calendar.test.ts
DomainWideDelegation utiltiies and repository - server.test.ts and domainWideDelegation.test.ts
Updating/Setting destinationCalendar - setDestinationCalendar.handler.test.ts
Doing a booking - domain-wide-delegation.test.ts
Fixes CAL-4610
Possible errors when enabling DWD
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?