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

[ BUG ] Breaking changes introduced in (at least) falconpy.oauth2.OAuth2 class #1043

Closed
morcef opened this issue Sep 5, 2023 · 10 comments · Fixed by #1046
Closed

[ BUG ] Breaking changes introduced in (at least) falconpy.oauth2.OAuth2 class #1043

morcef opened this issue Sep 5, 2023 · 10 comments · Fixed by #1046
Assignees
Labels
authentication Issues or questions regarding authentication bug 🐛 Something isn't working potential breaking change ‼️ This update contains a potentially breaking change SDK usage General SDK usage issues and questions

Comments

@morcef
Copy link

morcef commented Sep 5, 2023

Describe the bug
After upgrading to falconpy 1.3.1, I noticed a breaking change in the OAuth2 class. Previously both OAuth2.token_expired and OAuth2.authenticated were lambda functions (called as class methods), while they now are set as properties.

To Reproduce
Install falconpy < 1.3.0 and this code runs fine:

from falconpy import OAuth2

auth_object = OAuth2(
    client_id=CLIENT_ID,
    client_secret=CLIENT_SECRET,
)
auth_object.token()
auth_object.authenticated()

Then install falconpy >= 1.3.0, and this will return:

TypeError: 'bool' object is not callable

Expected behavior
Since this is not a major version update, this breaking change should not have been introduced.

Additional context
I have not look back into the history to verify if any other lambda functions were used previously elsewhere.

@morcef morcef added the bug 🐛 Something isn't working label Sep 5, 2023
@jshcodes
Copy link
Member

jshcodes commented Sep 5, 2023

This is a fantastic catch that should have been identified in acceptance testing!

The good news is, I'm pretty sure this will only effect the OAuth2 service class when calling the legacy handler, and could be an "easy" fix. (Hopefully, there is a naming conflict and an architectural issue we need to address.) The authenticated method is now deprecated in lieu of the authenticated property. (This is handled within the ServiceClass interface class.)

Since OAuth2 uses FalconInterface as a base class instead of ServiceClass, it's not inheriting this method. We might be able to address this by adding a deprecated handler for this method to the OAuth2 class (depending, due to the structure, this might not work).

I'm testing this change now, and will update here. Thank you for reporting the issue!!

@jshcodes jshcodes added authentication Issues or questions regarding authentication SDK usage General SDK usage issues and questions potential breaking change ‼️ This update contains a potentially breaking change labels Sep 5, 2023
@jshcodes jshcodes self-assigned this Sep 5, 2023
@jshcodes
Copy link
Member

jshcodes commented Sep 7, 2023

Hi @morcef!

Unfortunately, due to architectural constraints, resolving this was not as easy as I was hoping. We do have a resolution undergoing testing now, but there are a few changes that I need to detail.

A little background

In versions prior to 1.3.0, authenticated was defined as an attribute within the Uber Class. In regular Service Classes, this was defined as a callable method. In the special Service Class, OAuth2 (i.e. the legacy Authentication Object), this was defined as an attribute (which was a callable lambda).

One of the goals of the 1.3.0 refactor was to clean up a lot of this technical debt, align the terms used for specific functionality, and leverage shared objects where possible (and where it made sense). Due to all the naming collisions, authenticated was pretty high up on the list of targets to get refactored. It appears we successfully addressed every variation of potential conflict with this attribute's changes ... except for this scenario. (Insert booing crowd noises here.) This is an older usage pattern, and should not affect developers making use of regular Service Classes while leveraging Direct, Credential, Environment or Object Authentication.

Resolution

To address this breaking change and maintain backwards compatibility, we are reverting a few of the changes implemented by 1.3.0. We are also expanding the Uber Class into a submodule, which allows us to provide multiple versions of the APIHarness class.

  • The authenticated property has been renamed to be token_valid within the FalconInterface object.
  • The token_expired property has been renamed to be token_stale within the FalconInterface object.
  • The authenticated and token_expired abstract properties have been removed from the BaseFalconInterface class.
  • The authenticated and token_expired methods have been added back to regular Service Class and the special Service Class OAuth2.
  • The Uber Class has been expanded to a submodule that provides two classes:
    • APIHarness - The original Uber Class, modified to handle new constructor keywords and class properties. This modified version of the legacy Uber Class does support Debug Logging. Pythonic responses are also implemented but this functionality should be considered extremely experimental when using the legacy version. This version of the Uber Class does not support Environment Authentication. This class does not inherit from FalconInterface, so it leverages old attributes and methods.
    • APIHarnessV2 - The new and improved Uber Class! Providing all functional benefits implemented within version 1.3.0. This new version does inherit from the FalconInterface class, and leverages all of the new properties and derivative functionality as before.

What happens next

These changes are undergoing several rounds of unit testing, and another full round of acceptance testing where we confirm updates do not impact existing integrations or code samples. There is currently an open Pull Request (feel free to review), that will not merge to main until this testing has completed.

A pre-release package has been published to PyPI (1.3.2.dev1) if you are interested in testing these changes locally.

Once merged, this issue will automatically close. These changes will release to the production 1.3.2 package shortly thereafter (we will have a shortened soak period for this specific release). Finally, all documentation will be updated to reflect these changes.

We appreciate you identifying and reporting this issue for us. If you happen to be coming to Fal.Con this year, come see us on the expo hall floor! This find has definitely earned you some swag...

@morcef
Copy link
Author

morcef commented Sep 8, 2023

Hi @jshcodes !
Thank you for investigating this!
I had a quick look into the PR, which looks good 👍
Unfortunately I'm not able to come back to Fal.Con this year, but hopefully sometime again soon!

@davidt99
Copy link
Contributor

davidt99 commented Sep 11, 2023

Not sure if this was fixed in 1.3.2, but when I pass OAuth2 object as auth_object to any Uber Class Service Class, I'm receiving the following error:

main.py:8:
    falconpy.Detects(auth_object=self._auth_object)
/usr/local/lib/python3.11/site-packages/falconpy/_service_class/_service_class.py:166: in __init__
    if not self.token_status:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <falconpy.detects.Detects object at 0x7f8e8280a390>

    @property
    def token_status(self) -> int:
        """Provide the current token_status."""
>       return self.auth_object.token_status
E       AttributeError: 'Detects' object has no attribute 'auth_object'

/usr/local/lib/python3.11/site-packages/falconpy/_service_class/_base_service_class.py:193: AttributeError

@jshcodes
Copy link
Member

Not sure if this was fixed in 1.3.2, but when I pass OAuth2 object as auth_object to any Uber Class, I'm receiving the following error:

main.py:8:
    falconpy.Detects(auth_object=self._auth_object)
/usr/local/lib/python3.11/site-packages/falconpy/_service_class/_service_class.py:166: in __init__
    if not self.token_status:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <falconpy.detects.Detects object at 0x7f8e8280a390>

    @property
    def token_status(self) -> int:
        """Provide the current token_status."""
>       return self.auth_object.token_status
E       AttributeError: 'Detects' object has no attribute 'auth_object'

/usr/local/lib/python3.11/site-packages/falconpy/_service_class/_base_service_class.py:193: AttributeError

Hi @davidt99 -

The Uber Class does not supported Object Authentication. (In 1.3.0 it can be used as an authentication object, but it has never accepted the auth_object keyword.)

Can we see an example of the code being executed? The error above looks like it's originating from the Detects Service Class, not the Uber.

@davidt99
Copy link
Contributor

Sorry, my mistake, I meant service class.

@jshcodes
Copy link
Member

Hi @davidt99 - Can we see the executed code that generated this error?

@davidt99
Copy link
Contributor

This code is done in a test suite where I mock the OAuth2 object.
So something similar to:

auth_object: falconpy.OAuth2 | MagicMock = MagicMock(spec=falconpy.OAuth2(), autospec=True)
detects = falconpy.Detects(auth_object=auth_object)

But I see that in _base_service_class:L67, the type check is done by issubclass(type(auth_object), FalconInterface) and not isinstance(auth_object, FalconInterface) which autospec handles properly. I would be happy if the check would change (I can open a PR if needed).
Besides that, I also faced the authenticated method issue.

@jshcodes
Copy link
Member

Copy that. Testing this change now! 🙇

@jshcodes
Copy link
Member

jshcodes commented Sep 12, 2023

Hi @davidt99 -

You were right! Thank you for reporting this! 🙇

This was a quick fix and has cleared testing. We're including this update as part of the 1.3.2 release.

This is now your second really cool contribution... definitely come see us if you're coming to Fal.Con this year, we'll be on the expo hall floor with your swag! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication Issues or questions regarding authentication bug 🐛 Something isn't working potential breaking change ‼️ This update contains a potentially breaking change SDK usage General SDK usage issues and questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants