okta-sdk-python icon indicating copy to clipboard operation
okta-sdk-python copied to clipboard

Potential data loss when saving app profile fields

Open cmc333333 opened this issue 1 year ago • 7 comments

Hello!

We have an app with a custom schema, including some fields that are snake_cased. Say we've already assigned some properties to GROUP_ID for app APP_ID. When we run:

with Client() as client:
    assignment, _, _ = client.get_application_group_assignment(APP_ID, GROUP_ID)
    # should be a no-op:
    client.create_application_group_assignment(APP_ID, GROUP_ID, assignment)

we were seeing our attributes cleared out. Digging in, it seems that assignment.profile has converted the field names to camelCase (here, I think). This was particularly insidious as our application wasn't modifying the snake_cased fields.

I'd think the "right" thing to do here would be for profiles (user, group, application) to skip camelCasing -- it looks like there's already some logic in UserProfile to do something in this vein.

cmc333333 avatar Feb 15 '24 14:02 cmc333333

@cmc333333 do u have async and await ?

shouldn't it be

async with Client() as client:
    assignment, _, _ = await client.get_application_group_assignment(APP_ID, GROUP_ID)
    # should be a no-op:
    await client.create_application_group_assignment(APP_ID, GROUP_ID, assignment)

gabrielsroka avatar Feb 21 '24 02:02 gabrielsroka

Sorry, yes, please assume there was an await in there. We're converting it all to synchronous versions.

Also, as written, I think the call would error out rather than silently fail (Okta rejects the camelCase version of the field). It looks like both UserProfile and GroupProfile have some logic to account for this problem, so it could just be App profiles that are the problem?

cmc333333 avatar Feb 21 '24 14:02 cmc333333

We're converting it all to synchronous versions

how?

It looks like both UserProfile and GroupProfile have some logic to account for this problem

where?

here's my repro code

import okta.client
import asyncio

APP_ID = '0oa...'
GROUP_ID = '00g...'

async def main():
    async with okta.client.Client() as client:
        assignment, _, _ = await client.get_application_group_assignment(APP_ID, GROUP_ID)
        print(assignment.profile)
        await client.create_application_group_assignment(APP_ID, GROUP_ID, assignment)

asyncio.run(main())

i have attributes snake_case and camelCase, but the print shows {'camelCase': 'perl', 'snakeCase': 'python'} (which is wrong)

if i set logging enabled to true, i get

okta-sdk-python - http_client - ERROR - 
{'message': "Okta HTTP 400 E0000001 Api validation failed: Setproperty\nProperty 'snakeCase' not found"}

it sounds like the whole snake_case to camelCase conversion shouldn't be done (not that it should be converted and then converted back again)

gabrielsroka avatar Feb 21 '24 17:02 gabrielsroka

how?

async_to_sync. Not super related to the problem at hand.

where?

https://github.com/okta/okta-sdk-python/blob/d896f65ceab0671e64945525a84af6ce389fdd1d/okta/models/group_profile.py#L45-L49

https://github.com/okta/okta-sdk-python/blob/d896f65ceab0671e64945525a84af6ce389fdd1d/okta/models/user_profile.py#L132-L136

I've not dug in enough to understand how that logic's injected, though -- the call to form_response_body is probably before then.

Okta HTTP 400 E0000001

Exactly. I think there are weird edge cases where setting snake_case could override snakeCase and lead to data loss, but the important thing is that the calls aren't doing the "right thing".

it sounds like the whole snake_case to camelCase conversion shouldn't be done

That seems like a fine solution to me! We're working around this by using the response object (second element of the return value) directly.

cmc333333 avatar Feb 21 '24 22:02 cmc333333

Not super related to the problem at hand.

yes, and no. i can't repro your code without async and await (i'm not going to use a2s). a2s might have bugs, too. i'd recommend posting code just using the sdk

(i'll take a look at the rest of your response and reply)

gabrielsroka avatar Feb 21 '24 22:02 gabrielsroka

I'm glad you were able to see through my buggy code to the underlying problem.

cmc333333 avatar Feb 21 '24 22:02 cmc333333

ooh, i can print(error). i've never done that before

async def main():
    async with okta.client.Client() as client:
        assignment, response, error = await client.get_application_group_assignment(APP_ID, GROUP_ID)
        print(assignment.profile)
        # should be a no-op:
        assignment, response, error = await client.create_application_group_assignment(APP_ID, GROUP_ID, assignment)
        print(error)

i've looked the sdk code. still haven't found anything, but this is promising https://github.com/okta/okta-sdk-python/blob/d896f65ceab0671e64945525a84af6ce389fdd1d/okta/api_response.py#L136-L144

gabrielsroka avatar Feb 22 '24 14:02 gabrielsroka