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

feat: add parameter to customize token lifespan #92

Merged
merged 4 commits into from
Nov 11, 2024
Merged

Conversation

endroca
Copy link
Contributor

@endroca endroca commented Nov 4, 2024

No description provided.

@xgp
Copy link
Member

xgp commented Nov 4, 2024

Thanks @endroca . 2 things:

  1. create an issue for tracking this
  2. please run mvn fmt:format before committing, as the diff isn't readable because of your formatting changes

Copy link
Member

@xgp xgp left a comment

Choose a reason for hiding this comment

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

Couple of changes. Otherwise looking good.

return OptionalInt.empty();
}

return OptionalInt.of(Integer.parseInt(lifespan));
Copy link
Member

Choose a reason for hiding this comment

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

Should catch a NumberFormatException here, as the string doesn't get validated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I didn't validate that... there are two approaches here: Forcing an unknown error for Keycloak and preventing login, or going with the default settings and logging an error.

I'm more inclined to use the default configuration. What do you think?

@endroca endroca requested a review from xgp November 4, 2024 18:13
@endroca endroca requested a review from xgp November 7, 2024 02:14
@xgp xgp merged commit 374d88d into p2-inc:main Nov 11, 2024
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

Successfully merging this pull request may close these issues.

2 participants