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

Redirect user to login page when oauth session expired #84

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

Charl1996
Copy link
Contributor

Ticket

We've been having an issue on CCA where users which are already logged in to CCA could not access the site due to a 500 error. The error can be seen below.

File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/hq_superset/utils.py", line 220, in _get_domain_access
response = hq_request.get()
File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/hq_superset/hq_requests.py", line 28, in get
return self.commcare_provider.get(self.url, token=self.oauth_token)
File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/hq_superset/hq_requests.py", line 13, in oauth_token
return get_valid_cchq_oauth_token()
File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/hq_superset/oauth.py", line 91, in get_valid_cchq_oauth_token
refresh_response = refresh_and_fetch_token(refresh_token)
File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/hq_superset/oauth.py", line 99, in refresh_and_fetch_token
refresh_response = provider._get_oauth_client().refresh_token(
File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/authlib/oauth2/client.py", line 256, in refresh_token
return self._refresh_token(
File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/authlib/oauth2/client.py", line 377, in _refresh_token
token = self.parse_response_token(resp)
File "/home/ubuntu/www/.virtualenvs/superset/lib/python3.10/site-packages/authlib/oauth2/client.py", line 344, in parse_response_token
raise self.oauth_error_class(
authlib.integrations.base_client.errors.OAuthError: invalid_grant:

This was likely due to the refresh token having expired.

This PR simply adds a way of catching the OAuthSessionExpired if it's raised in any of the views and does the following:

  1. Log out the current user
  2. Redirect that user to the login page asking the user to log in back again.

A simple test has been added to test this behaviour.


appbuilder.add_view(views.HQDatasourceView, 'Update HQ Datasource', menu_cond=lambda *_: False)
appbuilder.add_view(views.SelectDomainView, 'Select a Domain', menu_cond=lambda *_: False)
appbuilder.add_api(api.OAuth)
appbuilder.add_api(api.DataSetChangeAPI)
oauth2_server.config_oauth2(app)

app.register_error_handler(OAuthSessionExpired, hq_domain.oauth_session_expired)
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this cater to "OAuthError"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I added the OAuthError to be caught but didn't push it. See bae3f7a. Thanks for reminder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Let me know if you want to test this on staging, I can still replicate it predictably

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkangia Yeah, that's maybe a good idea.

See #85

Copy link
Contributor

Choose a reason for hiding this comment

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

Approved, let me know when deployed to staging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkangia It's deployed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, I can see the message to login again now.

Screenshot from 2024-12-04 17-09-22

@Charl1996 Charl1996 mentioned this pull request Dec 4, 2024
@Charl1996 Charl1996 merged commit 0f09da7 into master Dec 4, 2024
3 checks passed
@Charl1996 Charl1996 deleted the redirect-to-login-for-expired-refresh-tokens branch December 4, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants