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: implement blacklist of tokens #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dariocravero
Copy link
Contributor

No description provided.

@@ -12,3 +12,6 @@
username_claim = sub
; Claim that is used as CouchDB roles. The claim must contain array of the users roles.
roles_claim = roles

; Blacklist tokens
blacklist = []
Copy link

Choose a reason for hiding this comment

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

This one is redundant as you use separate section for blacklisting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well spotted :) it was a leftover from the previous iteration that I oversaw :/

@@ -58,6 +58,12 @@ decode(Token, Config) ->
end, List), posix_time(calendar:universal_time()), Config)
end.

ensure_safe_token(Token, Config) ->
case couch_util:get_value(Token, Config) of
Copy link

Choose a reason for hiding this comment

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

How may tokens could be blacklisted? If you expect hundred/thousand values as real ones, better use couch_config:get/3 here since ETS table behind is more suitable for random access than lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it could be any number, it's really up to whomever runs the service.
Is this the fn you're talking about? https://github.com/apache/couchdb/blob/1.6.x/src/couchdb/couch_config.erl#L59
Would it be something like this?

ensure_safe_token(Token) ->
 case couch_config:get("jwt_auth_blacklist", Token) of
 ...

How would you test it?

Copy link

Choose a reason for hiding this comment

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

Right, I thought about /2 (:
For testing see https://github.com/apache/couchdb/blob/1.x.x/test/couchdb/couch_config_tests.erl
TL;DR start couch_config app, put there some value, call your function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good stuff, that seems like a cleaner approach. It could also be used across other sections of the code too I guess.
I wonder can I mock couch_config:get instead of setting up the whole lot... Would meck be a good start? (and when I was about to hit Comment I found this https://github.com/apache/couchdb-meck :P)

Copy link

Choose a reason for hiding this comment

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

You can use meck with 2.0. 1.x doesn't provides it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ok... Since we're in the 2.0 discussion, according to what you know, what would it take to make this 2.0 compatible?

Copy link

Choose a reason for hiding this comment

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

Well, for 2.0 better try to compile project and fix all the ongoing errors. For instance, couch_config turned into config. I'm not familiar with this codebase to say what else you'll have to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, will give that a go over the next few weeks. Thanks!

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