-
Notifications
You must be signed in to change notification settings - Fork 16.6k
fix(security): Add active property to guest user #35454
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset/security/guest_token.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #fe6ce1
Actionable Suggestions - 1
-
superset/security/guest_token.py - 1
- Flask-Login interface violation · Line 61-62
Review Details
-
Files reviewed - 1 · Commit Range:
bb30f55..bb30f55- superset/security/guest_token.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
| is_guest_user = True | ||
| active = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added active = True class attribute does not implement the required Flask-Login is_active property. Flask-Login expects user classes to provide an is_active property (not a class attribute) that returns a boolean indicating whether the user account is active. This mismatch will cause authentication failures when Flask-Login checks current_user.is_active for guest users. Replace the class attribute with a proper @property method is_active that returns True, ensuring compatibility with Flask-Login's user interface requirements.
Code suggestion
Check the AI-generated fix before applying
| is_guest_user = True | |
| active = True | |
| is_guest_user = True | |
| @property | |
| def is_active(self) -> bool: | |
| return True |
Code Review Run #fe6ce1
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| """ | ||
|
|
||
| is_guest_user = True | ||
| active = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would help if we can provide a bit more context as of why this issue came into existence and how this fixes it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tagged @dpgaspar as a reviewer because i think a recent FAB update caused this.
|
Following this PR, I use this functionality and have seen the same issues on 6.0.0rc2. Would love to see this merged in! |
| """ | ||
|
|
||
| is_guest_user = True | ||
| active = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be active or is_active?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with active = True and confirmed it fixes the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the error is:
if current_user.is_authenticated and current_user.active:
so it should be like this, but can you add the is_active also like:
@property
def is_active(self):
return self.activeLooking at it, it does seem that FAB should be using is_active instead of active, I'll make the change.
| """ | ||
|
|
||
| is_guest_user = True | ||
| active = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the error is:
if current_user.is_authenticated and current_user.active:
so it should be like this, but can you add the is_active also like:
@property
def is_active(self):
return self.activeLooking at it, it does seem that FAB should be using is_active instead of active, I'll make the change.
ebbdafb to
bb30f55
Compare
(cherry picked from commit 98fba1e)
SUMMARY
Fixes the error by adding a property for active on guest user.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Embed a dashboard and see that there are no errors.
ADDITIONAL INFORMATION