Skip to content

Add session idle lifetime and make session lifetime doubles#423

Merged
jimmyjames merged 6 commits intoauth0:masterfrom
pelletier197:feature/support-session-idle-lifetime-and-double-session-lifetime
Aug 30, 2022
Merged

Add session idle lifetime and make session lifetime doubles#423
jimmyjames merged 6 commits intoauth0:masterfrom
pelletier197:feature/support-session-idle-lifetime-and-double-session-lifetime

Conversation

@pelletier197
Copy link
Copy Markdown
Contributor

@pelletier197 pelletier197 commented Apr 19, 2022

Changes

  • Added session idle lifetime for tenant management
  • converted session lifetime into doubles (to allow setting less than an hour)
    • ⚠️ 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

image

Testing

  • This change adds test coverage
  • 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

@pelletier197 pelletier197 requested a review from a team as a code owner April 19, 2022 19:31
@pelletier197
Copy link
Copy Markdown
Contributor Author

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 jimmyjames added the needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue label Apr 21, 2022
@jimmyjames
Copy link
Copy Markdown
Contributor

👋 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
Copy link
Copy Markdown
Contributor Author

pelletier197 commented Apr 21, 2022

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

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 🤷 ). 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...

image

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:

  1. I will keep integer for now, but receiving 0.5 will turn it to 0, which doesn't quite make sense.
    image

  2. 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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

@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
Copy link
Copy Markdown

stale bot commented Aug 13, 2022

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! 🙇‍♂️

@stale stale bot added the closed:stale Issue or PR has not seen activity recently label Aug 13, 2022
@pelletier197
Copy link
Copy Markdown
Contributor Author

@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 jimmyjames removed the closed:stale Issue or PR has not seen activity recently label Aug 30, 2022
@jimmyjames
Copy link
Copy Markdown
Contributor

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!

Copy link
Copy Markdown
Contributor

@jimmyjames jimmyjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an update to the javadocs requested, otherwise looks good. Thanks!

@jimmyjames jimmyjames added CH: Added and removed needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue labels Aug 30, 2022
@jimmyjames jimmyjames added this to the v1-Next milestone Aug 30, 2022
@jimmyjames jimmyjames merged commit 41d7425 into auth0:master Aug 30, 2022
@jimmyjames jimmyjames mentioned this pull request Sep 19, 2022
@jimmyjames jimmyjames modified the milestones: v1-Next, 1.43.0 Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants