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.

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

Check user status #209

Merged
merged 2 commits into from
May 31, 2019
Merged

Check user status #209

merged 2 commits into from
May 31, 2019

Conversation

jvillafanez
Copy link
Member

Handle the case where the user associated to the token is disabled. Both the authorization_code and refresh_token will fail with a 400 status and a "unauthorized_client" error instead of returning a valid access token

Related to owncloud/client#7212 and https://github.com/owncloud/enterprise/issues/2509

@CLAassistant
Copy link

CLAassistant commented May 31, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 31, 2019

Codecov Report

Merging #209 into master will increase coverage by 0.34%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #209      +/-   ##
============================================
+ Coverage      65.7%   66.04%   +0.34%     
- Complexity      224      228       +4     
============================================
  Files            34       34              
  Lines           898      907       +9     
============================================
+ Hits            590      599       +9     
  Misses          308      308
Impacted Files Coverage Δ Complexity Δ
lib/Controller/OAuthApiController.php 97.77% <100%> (+0.24%) 23 <0> (+4) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e4057b...bdc0a40. Read the comment docs.

@jvillafanez
Copy link
Member Author

There is some piece still missing. The desktop client opens the url below, which keeps reloading it self over and over.
Screen Shot 2019-05-31 at 14 46 35
I guess the session is still active, and that's why the browser doesn't redirect to the login page until the session is invalidated at some point.

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Looks reasonable! 👍

@DeepDiver1975 DeepDiver1975 merged commit 7a9756e into master May 31, 2019
@delete-merged-branch delete-merged-branch bot deleted the check_user_status branch May 31, 2019 13:20
@jvillafanez
Copy link
Member Author

I'll need confirmation that the problem (#209 (comment)) is caused by core and not by the oAuth2 app. I suspect a middleware trying to login with a token / cookie or similar and core is rejecting the request because the user is disabled. The request doesn't seem to reach the PageController of the oAuth2 app, or at least I don't see how that code could fail like that there.
It's very likely that the session is still valid even with the user being disabled.

@DeepDiver1975
Copy link
Member

If you want to logout the user you have to call \OC::$server->getUserSession()->logout()

@HanaGemela
Copy link
Contributor

The 'User disabled' page still reloads with
Client: 2.6.0 (build 12703)
macOS 10.15
Server 10.3.0 stable
oAuth2 0.4.2RC3

Steps to recreate:

  1. Login with oAuth2 using enabled user
  2. Let the desktop client sync
  3. Quit the client
  4. Disable user
  5. Start the desktop client

It will eventually stop reloading and login page will be shown but till then the page reloads over and over again

@micbar
Copy link
Contributor

micbar commented Nov 19, 2019

@HanaGemela Thank you for testing.

This is a little bit ugly behaviour ;-)

@HanaGemela
Copy link
Contributor

Getting 400 error instead of 200 now. So the original bug has been resolved. But the reloading page still needs fixing

@DeepDiver1975
Copy link
Member

Please open a new issue instead of commenting on a closed PR. This helps to track this better. THX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants