search

zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
359 stars 361 forks source link

zulip: Add authentication before initializing client. #592

Closed TAM360 closed 4 years ago

TAM360 commented 4 years ago

Before sending any message to a user/stream via CLI, user's credentials are verified first. The idea is to update the users before they start using it (#533). This is done using validate_credentials() method which returns returns the result of get_profile API. The status of result is used to determine if the credentials are valid or not.

showell commented 4 years ago

@TAM360 Do you think you have time in the next couple days to address the remaining comments? It looks like it's mostly fixing a typo and limiting the scope of the try/except block.

TAM360 commented 4 years ago

@showell yeah I do. I am gonna try to finish this by Sunday.

showell commented 4 years ago

@TAM360 Can you rebase these commits into a single commit? We don't really want or need the merge commits or the commits that are just code-review fixes.

TAM360 commented 4 years ago

@showell I've already pushed 3-4 small commits to my branch. Will they get affected if I rebase my commits now and then push it?

showell commented 4 years ago

@showell I've already pushed 3-4 small commits to my branch. Will they get affected if I rebase my commits now and then push it?

Yes, but that's the idea behind rebasing--I want you to replace those.

TAM360 commented 4 years ago

@showell all right.

aero31aero commented 4 years ago

From my previous comment:

We should have this block of code in the Client class, just before it returns the client object.

It looks like you've added the code to throw the error in multiple places; we only need it in the zulip Client's constructor.

TAM360 commented 4 years ago

@aero31aero I've added the validation logic in Client's constructor but as a result it's failing 4 of the unit test. Should I update those tests or add new ones?

TAM360 commented 4 years ago

@zulipbot abandon

zulipbot commented 4 years ago

ERROR: You have not claimed this issue to work on yet.

andersk commented 4 years ago

(I’m not sure this PR is still active, but it’s apparently still being referenced.)

"result": "error" doesn’t necessarily imply that the credentials are wrong. We also need to check for HTTP status 401 or "code": "INVALID_API_KEY".

Also, sys.exit() returns a successful exit code that’s inappropriate for an error condition. I think in most cases we just want to let the exception propagate up.

showell commented 4 years ago

I am gonna close this since @TAM360 has abandoned it. @TAM360 Thanks for working on this!