Add session idle lifetime and make session lifetime doubles

pelletier197 commented 2 years ago

Changes

Added session idle lifetime for tenant management
converted session lifetime into doubles (to allow setting less than an hour)
- :warning: since I changed int to double, it's a breaking change. A pretty small one, but still.

References

Here is an example on my tenant that session lifetime and idle session lifetime can be double

Testing

[ ] This change adds test coverage
[X] This change has been tested on the latest version of the platform/language or why not

Sample code

public class Main {
    public static void main(String[] args) throws Auth0Exception {
        ManagementAPI api = new ManagementAPI("<>", "<>");
        Tenant tenant = api.tenants().get(null).execute();
        System.out.println("Session:" + tenant.getSessionLifetime());
        System.out.println("Idle:" + tenant.getIdleSessionLifetime());

        Tenant newTenant = new Tenant();
        newTenant.setIdleSessionLifetime(0.5);
        newTenant.setSessionLifetime(12.0);
        api.tenants().update(newTenant).execute();

        Tenant updatedTenant = api.tenants().get(null).execute();
        System.out.println("Updated Session:" + updatedTenant.getSessionLifetime());
        System.out.println("Updated Idle:" + updatedTenant.getIdleSessionLifetime());
    }
}

Should output

Session:72.0
Idle:48.0
Updated Session:12.0
Updated Idle:0.5

Checklist

[X] I have read the Auth0 general contribution guidelines
[X] I have read the Auth0 Code of Conduct
[X] All existing and new tests complete without errors

pelletier197 commented 2 years ago

As I stated in the PR, I modified an Integer to a Double. Do you consider this to be an issue? If yes, to you have an alternative to propose to still have the possibility to use a Double as the sessionLifetime ?

jimmyjames commented 2 years ago

👋 Hi @pelletier197, thanks for the PR.

I looked at the schema of the tenants API, and the session_lifetime and idle_session_lifetime are defined as integers. When I tried updating the settings using a double (e.g., 75.5), I get a 400 error:

{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "Payload validation error: 'Expected type integer but found type number' on property idle_session_lifetime (Number of hours for which a session can be inactive before the user must log in again).",
  "errorCode": "invalid_body"
}

We should add support for idle_session_lifetime, but I think it should be an integer. Did you get a successful response when updating a tenant with a double value? If so, what value?

Regarding the change from Integer to Double, it may be a moot point based on the above, but as you noted it would be a breaking change and so we shouldn't do it in the current release. We can identify a different approach to prevent a breaking change, but first let's figure out if it even can/should be a double.

Thanks!

pelletier197 commented 2 years ago

@jimmyjames That is quite strange. I get the same error as you now. I swear that this code worked 2 days ago :thinking: :thinking: :thinking: :thinking:

What is even weirder is that, in the UI, when you go in the Dashboard > Settings > Login Session Management (Notice, the time is in minutes in the UI, but in hours in the API :shrug: ). If you try to go in the UI and save the settings in minutes, then call the API to fetch tenant settings, you'll see a response in double...

This is the answer I get when I call the management API:

"idle_session_lifetime": 0.5,
"session_lifetime": 12,

But you are right, you are only able to do the PATCH with an int.. Feels like the issue is on the Auth0 API then, wouldn't it? Now here's what I propose:

I will keep integer for now, but receiving 0.5 will turn it to 0, which doesn't quite make sense.
And you can check on your side what's wrong with this API in the meantime I guess.

I still won't be able to use the API cause I need the timeout to be 30 minutes to respect our pen-tester recommendations, but we did it manually in the UI for now..

Thanks a lot!

jimmyjames commented 2 years ago

Thanks @pelletier197, yes I see the same behavior you are reporting. It is confusing 😕

Sometimes the dashboard will use non-public APIs; that may be what's happening here. It is unfortunate that the API will return a double when the lifetime is set through the UI to be fractions of hours, but I think your assessment of keeping it an int aligns with the public API and is probably the best we can do for now. I'll think about it a bit then review the specific changes.

Thanks for raising and providing all the details 🙇

jimmyjames commented 2 years ago

@poovamraj would you mind reviewing this PR while I'm out? As you can see from the conversation, there is a discrepancy between the API behavior and the dashboard UX. Given that the API only works in whole hours, I think adding support for the session idle lifetime as an integer makes sense.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

pelletier197 commented 2 years ago

@jimmyjames any news ? I don't really need this much anymore, but any reason not to merge it? Otherwise, I'll close it. Thanks!

jimmyjames commented 2 years ago

Hey @pelletier197, apologies for the delay while I was away. I'm going to look into this again this week and see if this change is something we can do, or if it's something we can't adequately address in the SDKs. Thanks!

auth0 / auth0-java