fix(appauth): skip expired tokens before bcrypt and purge on load

[paul43210]

Summary

• Skip expired tokens before bcrypt: GetAppPassword now checks token expiration before the expensive bcrypt.CompareHashAndPassword call, preventing accumulated expired tokens from slowing down every authentication request (~70ms per token at bcrypt cost 11)
• Purge expired tokens on startup: New() removes expired tokens when loading the JSON file so they don't accumulate indefinitely. Persist failure is non-fatal (logged, not returned as error)
• Purge expired tokens during generation: GenerateAppPassword opportunistically cleans up expired tokens for the current user
• Use RWMutex for concurrent reads: ListAppPasswords and the lookup phase of GetAppPassword use RLock to allow concurrent read access

Background

This was originally submitted as owncloud/ocis#11998 against the vendored copy. Per @jvillafanez's review, re-submitting here against the reva source. All review feedback has been addressed:

1. Non-fatal purge-on-load — save failure after purging is logged but doesn't prevent startup
2. RLock/Lock upgrade in GetAppPassword — uses two-phase approach with re-check after lock upgrade, with comments explaining the tradeoff
3. Go spec citation for map deletion during range — both purgeExpiredUserTokens and purgeExpiredTokens include the spec reference
4. Dedicated isExpired() helper — extracted for consistent expiry checks across all call sites

Test plan

• New json_expiry_test.go with 7 tests covering:
  • Expired tokens are skipped during auth
  • Valid tokens still authenticate correctly
  • Nil expiration means "never expires"
  • Zero-second expiration means "never expires"
  • Purge on load removes expired tokens from JSON file
  • GenerateAppPassword purges expired tokens for the user
  • isExpired() unit tests (nil, zero, future, past)
• Updated existing json_test.go: adjusted ListAppPasswords with expired password test expectation (purge-on-load now removes them)
• All 22 tests pass (7 new + 15 existing)

🤖 Generated with Claude Code

[jvillafanez]

I think we'll have to ignore the error in this particular case. Logging like this will ignore any configuration over the logging facility we could have, so it isn't good. Usually, we should get the logger from the context, but there is no context available here.
Unless there are better ideas, I'd vote for ignoring the error.

[jvillafanez]

I don't really like using now() as function here. It's part of the package under testing, so it should be tested. I can agree that testing it isn't worthy, but since it's part of the package that is being tested, we shouldn't use it here.

[jvillafanez]

I'm not sure if there are better alternatives... This is an implementation detail that shouldn't be here, but I understand that getting the actual hash is going to be painful.
If the hash is short, we can try to use a binary string, but if it will make the test unreadable it isn't worthy.

[jvillafanez]

Regarding the tests, I think we should include a configuration option to decide whether purge the tokens during initialization or not. Something like keep_expired_tokens_on_load.

By default it should be false, so we keep the current behavior of purging the tokens during initialization, but for the tests we can set it to true so we can inject expired tokens for the tests.
This is because the tests should only access to public methods of the package being tested. We aren't interested in private methods (there could be exceptions to this rule), and we shouldn't care about the internals of the object we're testing; we only care about the input and the output.

This is the easiest solution for you to test with expired tokens without having to modify the internal state of the auth manager.

---

Side note, the configuration options are in https://github.com/paul43210/reva/blob/4dbca37085263a3336ed88d65d72fb6350e6d8e5/pkg/appauth/manager/json/json.go#L49-L53 , so adding a boolean value there is pretty easy. I don't think we'll need to link the configuration option with oCIS for now because we want to purge the tokens by default, but it's possible to add a configuration option in oCIS in case we want to let the admin to configure it. For now, let's keep it just for testing.

[paul43210]

Thanks for the review! All three points addressed in 65f86bd:

1. log.Printf removed — replaced with _ = manager.save(). Agreed there's no good way to log here without a context, so silently ignoring is the cleanest option.
2. now() in tests — replaced with inline &typespb.Timestamp{Seconds: uint64(time.Now().Unix())} so we're not relying on the package under test.
3. bcrypt hash — keeping as-is since the alternative would hurt readability.

[paul43210]

All addressed in 43b9473:

Config option: Added keep_expired_tokens_on_load (default false). Tests use this to inject expired tokens via GenerateAppPassword without them being purged on startup.

Tests refactored to public API only:

• newTestMgr now returns appauth.Manager interface instead of *jsonManager
• All token creation goes through GenerateAppPassword, verification through ListAppPasswords/GetAppPassword
• No more direct access to mgr.passwords — removed bcrypt, encoding/json, and os imports from the test file
• Kept TestIsExpired as the one exception since it's a pure function with clear input/output

log.Printf removed: Save error is silently ignored with _ = manager.save().

[jvillafanez]

This isn't doing what the test reads...
I expect to have / create a json file with the data containing the tokens, some of them expired. You load the file and check the tokens. You can reuse the same data or file to check what happens if we purge the tokens on load or not (2 different tests)

[jvillafanez]

This is a different test

[jvillafanez]

With the tests I've notice that, even if the GenerateAppPassword purges expired tokens, it still allows expired tokens to be added. This seems odd to me.

• GenerateAppPassword might allow adding expired tokens without purging. We'd need to move the purging to another place. This doesn't seem a good idea.
• GenerateAppPassword might outright reject adding expired tokens. If the token has been expired already, the call to the method is ignored (quick return without doing anything). Purging is kept because tokens might expire naturally after a while.
• GenerateAppPassword can add expire tokens, but those will be purged right before the data is saved. By the time the call to the method finishes, all the expired tokens are gone, including the one just added.

Second option seems the right one. I'm not entirely sure if we should purge the tokens under those conditions or if we can ignore the call entirely (expired tokens are already there, so it might not matter if they're kept for a while longer).

In addition, check the names of the tests. In general, a test should only have one different call for the package under testing, so if you're testing the GetAppPassword there shouldn't be other calls to other methods. I think it's fine to have some "set + get" tests, basically to test the "set" operations (GenerateAppPassword in this case), but this should be reflected in the test name.
The idea to include the optional keep_expired_tokens_on_load configuration was to allow including data without relying on the GenerateAppPassword method, so you can test the GetAppPassword method in a clean way.