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#40534 only allow user to connect via OAuth #35

Merged
merged 2 commits into from
Feb 21, 2022
Merged

OP#40534 only allow user to connect via OAuth #35

merged 2 commits into from
Feb 21, 2022

Conversation

individual-it
Copy link
Collaborator

@individual-it individual-it commented Feb 3, 2022 •
edited by lindenthal
Loading

the user should not have any option to connect via api tokens to OpenProject. This PR is part of that

  • the oAuth URL is constructed in PHP and handed over to vue
  • oAuth button is a separate Vue component and used in files-details-tab, dashboard, personal settings
  • in personal settings page the user can only connect and disconnect from OpenProject (via oAuth)

in following PR more work needs to be done:

  • delete back-end code that deals with api tokens
  • show correct status of the connection in the file-details-tab (currently connect button is displayed even the connection is established)

https://community.openproject.org/projects/817/work_packages/40534/activity

@individual-it individual-it self-assigned this Feb 3, 2022
@individual-it individual-it mentioned this pull request Feb 3, 2022
Closed
@individual-it individual-it changed the title only allow user to connect via OAuth OP#40534 only allow user to connect via OAuth Feb 3, 2022
@individual-it
Copy link
Collaborator Author

I've not adjusted also the documentation

julien-nc
julien-nc requested changes Feb 3, 2022
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Very nice.
A few minor inline comments.

lib/Dashboard/OpenProjectWidget.php Outdated Show resolved Hide resolved
lib/Listener/LoadSidebarScript.php Outdated Show resolved Hide resolved
lib/Settings/Personal.php Outdated Show resolved Hide resolved
lib/Settings/Personal.php Outdated Show resolved Hide resolved
lib/Service/OpenProjectAPIService.php
* please note that the state parameter is still missing, that needs to be generated dynamically
* and saved to the DB before calling the OAuth URI
*/
public static function getOpenProjectOauthURL(IConfig $config, IURLGenerator $urlGenerator): string {
Copy link
Member

Choose a reason for hiding this comment

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

IURLGenerator could be injected in the constructor of OpenProjectAPIService instead of passing it as a parameter to getOpenProjectOauthURL. This would avoid having to inject IURLGenerator in every class calling this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, but then every class that calls the method would have to initiate the complete OpenProjectAPIService and by that need to get injected even more
e..g from OpenProjectWidget we would need the logger, usermanager, avatarmanager etc.
Or maybe we want to place that function somewhere else? A helper class?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree. As long as getOpenProjectOauthURL is a static method, let's keep it that way.
If you feel like it belongs in a new class/file, feel free to move it 😁.

@individual-it
Copy link
Collaborator Author

@eneiluj could you re-review and give your opinion on my comment here #35 (comment) ?

@individual-it individual-it force-pushed the oauthConnectFromDashboardAdnFiles branch 2 times, most recently from 2796780 to ac920d8 Compare February 16, 2022 04:10
@individual-it individual-it mentioned this pull request Feb 16, 2022
Merged
@individual-it individual-it force-pushed the oauthConnectFromDashboardAdnFiles branch from ac920d8 to aff7051 Compare February 18, 2022 06:59
julien-nc
julien-nc previously requested changes Feb 18, 2022
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

👍
One last small change and we're good to go.

lib/Service/OpenProjectAPIService.php Outdated
return $oauthUrl .
'/oauth/authorize' .
'?client_id=' . $clientID .
'&redirect_uri=' . $redirectUri .
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'&redirect_uri=' . $redirectUri .
'&redirect_uri=' . urlencode($redirectUri) .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, I've adjusted that and also squashed all commits

lib/Service/OpenProjectAPIService.php
* please note that the state parameter is still missing, that needs to be generated dynamically
* and saved to the DB before calling the OAuth URI
*/
public static function getOpenProjectOauthURL(IConfig $config, IURLGenerator $urlGenerator): string {
Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree. As long as getOpenProjectOauthURL is a static method, let's keep it that way.
If you feel like it belongs in a new class/file, feel free to move it 😁.

@individual-it
directly connect by OAuth from dashboard and file details
4063751 
and only allow oAuth connection for user

Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it individual-it force-pushed the oauthConnectFromDashboardAdnFiles branch from aff7051 to 4063751 Compare February 18, 2022 11:41
@individual-it
don't use the user setting as URL to openproject
95bc406 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it
Copy link
Collaborator Author

I found an other issue that I've missed before. The url to openproject was still taken from the user settings

@github-actions
Copy link

JS Code Coverage

Coverage after merging oauthConnectFromDashboardAdnFiles into master will be
31.15%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/components
   AdminSettings.vue0%100%0%0%101, 48–54, 66, 82–84, 88, 95–96, 98
   OAuthConnectButton.vue100%100%100%100%
   PersonalSettings.vue33.33%26.32%27.27%38.89%102–103, 103, 103, 106–107, 107, 107, 110–111, 111, 111, 114, 117–118, 120–121, 121–123, 123, 123, 123–124, 129–130, 130, 130, 135, 141, 89–92, 98–99
src/components/settings
   SettingsTitle.vue0%100%0%0%14
src/components/tab
   EmptyContent.vue100%100%100%100%
   SearchInput.vue30.61%25%45.45%30.16%100, 100, 100, 100–102, 110–112, 115–117, 120–122, 122, 122–124, 124, 124–125, 128, 131, 138, 138, 138–140, 140, 140–141, 143, 148, 148, 148, 153–162, 162–163, 178–182, 182–184, 186–187, 190, 192, 93, 99
src/views
   Dashboard.vue0%0%0%0%101, 106, 106, 106–107, 109, 115, 119–120, 128, 131, 135–137, 139, 142–143, 146–147, 147–148, 152–153, 153–154, 156, 158–159, 159, 159, 159, 159–161, 161, 161, 161, 161–163, 166, 171, 171, 171, 173–174, 174, 174–175, 177, 177–179, 183, 187, 190, 190, 190, 193, 193, 193, 198, 201, 201, 201, 208, 211, 211, 211, 216, 216, 216, 22, 221, 221, 221, 226, 229, 23, 232, 235, 235, 235, 238, 238, 238, 24, 241, 244, 247, 25–30, 47, 54, 54, 61, 64–65, 77–78, 78, 78, 81, 84, 84, 84–86, 86, 86–88, 88–89, 91, 94, 94, 94–96, 96, 96–98, 98–99
   ProjectsTab.vue90.91%100%83.33%90.48%74–75

@github-actions
Copy link

PHP Code Coverage

Coverage after merging oauthConnectFromDashboardAdnFiles into master will be
44.69%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
lib/AppInfo
   Application.php0%100%0%0%101, 105, 49, 51–52, 54–56, 59–60, 63–65, 67, 70–71, 74–77, 79–84, 86, 88, 91, 95, 99
lib/BackgroundJob
   CheckNotifications.php0%100%0%0%48, 50, 52–53, 61–62
lib/Controller
   ConfigController.php60.92%100%60%60.98%106–107, 109, 187–192, 194–197, 199, 201, 71–72, 74, 76–78, 80–87, 92–93, 95
   OpenProjectAPIController.php96.49%100%85.71%98%82
lib/Dashboard
   OpenProjectWidget.php0%100%0%0%106–109, 61–64, 71, 78, 85, 92, 99
lib/Listener
   LoadSidebarScript.php0%100%0%0%59–61, 65–66, 68, 70, 72–74, 76–78
lib/Notification
   Notifier.php0%100%0%0%100, 110, 112, 114, 47–50, 60, 69, 80, 82, 85, 87–92
lib/Search
   OpenProjectSearchProvider.php0%100%0%0%102, 109–110, 113–116, 118–119, 123, 127–132, 134–136, 139–140, 142–143, 147–148, 158, 170, 178, 181, 184, 193, 196, 207, 71–75, 82, 89, 97, 99
   OpenProjectSearchResultEntry.php100%100%100%100%
lib/Service
   OpenProjectAPIService.php60%100%66.67%59.53%105–115, 117–121, 123–128, 130–131, 134–137, 144, 151–155, 165–166, 168–172, 174–175, 183–187, 318, 361, 376–377, 379, 385, 391–396, 398, 404, 432–435, 438–439, 441, 443–444, 466–467, 474, 477–480, 482, 488, 492–494, 95–98
lib/Settings
   Admin.php0%100%0%0%25–26, 33–35, 37, 42–43, 47, 51
   AdminSection.php0%100%0%0%19–20, 29, 39, 48, 55
   Personal.php0%100%0%0%38–41, 48–52, 55, 57, 65–66, 70, 74
   PersonalSection.php0%100%0%0%19–20, 29, 39, 48, 55

@individual-it individual-it merged commit 3e9d5ac into master Feb 21, 2022
@delete-merged-branch delete-merged-branch bot deleted the oauthConnectFromDashboardAdnFiles branch February 21, 2022 10:47
@individual-it individual-it mentioned this pull request Feb 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiranparajuli589 kiranparajuli589 kiranparajuli589 approved these changes

@julien-nc julien-nc Awaiting requested review from julien-nc

Assignees

@individual-it individual-it

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@individual-it @julien-nc @kiranparajuli589