Skip to content

Enforce configured peer requirements in accountreq helper for all reqs - #587

Open
jonasbardino wants to merge 1 commit into
nextfrom
fix/accountreq-to-enforce-configured-peer-validation-even-for-reqs-without-peers-fields
Open

Enforce configured peer requirements in accountreq helper for all reqs#587
jonasbardino wants to merge 1 commit into
nextfrom
fix/accountreq-to-enforce-configured-peer-validation-even-for-reqs-without-peers-fields

Conversation

@jonasbardino

@jonasbardino jonasbardino commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Make sure the accept_account_req helper from accountreq always enforces any configured peer requirements even if the request handled lacks valid peer fields. This may or may not fix the issues reported in #586.

Extends the unit test suite with regression tests to cover a few cases of invalid or missing peers in combination with mandatory peer configuration.

Also makes sure that the various renewal and password reset tests cover the relevant 'temporal' account status rather than the empty default (interpreted as 'active').

Fast-forward a few unit tests to use the latest usersupp helpers to limit code duplication.

…forces any

configured mandatory peer requirements even if the request handled lacks valid
peer fields.

Extends the unit test suite with regression tests to cover a few cases of
invalid or missing peers in combination with mandatory peer configuration.

Also makes sure that the various renewal and password reset tests cover the
relevant 'temporal' account status rather than the empty default (interpreted
as 'active').

Fast-forward a few unit tests to use the latest usersupp helpers to limit code
duplication.
@jonasbardino jonasbardino self-assigned this Jul 5, 2026
@jonasbardino jonasbardino added the bug Something isn't working label Jul 5, 2026
@jonasbardino jonasbardino linked an issue Jul 5, 2026 that may be closed by this pull request
@jonasbardino
jonasbardino marked this pull request as ready for review July 5, 2026 13:49
@jonasbardino
jonasbardino requested a review from a team July 5, 2026 13:50

@Martin-Rehr Martin-Rehr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes looks sane but it seems like there is an issue in mig/lib/janitor.py:319:

if accept_account_req(req_id, configuration, peer_id, ...):
    _logger.info("accepted authorized %r access renew" % client_id)
else:
    _logger.warning("failed authorized %r access renew" % client_id)

accept_account_req returns a 2-tuple a non-empty tuple is evaluated to True, so the else is never reached ?

Martin-Rehr
Martin-Rehr approved these changes Jul 28, 2026
@jonasbardino

jonasbardino commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

The changes looks sane but it seems like there is an issue in mig/lib/janitor.py:319:

if accept_account_req(req_id, configuration, peer_id, ...):
    _logger.info("accepted authorized %r access renew" % client_id)
else:
    _logger.warning("failed authorized %r access renew" % client_id)

accept_account_req returns a 2-tuple a non-empty tuple is evaluated to True, so the else is never reached ?

Yes, I've already found and fixed it in 630acfa but thanks.

We can pull that particular fix in here as well to fast-track it if you like?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Janitor renewal of accounts without proper peer validation?

2 participants