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

[auth] Fix session token refresh #2474

Merged
merged 4 commits into from
Aug 4, 2023
Merged

[auth] Fix session token refresh #2474

merged 4 commits into from
Aug 4, 2023

Conversation

Seltyk
Copy link
Contributor

@Seltyk Seltyk commented Aug 1, 2023

Description
Refreshing session tokens first loads the authenticated client application from a SQL query, then verifies it against the user's browser session, and finally refreshes the token. The verification step uses a simple inequality check between two ClientApplication objects. Because this class does not have a definition for (in)equality (i.e. the __eq__ and __ne__ methods are not implemented), that inequality only checks the objects' pointers/addresses*, which is guaranteed to fail. Thankfully, the ClientApplication class has an id field which matches the primary key in the associated database row. Comparing the underlying hex string values should make correct equality checks. This PR's main change is a simple implementation of __eq__ for ClientApplication as described. Because __ne__ is not implemented, inequality defers to this method and inverts the output, so both == and != are well-defined.

*I presume, based on zero evidence

This request also makes some nitpick changes, listed here:

  • ghostbytes in both modified files is removed, including on lines matching the regex /^\s+$/
  • unused imports and variables are removed (resolving a handful of Ruff lints)
  • a module-level import is moved to the top of the file (resolving another lint)
  • value == False and value == True are replaced (resolving the last lint)
  • docstring typo "dictionairy" is resolved
  • token refresh failures return code 400 instead of 200
  • "coding" replaced with "encoding"

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Seltyk <whhacker.dcx@gmail.com>
- error states should not HTTP return 200, they
  should always be 4xx
- "dictionairy" typo

Signed-off-by: Seltyk <whhacker.dcx@gmail.com>
@Seltyk Seltyk changed the title Fix session token refresh [auth] Fix session token refresh Aug 1, 2023
@Ulincsys
Copy link
Contributor

Ulincsys commented Aug 2, 2023

The equality method of ClientApplication should check that the other object is of the same class type as self before performing the ID comparison, and return False if it is not. Currently, if one were to try comparing it against an object without the id attribute, it would throw an exception.

Actually, I misunderstood the changes made here. ClientApplication is a subclass of the SQLAlchemy declarative_base class, which provides the functionality observed, the comparison should be correct. Also, the linter's output regarding class.attribute == False is not accurate for the same reason. The conditional in this instance returns a callable for use in filter(), it does not resolve to a boolean.

Reference for this behavior can be found here: https://docs.sqlalchemy.org/en/13/orm/query.html#sqlalchemy.orm.query.Query.filter

@Seltyk
Copy link
Contributor Author

Seltyk commented Aug 2, 2023

ClientApplication is a subclass of the SQLAlchemy declarative_base class, which provides the functionality observed

At a glance, the declarative_base call chain never defines equality for its objects, and the docs don't mention equality or the __eq__ method. That said, if it does define this equality, I'm getting {"status": "Invalid application"} when in theory I shouldn't. This PR was intended to address that issue, and in my testing, it does.

The conditional in this instance returns a callable for use in filter(), it does not resolve to a boolean.

There were two == Bool instances. One is checkPassword, which is the result of a function that returns a bool, so this one should be no issue. The other, which I think is what you mentioned, is UserGroup.favorited. I had to stare at it for a few minutes here, but I see what you're getting at now. I'll file a bug report with Ruff and revert that change.

EDIT: there is already a bug report

Seltyk added a commit to Seltyk/augur that referenced this pull request Aug 2, 2023
See chaoss#2474 (comment)

Signed-off-by: Seltyk <whhacker.dcx@gmail.com>
"It won't be inexcept... unexcept... it won't
throw an exception" --Ulincsys

Signed-off-by: Seltyk <whhacker.dcx@gmail.com>
@Ulincsys Ulincsys merged commit f8f546b into chaoss:dev Aug 4, 2023
1 check passed
@Seltyk Seltyk deleted the refresh branch August 7, 2023 15:48
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