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

Bad Request on every api call with v1.0.3 #36

Open
nikned opened this issue Jul 5, 2018 · 11 comments
Open

Bad Request on every api call with v1.0.3 #36

nikned opened this issue Jul 5, 2018 · 11 comments

Comments

@nikned
Copy link

nikned commented Jul 5, 2018

after migrating from v1.0.1 to v1.0.3, every API call to the secureAuth instance (v9.2.0) SAExecuter returns back "Bad Request"

Note : The same request(s) works with v1.0.1 .
Note: All provided integration tests are failing with the same issue.

PFA
secure-auth-api-error

@ghost
Copy link

ghost commented Jul 5, 2018

@nikned Looks like the Extended Date format that was updated is causing this error across the board. Apologies I let this out. I will post a patch, that will be versioned as 1.0.3.1

@ghost
Copy link

ghost commented Jul 5, 2018

@nikned I pushed the update, You should have all tests pass with this version.

@ghost ghost self-assigned this Jul 5, 2018
@nikned
Copy link
Author

nikned commented Jul 5, 2018

@sa-rrowcliffe , this issue is fixed now , appreciate it 👍

I see another issue with factorsByUser call ,
user_id : [email protected] ( gmail + trick to create test accounts)
Note : all the API calls search the user successfully in adlds except "factorsByUser" call

I see recent change in v1.0.3 is uncommenting encode() function in public FactorsResponse factorsByUser(String userid) method in SAAccess. This is causing multiple issues ,
here is the scenario:
in v1.0.3 :

  1. user_id : [email protected] ==> after encoding ==> test%2BsecureAuth%40gmail.com status 404 , reason=not found
    2.user_id : [email protected] ==>after encoding ==> user_id : testsecureAuth%40gmail.com
    status 401, reason=unauthorized
    3.user_id : [email protected] ==>force skipping encoding ==> user_id : [email protected]
    status 200, reason=ok
    4.user+id : [email protected] ==> force skipping encoding ==> user_id: [email protected] ( note this specific scenario is happening in v1.0.1 as well )
    status 404 , reason=not found.

@ghost
Copy link

ghost commented Jul 5, 2018

@nikned We uncommented it for a specific case. Seems it is impacting the more general use cases.
Think we should put a boolean for encoding in the SDK?

@nikned
Copy link
Author

nikned commented Jul 6, 2018

@sa-rrowcliffe ,

  1. I see in code most of the calls are not doing any encoding for e.g adaptauth,develiverOTPby<sms/email/phone> etc .. and these are working as expected , don't know the case why encoding is needed only on factorsByUser call.
    2.I believe keeping a flag for encoding will give better option to turn on/off on encoding as needed .But still that will not solve the issue with "+" sign or any special characters in the userIds just for the factorsByUser call , where rest of the calls are working as expected. may be a revisit on factorsByUser implementation why its behaving differently and encoding logic along with boolean for encoding ?

@ghost
Copy link

ghost commented Jul 6, 2018

@nikned I think we are going to run into a challenge with the + or other special Char, The encoding is really looking for UPN format, the IDP itself isn't parsing the encoding in the form of URLEncoding. I have put in a request into IDP to support URL encoding

@nikned
Copy link
Author

nikned commented Jul 6, 2018

@sa-rrowcliffe

Note : other calls are able to fetch user_ids with + signs from adlds . fyi , UPNs on our adlds are "email addresses with plus signs" and calls are working as expected , its only factorsCall which is having this issue.

  1. And also i believe instead of making factors request with username in the url
    ==> /users/{username}/factors

like other calls( eg adaptiveauth /api/v1/adaptauth) by passing username as parameters
/users/factors/
Json params
{
"user_id": "",
}
would solve much issues with encoding and other cases.

  1. any suggestions how to make + work until the fix is in place ? any work arounds which i can try ?, as this is blocking us moving forward on consuming secureAuth api

@ghost
Copy link

ghost commented Jul 6, 2018

@nikned

The + support has to come from IDP. The SDK is just trying to make implementing the rest APIs easier.

Can you open a support case to call out the urlencoding which would be an easier fix then changing the endpoint.

We use path parameters in many parts of the restapi

@nikned
Copy link
Author

nikned commented Jul 6, 2018

@sa-rrowcliffe thanks for the clarification , we are going to open support case for this

@nikned
Copy link
Author

nikned commented Jul 6, 2018

@sa-rrowcliffe i believe placing a boolean for encoding in the SDK would be still viable at this point thus gives option to turn off to make general usecases work, curretly its breaking most of the general userId formats

@nikned
Copy link
Author

nikned commented Jul 16, 2018

@sa-rrowcliffe just to re-check any eta on this fix for adding boolean flag for encoding?

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

No branches or pull requests

1 participant