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

Fix OAuth2 Event Trigger #28345

Merged
merged 2 commits into from
Jul 14, 2017
Merged

Fix OAuth2 Event Trigger #28345

merged 2 commits into from
Jul 14, 2017

Conversation

Hemant-Mann
Copy link
Contributor

Description

This PR aims to fix a minor bug generated by the changes of #28179

Related Issue

#28179

Steps to Reproduce the Bug:

  • Checkout the master branch
  • Set a break point in oauth2.js on line 31
  • Append /owncloud/index.php/settings/admin?sectionid=storage&code=a i.e. code={something} in the URL and reload the page
  • Select Google Drive from the storage you will see the browser JS execution stops at line 31 because the event was triggered since the condition params['code'] !== undefined was met

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Hemant-Mann
Fix Event Trigger
3d09b3c 
Event was triggered on storage selection if code={something} was present in URL
@Hemant-Mann Hemant-Mann changed the title Fix Event Trigger Fix OAuth2 Event Trigger Jul 8, 2017
PVince81
PVince81 reviewed Jul 10, 2017
View reviewed changes
apps/files_external/js/oauth2.js Outdated
@@ -27,7 +27,7 @@ $(document).ready(function() {
params[key] = value;
});

if (params['code'] !== undefined) {
if (params['code'] !== undefined && client_id != '' && client_secret != '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please always use strict equality: === or !== in Javascript to avoid surprises.

I recommend to use JSLint or JSHint which would catch these

Copy link
Contributor Author

@Hemant-Mann Hemant-Mann Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I generally go with strict checking but in this case I went with loose checking because I just wanted to check whether the variable client_id and client_secret were having some value

like if (client_id && client_secret) { // do something }

Copy link
Contributor

Choose a reason for hiding this comment

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

please fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay will do this right now

@Hemant-Mann
Copy link
Contributor Author

@PVince81 I have added strict checking

PVince81
PVince81 approved these changes Jul 13, 2017
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 if you retested with the changes and it still works

@Hemant-Mann
Copy link
Contributor Author

Yes I retested with the changes.
The bug is fixed and oauth2 authentication steps works as intended :)

@PVince81 PVince81 merged commit 5a83c5a into owncloud:master Jul 14, 2017
@PVince81
Copy link
Contributor

@Hemant-Mann awesome, thanks!

@PVince81
Copy link
Contributor

@Hemant-Mann please backport to stable10

@PVince81 PVince81 added this to the development milestone Jul 14, 2017
@Hemant-Mann Hemant-Mann deleted the oauth2-event-trigger branch July 14, 2017 15:52
@Hemant-Mann Hemant-Mann mentioned this pull request Jul 14, 2017
Merged
@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PVince81 PVince81 PVince81 approved these changes

Assignees
No one assigned
Labels
3 - To Review status/STALE
Projects
None yet
Milestone
10.0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@Hemant-Mann @PVince81 @ownclouders