feat: protect segment operations for change requests #4417
Conversation
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| @@ -177,7 +177,7 @@ export default class FakeProjectStore implements IProjectStore { | |||
| projectId: string, | |||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
| environment: string, | |||
| ): Promise<CreateFeatureStrategySchema | undefined> { | |||
| ): Promise<CreateFeatureStrategySchema | null> { | |||
| @@ -248,4 +283,16 @@ export class SegmentService implements ISegmentService { | |||
| ); | |||
| } | |||
| } | |||
|
|
|||
| private async stopWhenChangeRequestsEnabled(project?: string, user?: User) { | |||
There was a problem hiding this comment.
this is the most important method
| @@ -108,13 +130,26 @@ export class SegmentService implements ISegmentService { | |||
|
|
|||
| await this.eventStore.store({ | |||
| type: SEGMENT_UPDATED, | |||
| createdBy: user.email || user.username, | |||
| createdBy: user.email || user.username || 'unknown', | |||
| data: segment, | ||
| }); | ||
|
|
||
| return segment; | ||
| } | ||
|
|
||
| async update( | ||
| async update(id: number, data: unknown, user: User): Promise<void> { | ||
| if (this.flagResolver.isEnabled('segmentChangeRequests')) { |
There was a problem hiding this comment.
we have to be careful not to enable it before the UI is ready
There was a problem hiding this comment.
Fair. It's currently enabled in dev, but we can turn that off
| @@ -35,6 +35,23 @@ export class ChangeRequestAccessReadModel | |||
| return !(changeRequestEnabled && !canSkipChangeRequest); | |||
| } | |||
|
|
|||
| public async canBypassChangeRequestForProject( | |||
There was a problem hiding this comment.
no check for environment. If a user can skip change requests in any env we're good for now. We can tweak this logic later. For now need something cheap to experiment
| : Promise.resolve(false), | ||
| this.isChangeRequestsEnabledForProject(project), | ||
| ]); | ||
| return !(changeRequestEnabled && !canSkipChangeRequest); |
There was a problem hiding this comment.
Could we express this without the double negation? I'm struggling to parse what we're saying here.
There was a problem hiding this comment.
great catch, changing it to return canSkipChangeRequest || !changeRequestEnabled;
| data: segment, | ||
| }); | ||
|
|
||
| return segment; | ||
| } | ||
|
|
||
| async update( | ||
| async update(id: number, data: unknown, user: User): Promise<void> { | ||
| if (this.flagResolver.isEnabled('segmentChangeRequests')) { |
There was a problem hiding this comment.
Fair. It's currently enabled in dev, but we can turn that off
Looking at #3315 I think the plan was to use the interface to establish the contract as enterprise and OSS had different implementations. I don't remember the context but we were moving things that were on OSS to the enterprise repo and this was probably in the expansion phase of the migration and it just stayed... If it doesn't make more sense and you want to remove it, I vote we follow our conventions (and this is out of our code conventions) |
About the changes
Update segment and delete segment now have protected mode which means that with CR enabled you can't change them willy-nilly. You either need skip permission for any project environment or go through a change request.
There's a corresponding enterprise change where I have tests and fix breaking changes.
Important files
Discussion points
I noticed that we have interface for the segment service. Since this is the only service with an interface it looks odd. I'm tempted to remove it since there's only one impl of the interface and TS is structurally typed. @gastonfournier since you added the interface WDYT?