Cat won't talk to strangers if not allowed to

[lucagobbi]

Description

Currently, if you set only the CCAT_WS_API_KEY you'll still be able to talk to the Cat even when no key is passed.

I fixed it refactoring the authorize_user_from_key method.

Actually, I think that, in the future, we should move out the control: "if no keys are set, then always pass", but is it strictly related to the AuthHandler? If so, then its right to keep it there even if out of scope. Plus, is it correct to gave base permissions if no keys are set? Probably would be better to distinguish even there the requested resource.

Related to issue #895

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas

[pieroit]

If no keys are set, I guess we can give full permissions to anybody?
In this way if there are no keys set, you cannot use most endpoints (I need to check)

[lucagobbi]

Yep, agree with that, otherwise we could differentiate by requested resource:

if not http_key and not ws_key:
            return AuthUserInfo(
                id=user_id,
                name=user_id,
                permissions=get_base_permissions() if auth_resource == AuthResource.CONVERSATION else get_full_permissions()

[pieroit]

There are also endpoints for conversation not related to websocket :(

[lucagobbi]

Damn, you're right! So, currently, you have to pass the WS api key for the http message endpoint if only the WS key is set? And when only the http key is set if you not passing the api key you can message freely because None == None...

[pieroit]

Thanks @lucagobbi I left some comments
Maybe we can have a chat to redesign a few things

[lucagobbi]

Thanks @lucagobbi I left some comments Maybe we can have a chat to redesign a few things

Thank you Piero, I think we might need some more adjustments, ping me whenever you want

[pieroit]

API KEY permissions rationale:

• no CCAT_API_KEY, no CCATA_API_KEY_WS = full permissions on everything
• no CCAT_API_KEY, yes CCATA_API_KEY_WS = full permissions on http, ws only if key is present
• yes CCAT_API_KEY, no CCATA_API_KEY_WS = full permissions on http if you provide key, open ws
• yes CCAT_API_KEY, yes CCATA_API_KEY_WS = full permissions on both if you provide the key

JWT:

• if you are using JWT without setting the keys????
• in this case the auth handlers are active

P.S.: intervention point (here)

[lucagobbi]

API KEY permissions rationale:

• no CCAT_API_KEY, no CCATA_API_KEY_WS = full permissions on everything
• no CCAT_API_KEY, yes CCATA_API_KEY_WS = full permissions on http, ws only if key is present
• yes CCAT_API_KEY, no CCATA_API_KEY_WS = full permissions on http if you provide key, open ws
• yes CCAT_API_KEY, yes CCATA_API_KEY_WS = full permissions on both if you provide the key

JWT:

• if you are using JWT without setting the keys????
• in this case the auth handlers are active

P.S.: intervention point (here)

I cant manage to find a way to do what we have said during the last dev meeting without a proper refactor of those classes, which, for the moment, I'd avoid.

my alt proposal is to:

• set full_permissions if no keys are set (as you've suggested in your first comment)
• add a specific auth resource for the HTTP message endpoint like: AuthResource.MESSAGE or distinguish two conversation resources: AuthResource.CONVERSATION_WS / AuthResource.CONVERSATION_HTTP. In this way we can keep this PR as is, following the API KEY rationale you proposed, fixing those little security bugs and keeping separate ws controls from http ones.

What do you think?

[pieroit]

I cant manage to find a way to do what we have said during the last dev meeting without a proper refactor of those classes, which, for the moment, I'd avoid.

Sorry for this :(

my alt proposal is to:

• set full_permissions if no keys are set (as you've suggested in your first comment)
• add a specific auth resource for the HTTP message endpoint like: AuthResource.MESSAGE or distinguish two conversation resources: AuthResource.CONVERSATION_WS / AuthResource.CONVERSATION_HTTP. In this way we can keep this PR as is, following the API KEY rationale you proposed, fixing those little security bugs and keeping separate ws controls from http ones.

What do you think?

Not sure introducing new permissions is the best way.

What's wrong with this PR as is?
If you use the WS key you can use WS, if you use the HTTP key you can use HTTP

POST /message is an http endpoint, so no issue right?

[lucagobbi]

@pieroit I've added the protocol based control that we talked about yesterday, this decouples auth key methods from auth resources. What do you think about it?

[pieroit]

Looks great! Will check asap locally

[pieroit]

Thanks @lucagobbi for the contribution and patience :)
I rewrote the tests on keys to check them independently