Skip to content

Handle invalid user docid in changes_callback#1827

Closed
popojargo wants to merge 1 commit intoapache:masterfrom
popojargo:fix/user-cache-failure-with-wrong-id
Closed

Handle invalid user docid in changes_callback#1827
popojargo wants to merge 1 commit intoapache:masterfrom
popojargo:fix/user-cache-failure-with-wrong-id

Conversation

@popojargo
Copy link
Member

@popojargo popojargo commented Dec 22, 2018

Overview

Handle user document id without correct id. I simply catch any errors and log a debug message (perhaps we want a info or warn level message?)

Testing recommendations

  1. Disable the VDU in _users database (I removed the validate function)
  2. Set the log level to Debug
  3. Add an valid user without an id (let Couch generate one)
  4. Replicate the user database to another database to trigger the changes_callback function.
  5. You should see the debug level saying that the id is invalid and the cache reader should not throw a function clause.

Related Issues or Pull Requests

#1821

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

try
UserName = username(DocId),
couch_log:debug("Invalidating cached credentials for ~s", [UserName]),
ets_lru:remove(?CACHE, UserName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a problem that we don't remove the doc from the cache if it has an invalid name? It seems like it should be removed if it's stale, despite it having an unexpected prefix.

Is it possible (or desirable) to prevent such a doc from even entering the cache at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look into this this weekend. I would be indeed better to simply prevent the doc to enter the cache. Altought, if the user document is invalid, can we still use the user credentials to login? That's something I will check today.

@wohali
Copy link
Member

wohali commented Oct 9, 2020

@popojargo any plans to get this in? I am preparing to mass-close old PRs that never got merged.

@popojargo popojargo closed this Oct 13, 2020
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.

3 participants