permissions-center: expose new `permissionSyncJobs` GraphQL API. by sashaostrikov · Pull Request #47933 · sourcegraph/sourcegraph-public-snapshot

sashaostrikov · 2023-02-21T08:29:30Z

This substitutes experimental permissionsSyncJobs API and removes storing permission sync stats into Redis.

Test plan:
Unit tests updated and added.
Local sg run and API query tests.

Closes https://github.com/sourcegraph/sourcegraph/issues/47731.
Closes https://github.com/sourcegraph/sourcegraph/issues/47308.

sourcegraph-bot · 2023-02-21T08:31:21Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff ccfa75c...a0efa5d.

Notify	File(s)
@indradhanush	enterprise/cmd/repo-updater/internal/authz/perms_syncer.go enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker.go
@unknwon	enterprise/cmd/frontend/internal/authz/init.go enterprise/cmd/frontend/internal/authz/resolvers/permission_sync_jobs.go enterprise/cmd/frontend/internal/authz/resolvers/permission_sync_jobs_test.go enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go enterprise/cmd/frontend/internal/authz/resolvers/resolver.go enterprise/cmd/frontend/internal/authz/resolvers/resolver_test.go enterprise/cmd/repo-updater/internal/authz/perms_syncer.go enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker.go enterprise/internal/authz/syncjobs/BUILD.bazel enterprise/internal/authz/syncjobs/mocks_test.go enterprise/internal/authz/syncjobs/records_reader.go enterprise/internal/authz/syncjobs/records_reader_test.go enterprise/internal/authz/syncjobs/records_store.go enterprise/internal/authz/syncjobs/records_store_test.go enterprise/internal/authz/syncjobs/status.go

sashaostrikov · 2023-02-21T08:32:45Z

Currently we use a reason as a message, I'm still not 100% sure should we convert stuff like REASON_USER_EMAIL_REMOVED to more user friendly string in frontend or backend.

Anyway, I will add this function to our backend/UI code in one of the following PRs.

Umm, I won't mind doing it on frontend eve. Your choice.

sg-e2e-regression-test-bob · 2023-02-21T08:36:44Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.02% (+0.51 kb)	0.00% (+0.58 kb)	0.00% (+0.07 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 309a464 and ccfa75c or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

0xnmn · 2023-02-21T10:08:35Z

I just realised we are not showing the priority on the frontend. Might be a good idea to show it somehow.

0xnmn · 2023-02-21T10:11:38Z

What is PermissionSyncJobProvider ?

this is effectively a code host AFAIR
described here https://github.com/sourcegraph/sourcegraph/issues/47301

0xnmn · 2023-02-21T10:18:09Z

NO, any user can also trigger manual sync for themselves.

It should be CheckSiteAdminOrSameUser

hmm, this is an unintended change, something weird must have happened during rebase. Will revert it

0xnmn · 2023-02-21T10:18:35Z

NO, any user can also view their own perms info.

It should be CheckSiteAdminOrSameUser

☝️ This. I recently changed it so that users can view the info as well, so please don't take it away 😄

0xnmn · 2023-02-21T10:20:45Z

First will be nil in case of backward pagination. This condition is wrong.

You should be able to remove this check safely.

0xnmn

Everything looks good except the 2 security checks. Non-site admin users can also view their own perms info and trigger a manual sync for themselves.

This substitutes experimental `permissionsSyncJobs` API and removes storing permission sync stats into Redis. Test plan: Unit tests updated and added. Local sg run and API query tests.

kopancek · 2023-02-21T09:24:31Z

What about this TODO? Where should the link point to? This PR? :)

nice, forgot about it 😬

kopancek · 2023-02-21T09:24:48Z

Same as above

kopancek · 2023-02-21T09:26:02Z

Why did we change from a pointer to a non-pointer on args? As far as I see, the graphql schema is defined in almost the same way, so would like to understand this for my own sake.

ListPermissionSyncJobsArgs is an interface (we sometimes declare interfaces, sometimes don't, I just like the former approach)

kopancek · 2023-02-21T09:27:58Z

What is the difference between MANUAL and SOURCEGRAPH?

MANUAL is a manual sync for a user triggered by user.
SOURCEGRAPH is a sync due to internal sg event (email is added/deleted, user is added to sg org, etc.)

kopancek · 2023-02-21T11:52:11Z

What if both UserID and RepositoryID are 0? Should we return an error in that case?

it is mutually exclusive via DB constraint.
Should not happen (c)

kopancek · 2023-02-21T11:53:05Z

☝️ This. I recently changed it so that users can view the info as well, so please don't take it away 😄

kopancek · 2023-02-21T11:55:08Z

A unit test would be nice for a lib function like this...

added here!

bobheadxi · 2023-02-21T18:56:20Z

 ### Changed

-
+- Experimental GraphQL query, `permissionsSyncJobs` is removed and substituted with new non-experimental `permissionSyncJobs` query (mind the singular form of permission) which provides full information about permission sync jobs stored in the database. `authz.syncJobsRecordsTTL`. [#47933](https://github.com/sourcegraph/sourcegraph/pull/47933)


Incomplete sentence I think? there's a mention of authz.syncJobsRecordsTTL that seems to have been moved to the removed section

whoops, that's true, I'm gonna fix it now

thanks for catching it!

fixed here
@bobheadxi

sashaostrikov added the permissions-center Tracking issue for all-things permissions center. label Feb 21, 2023

sashaostrikov requested review from a team and bobheadxi February 21, 2023 08:29

sashaostrikov self-assigned this Feb 21, 2023

cla-bot Bot added the cla-signed label Feb 21, 2023

sourcegraph-bot mentioned this pull request Feb 21, 2023

Permissions Center: Tracking issue #46317

Closed

sashaostrikov commented Feb 21, 2023

View reviewed changes

sourcegraph-bot mentioned this pull request Feb 21, 2023

Permission Syncs Dashboard: Tracking issue #46329

Closed

71 tasks

sashaostrikov force-pushed the ao/pc/sync-job-api branch from 2df4b80 to 5c3bdcc Compare February 21, 2023 09:57

0xnmn reviewed Feb 21, 2023

View reviewed changes

0xnmn suggested changes Feb 21, 2023

View reviewed changes

sashaostrikov added 3 commits February 21, 2023 15:48

permissions-center: expose new permissionSyncJobs GraphQL API.

0325f07

This substitutes experimental `permissionsSyncJobs` API and removes storing permission sync stats into Redis. Test plan: Unit tests updated and added. Local sg run and API query tests.

add missing gql docs

c85af32

remove the file added after rebasing by mistake

891a4f2

kopancek reviewed Feb 21, 2023

View reviewed changes

revert weird changes

cc24d48

sashaostrikov force-pushed the ao/pc/sync-job-api branch from 3e3bed5 to cc24d48 Compare February 21, 2023 11:55

sashaostrikov added 2 commits February 21, 2023 16:00

fix TODOs in changelog

d8b06a1

revert tests as well

a0efa5d

sashaostrikov mentioned this pull request Feb 21, 2023

permissions-center: use new permissionSyncJobs query. sourcegraph/src-cli#942

Merged

add test for time-related lib functions

309a464

sashaostrikov requested a review from 0xnmn February 21, 2023 12:49

0xnmn approved these changes Feb 21, 2023

View reviewed changes

sashaostrikov merged commit e8416fc into main Feb 21, 2023

sashaostrikov deleted the ao/pc/sync-job-api branch February 21, 2023 18:09

bobheadxi reviewed Feb 21, 2023

View reviewed changes

Conversation

sashaostrikov commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcegraph-bot commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sg-e2e-regression-test-bob commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnmn Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnmn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sashaostrikov commented Feb 21, 2023 •

edited

Loading

sourcegraph-bot commented Feb 21, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Feb 21, 2023 •

edited

Loading

0xnmn Feb 21, 2023 •

edited

Loading