Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

ui_auth.session_timeout config should allow standard duration formats #9239

Closed
richvdh opened this issue Jan 27, 2021 · 10 comments
Closed

ui_auth.session_timeout config should allow standard duration formats #9239

richvdh opened this issue Jan 27, 2021 · 10 comments
Labels
good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@richvdh
Copy link
Member

richvdh commented Jan 27, 2021

most durations in the config can be specified in terms of number of seconds/hours/etc (search for parse_duration). ui_auth.session_timeout seems to have missed the memo.

@richvdh
Copy link
Member Author

richvdh commented Jan 27, 2021

related: #5584 (comment)

@anoadragon453 anoadragon453 added good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. labels Feb 1, 2021
@arya2331
Copy link
Contributor

Hey @richvdh, I wanted to work on this issue. Can I go ahead? How can I start with this?

@richvdh
Copy link
Member Author

richvdh commented Feb 11, 2021

yes, absolutely. We welcome any contributions - no need to ask permission!

To get started, I suggest you read through https://github.com/matrix-org/synapse/blob/develop/CONTRIBUTING.md if you haven't already, and get yourself a copy of Synapse running on your development machine. Then just find where this setting is parsed, and update the parsing code to allow more duration formats!

If you have any questions, feel free to ask in #synapse-dev:matrix.org.

@arya2331
Copy link
Contributor

@richvdh what more duration formats are to be allowed in parse-duration?

@richvdh
Copy link
Member Author

richvdh commented Feb 12, 2021

parse_duration itself is fine. The problem is that it's not being called for ui_auth.session_timeout.

@arya2331
Copy link
Contributor

arya2331 commented Feb 12, 2021

@richvdh I have written the code for calling parse_duartion for ui_auth.session_timeout. Is that ok? and how should I test it?

@richvdh
Copy link
Member Author

richvdh commented Feb 16, 2021

To test it:

  • do the existing unit tests pass?
  • you could try manually configuring Synapse with different duration formats and checking that it starts ok
  • you could add a new AuthConfigTestCase that checks the setting is parsed correctly (look at tests.config.test_ratelimiting.RatelimitConfigTestCase for an example of testing config parsing)
  • you could also maybe update tests.rest.client.v2_alpha.test_auth.UIAuthTests.test_can_reuse_session to use a formatted duration for session_timeout instead of an integer.

@arya2331
Copy link
Contributor

arya2331 commented Feb 17, 2021

@richvdh all existing unit tests passed and I am all set to raise a PR. Now I have a doubt, should I push only changes related to calling parse_duration or also push changes for `tests.rest.client.v2_alpha.test_auth.UIAuthTests.test_can_reuse_session'.

@clokep
Copy link
Member

clokep commented Feb 17, 2021

@richvdh all existing unit tests passed and I am all set to raise a PR. Now I have a doubt, should I push only changes related to calling parse_duration or also push changes for `tests.rest.client.v2_alpha.test_auth.UIAuthTests.test_can_reuse_session'.

You'll want to push both changes so that we can test the new code.

@clokep
Copy link
Member

clokep commented Feb 18, 2021

Fixed by #9426.

@clokep clokep closed this as completed Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

4 participants