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

fix: Check if Oauth login with OKTA is correct #1926

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

safeith
Copy link
Contributor

@safeith safeith commented Sep 15, 2022

Description

We should check the errors for OKTA Oauth; if we do not check it, it will continue and will create a user with the default information that we create in the return clause.
image

Why does this happen?
Sometimes users are in more than 100 groups and Okta does not return all the groups and it return the following error message

User info from Okta: {'error': 'server_error', 'error_description': 'The groups claim matched too many groups and must be configured to match fewer groups.'}

It returns an error but, we do not check it and we let it to continue with the following return clause

return {
    "username": "okta_",
    "first_name": "",
    "last_name": "",
    "email": "",
    "role_keys": [],
}

And with data with register a new user(s) here

Finally we have imagery user that many users can login with it

ADDITIONAL INFORMATION

  • Has associated issue:
  • Is CRUD MVC related.
  • Is Auth, RBAC security related.
  • Changes the security db schema.
  • Introduces new feature
  • Removes existing feature

@safeith safeith changed the title Check if oauth login with okta is correct Fix OKTA Oauth Sep 15, 2022
@safeith safeith changed the title Fix OKTA Oauth fix: Check if Oauth login with OKTA is correct Sep 15, 2022
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 43.59%. Comparing base (6f00efc) to head (adea7d8).
Report is 20 commits behind head on master.

Files Patch % Lines
flask_appbuilder/security/manager.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1926       +/-   ##
===========================================
- Coverage   79.71%   43.59%   -36.12%     
===========================================
  Files          72       72               
  Lines        8990     8754      -236     
===========================================
- Hits         7166     3816     -3350     
- Misses       1824     4938     +3114     
Flag Coverage Δ
python 43.59% <0.00%> (-36.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here, could you please rebase and add a couple of tests. Thank you!

@safeith
Copy link
Contributor Author

safeith commented Mar 11, 2024

Hi @dpgaspar
No worries, I rebased it.

@dpgaspar dpgaspar merged commit c65e067 into dpgaspar:master Mar 12, 2024
13 of 15 checks passed
@safeith safeith deleted the fix-okta-login branch March 14, 2024 20:01
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