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

Jump to bottom

[OP#49416] Encrypt the OAuth2 client secrets #445

Merged
merged 4 commits into from
Aug 1, 2023
Merged

[OP#49416] Encrypt the OAuth2 client secrets #445

merged 4 commits into from
Aug 1, 2023

Conversation

SwikritiT
Copy link
Contributor

@SwikritiT SwikritiT commented Jul 27, 2023 •
edited
Loading

Description

The server implemented the feature to encrypt the oauth client secret in nextcloud/server#38398
but these changes were not adapted to our app so the OAuth connection wasn't successful from the open project side.

As this feature is only available from the nextcloud

  • >= 25.0.8
  • >= 26.0.4
  • >= 27.0.1

So the necessary check to ensure the version of the server is made.

Also the client secret is displayed only once now, right after it's created.

Related workpackage

OP#49416 - https://community.openproject.org/projects/nextcloud-integration/work_packages/49416

@SwikritiT SwikritiT changed the title Encrypt the OAuth2 client secrets [OP#49416] Encrypt the OAuth2 client secrets Jul 27, 2023
@SwikritiT SwikritiT force-pushed the encrypt-the-client-secrets branch from ef49e92 to a71c3c0 Compare July 28, 2023 10:56
@SwikritiT SwikritiT marked this pull request as ready for review July 28, 2023 11:43
@SwikritiT SwikritiT requested a review from SagarGi July 28, 2023 11:43
SagarGi
SagarGi approved these changes Jul 28, 2023
View reviewed changes
Copy link
Collaborator

@SagarGi SagarGi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@SwikritiT SwikritiT force-pushed the encrypt-the-client-secrets branch from 13e28a7 to 05a8751 Compare July 31, 2023 03:40
SwikritiT
SwikritiT commented Jul 31, 2023
View reviewed changes
.github/workflows/ci.yml
@@ -161,7 +161,7 @@ jobs:
if: ${{ github.event_name == 'pull_request' && matrix.nextcloudVersion == 'master' && matrix.phpVersion == '8.1' }}
uses: VeryGoodOpenSource/very_good_coverage@v2
with:
min_coverage: '57'
min_coverage: '56'
Copy link
Contributor Author

@SwikritiT SwikritiT Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm lowering the coverage because the code changes in this PR is related to lib/Service/OauthService.php and we don't have any unit tests for this particular file. Here's a related workpackage https://community.openproject.org/projects/nextcloud-integration/work_packages/42896

I don't want the low coverage to block this PR. We can incerase the coverage once again after adding some tests on some other PR

SwikritiT added 3 commits July 31, 2023 11:15
@SwikritiT
Encrypt the OAuth2 client secrets
d14af27 
check versions of nc

add migration step

update version and extensions

try running qb

disable migration

enable migration

enable migration

enable migration

enable migration

enable migration

remove migration

add more version checks

add more version checks

add migration

add migration

add extension

don't show secrets

update unit tests

fix api test

fix api test

Signed-off-by: Swikriti Tripathi <swikriti808@gmail.com>
@SwikritiT
revert ci changes
6a9f3c8 
Signed-off-by: Swikriti Tripathi <swikriti808@gmail.com>
@SwikritiT
lower the coverage
579bca3 
Signed-off-by: Swikriti Tripathi <swikriti808@gmail.com>
@SwikritiT SwikritiT force-pushed the encrypt-the-client-secrets branch from 05a8751 to 579bca3 Compare July 31, 2023 05:30
@SwikritiT
fix test expectations
0dc29fb 
Signed-off-by: Swikriti Tripathi <swikriti808@gmail.com>
@SwikritiT SwikritiT force-pushed the encrypt-the-client-secrets branch from bebe7ec to 0dc29fb Compare July 31, 2023 05:59
@SwikritiT SwikritiT requested a review from SagarGi July 31, 2023 05:59
@github-actions
Copy link

JS Code Coverage

Coverage after merging encrypt-the-client-secrets into master will be
90.30%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   adminSettings.js0%0%0%0%1, 1, 10–19, 2, 20–25, 3–9
   bootstrap.js0%0%0%0%1, 1–7
   dashboard.js0%0%0%0%1, 1, 10–19, 2, 20–25, 3–9
   fileActions.js0%0%0%0%1, 1, 10–17, 2–9
   personalSettings.js0%0%0%0%1, 1, 10–19, 2, 20–25, 3–9
   projectTab.js0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–66, 7–9
   reference.js0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–43, 5–9
   utils.js63.64%33.33%50%66%10–14, 17–26, 6–9
src/components
   AdminSettings.vue99.28%95.29%84.62%99.91%1, 1, 1, 1, 1
   OAuthConnectButton.vue99.17%87.50%100%100%1
   PersonalSettings.vue98.87%93.33%85.71%100%1
src/components/admin
   FieldValue.vue97.12%83.33%100%98.89%1, 23, 23
   FormHeading.vue97.58%75%100%99.34%1, 1, 29, 29
   ProjectFolderError.vue96.83%80%100%98.21%1, 1
   TextInput.vue99.25%95%88.89%100%1
src/components/icons
   ClippyIcon.vue93.18%50%50%97.50%1, 1
src/components/settings
   CheckBox.vue92.45%80%66.67%97.62%1, 1
   SettingsTitle.vue94.74%50%100%97.14%1, 1
src/components/tab
   EmptyContent.vue97.47%81.25%100%99.29%1, 1, 31, 31
   SearchInput.vue99.57%88.89%100%100%1
   WorkPackage.vue98.36%40%100%99.33%1, 1, 1, 43, 43
src/utils
   workpackageHelper.js94.21%88%100%95.65%17–19, 48, 48–50
src/views
   Dashboard.vue98.97%71.43%100%99.65%1, 1, 1
   ProjectsTab.vue99.74%93.75%100%100%23
   WorkPackageReferenceWidget.vue0%0%0%0%1, 1, 10, 100–109, 11, 110–119, 12, 120–129, 13, 130–132, 14–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–99

@github-actions
Copy link

PHP Code Coverage

Coverage after merging encrypt-the-client-secrets into master will be
56.80%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
server/apps/integration_openproject/lib/AppInfo
   Application.php11.43%100%25%9.68%100–102, 106–109, 111, 115–120, 131, 135, 68–69, 72, 76, 79, 83, 85, 89, 94, 96–98
server/apps/integration_openproject/lib/BackgroundJob
   RemoveExpiredDirectUploadTokens.php0%100%0%0%42, 44–46, 55–56
server/apps/integration_openproject/lib/Controller
   ConfigController.php66.40%100%50%67.36%134, 151–152, 154, 156–158, 160–161, 166–167, 169, 219, 280, 369–370, 373–374, 504–507, 509–510, 513, 521, 532, 546–548, 563–567, 569–570, 572–575, 577–579, 597–602, 604–605, 607–609, 612, 614–617, 619–621, 635, 644–647, 649–652, 662–667
   DirectDownloadController.php0%100%0%0%36–38, 53–55, 57, 60–61
   DirectUploadController.php71.43%100%100%70.43%131–132, 177, 190, 194–197, 199, 209, 216, 232–234, 236–237, 240–242, 248, 250, 255–256, 263–264, 267–268, 271–272, 288–289, 309, 314, 320
   FilesController.php78.86%100%100%77.59%168, 220–221, 272, 278–282, 285–287, 289, 291, 302–304, 307–308, 310–311, 315–318, 321
   OpenProjectAPIController.php82.27%100%73.33%82.93%138, 174, 191–194, 198, 202, 204–209, 211, 220–223, 226, 228, 230–233, 235–236, 241, 253, 262, 280, 289, 554, 556, 95
server/apps/integration_openproject/lib/Dashboard
   OpenProjectWidget.php0%100%0%0%101–102, 104–108, 116, 123–124, 126, 128–129, 131–132, 134, 137–138, 140–141, 143, 69–73, 80, 87, 94
server/apps/integration_openproject/lib/Exception
   OpenprojectErrorException.php100%100%100%100%
   OpenprojectFileNotUploadedException.php100%100%100%100%
   OpenprojectGroupfolderSetupConflictException.php100%100%100%100%
   OpenprojectResponseException.php100%100%100%100%
   OpenprojectUnauthorizedUserException.php0%100%0%0%16
server/apps/integration_openproject/lib/Listener
   BeforeGroupDeletedListener.php0%100%0%0%48, 56–57, 60–63
   BeforeNodeInsideOpenProjectGroupfilderChangedListener.php0%100%0%0%40–42, 46–51, 53–58, 60, 62, 66
   BeforeUserDeletedListener.php0%100%0%0%48, 55–56, 58–61
   LoadSidebarScript.php0%100%0%0%100, 102, 104, 106–108, 110, 112, 114–115, 117–118, 120, 122, 75–81, 83–84, 86–87, 89–90, 96–97, 99
   OpenProjectReferenceListener.php0%100%0%0%37–38, 41
   UserChangedListener.php0%100%0%0%52, 59–60, 63–68
server/apps/integration_openproject/lib/Migration
   Version2001Date20221213083550.php0%100%0%0%47, 57, 60, 63, 67, 70, 73, 77–79, 81
   Version2310Date20230116153411.php0%100%0%0%46, 49–52, 54–56, 60, 64, 68, 72, 76, 81–82, 84
   Version2400Date20230504144300.php0%100%0%0%47, 57, 60
server/apps/integration_openproject/lib/Reference
   WorkPackageReferenceProvider.php0%100%0%0%102, 109–112, 115–117, 120, 127–130, 132, 134–136, 138, 140, 142, 146, 155, 163–164, 172, 53, 60, 67, 74–75, 83, 96–99
server/apps/integration_openproject/lib/Search
   OpenProjectSearchProvider.php0%100%0%0%103–104, 107–112, 114–115, 118–119, 121–123, 125–127, 131–132, 134–135, 139–144, 150–151, 153, 66–69, 76, 83, 91, 93, 96
   OpenProjectSearchResultEntry.php100%100%100%100%
server/apps/integration_openproject/lib/Service
   DatabaseService.php43.90%100%60%41.67%125–128, 131, 80–87, 89–93, 95–97
   DirectDownloadService.php88%100%100%86.96%65–66, 68
   DirectUploadService.php54.55%100%66.67%52.63%112, 118, 79–82, 84, 89, 91
   OauthService.php0%100%0%0%108–115, 45–47, 56–66, 68, 70–72, 75–78, 89, 91–94, 96–97
   OpenProjectAPIService.php69.20%100%65.85%69.53%1000, 1006, 1011, 1015, 1025, 1027, 1030, 1032, 1131–1133, 1143, 1153–1159, 1167–1171, 1179–1186, 1203–1210, 1219–1224, 180–184, 255, 364–365, 367, 381–382, 391, 395, 416, 504–505, 512, 515–518, 520, 526, 530–532, 552,

@SwikritiT SwikritiT merged commit 46e654b into master Aug 1, 2023
@delete-merged-branch delete-merged-branch bot deleted the encrypt-the-client-secrets branch August 1, 2023 04:05
@SagarGi SagarGi mentioned this pull request May 10, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SagarGi SagarGi SagarGi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SwikritiT @SagarGi