-
Notifications
You must be signed in to change notification settings - Fork 0
Use SkypeToken authentication in chat API. #1
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
base: master
Are you sure you want to change the base?
Conversation
665d7ed to
e8394ae
Compare
…tly, because it will be obtained passively from server responses.
…Token if available. Add TFL specific headers.
…tion token authentication provider.
test/client.py
Outdated
| self.assertEqual(sk.conn.tokens["reg"], "registrationToken={0}".format(Data.regToken)) | ||
| # Registration token is now obtained passively, so it won't be present until an API call returns it | ||
| # Make an API call that will trigger registration token extraction | ||
| sk.chats.recent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that you put it here just to check ["reg"] token and it is part of authentification, but the test testChatList already exist. We can do the same checks there.
Everything that can be tested for a lot of functionalities like redirects etc, can be moved to separate functions with assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not an auth test, this is test to check that when reg token is provided by chat API it is saved to reuse in subsequent requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correct, the adding of reg token is the part of auth process. And by the way, I think, we can need to check it with a lot of requests from the lib.
| body=json.dumps({"skypetoken": Data.skypeToken, "expiresIn": 86400})) | ||
| # Request registration token. | ||
| # Registration tokens are now provided passively in response headers | ||
| expiry = int(time.mktime((datetime.now() + timedelta(days=1)).timetuple())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is interesting, why is Data.tokenExpiry not used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is used in users/ME/conversations mock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a problem?
test/client.py
Outdated
| self.assertEqual(sk.conn.endpoints["main"].id, "{{{0}}}".format(Data.endpointId)) | ||
| # Registration token is now obtained passively, so it won't be present until an API call returns it | ||
| # Make an API call that will trigger registration token extraction | ||
| sk.chats.recent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be just auth redirects, not another functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, here the issue is with redirect from /endpoint api that was used on explicit token retrieval, so tests is adjusted to how it works now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sk.chats.recent() haven't used here earlier. And just authentication via Skype() was tested. This is ok, that you removed assertions that are not actual for authorization now.
test/client.py
Outdated
|
|
||
| expiry = int(time.mktime(Data.tokenExpiry.timetuple())) | ||
| # Mock a request that returns a Set-RegistrationToken header | ||
| responses.add(responses.GET, "{0}/users/ME/conversations".format(SkypeConnection.API_MSGSHOST), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have added this dummy earlier, as I see. and generally I think we can just not do this test. We checked the same things earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better to have explicit test for this functionality, test id left, recent() calls removed form auth tests
| sk = mockSkype() | ||
|
|
||
| # Intercept the actual request to verify headers | ||
| def request_callback(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can just do this function separately and use everywhere it needed. This test can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test is checking if headers included in requests created by chat api, totally legit tests, no other places where it is checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but we can check them on each request, where it is needed. Do you think that no scenarios when it could be useful?
test/client.py
Outdated
| registerMocks() # Re-register all the standard mocks | ||
|
|
||
| # Add our specific test mock with registration token | ||
| responses.add(responses.GET, "{0}/users/ME/conversations".format(SkypeConnection.API_MSGSHOST), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all dummies through responses are collected in registerMocks. They shouldn't be in tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general they must be per tests, so having all dummied in one method is bad pattern, because no arrange state visibility, but leaving as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of urls that can repeat from test to test. It will be hard to repeat responses dummies prepare for each.
| # No-op: Registration tokens are now extracted from Set-RegistrationToken headers | ||
| # in all HTTP responses via _extractRegistrationToken() | ||
| pass | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think better to do something like NotImplementedError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From backward compatibility it is not, better to have it undefined, it won't introduce breaking chages. I future it should just be removed, but on this stage looks ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean the legacy code with usage of previous versions skpy by users? How will they understand then, that this is not supported place that should be changed? If it just returns None, it can be a suffer to debug.
| """ | ||
| # No-op: Registration tokens are now extracted from Set-RegistrationToken headers | ||
| # in all HTTP responses via _extractRegistrationToken() | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_extractRegistrationToken doesn't exist yet here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_extractRegistrationToken is in same file, so it is correct to reference it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_extractRegistrationToken is in same file, so it is correct to reference it here
yes, but it didn't exist in this commit yet :)
| return resp | ||
|
|
||
| def _extractRegistrationToken(self, resp): | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think would be better to name like _initializeRegTokenFromHeader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init is not correct here because reg token is extracted from each appropriate request and is updated if needed, FromHeader is also not describing properly what parameters are passed and what headers to use, thus better name can be _extractRegistrationTokenFromResponse or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_extractRegistrationTokenFromResponse
_updateRegistrationTokenFromResponse. The problem here is that not clear that something happens after token extraction.
| status=200, content_type="application/json", | ||
| json={"threadId": Data.chatThreadId}) | ||
| # Guest auth: join the conversation. | ||
| responses.add(responses.POST, "{0}/threads/{1}/members".format(SkypeConnection.API_JOIN_CREATE, Data.chatThreadId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still can't understand when this both responses appear. Can you describe a bit more the user path here or in commit message?
| responses.add(responses.GET, "{0}/users/ME/conversations".format(SkypeConnection.API_MSGSHOST), | ||
| status=200, content_type="application/json", | ||
| adding_headers={"Set-RegistrationToken": "registrationToken={0}; expires={1}" | ||
| .format(Data.regToken, expiry)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just checked and I have Set-RegistrationToken header usually just on requests on this endpoint https://teams.live.com/api/chatsvc/consumer/v1/threads/48:calllogs. Are you sure that on /users/ME/conversations it can be too?
|
|
||
| @responses.activate | ||
| def testPassiveRegistrationTokenExtraction(self): | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I understand correct that this test is mainly for _extractRegistrationToken?
| def testRequiredHeaders(self): | ||
| """ | ||
| Test that required ms-ic3 headers are included in all requests. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correct this is not for all requests, but just for requests connected with /users/ME/conversations, isn't it?
| ## ❌ Need Fixes | ||
|
|
||
| ### 1. Contacts API | ||
| - **Error**: User lookup returns `None` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just checked, contacts = myteams.contacts works fine, the same as contacts['teamsid']. Can you clearlify a bit, what is not working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All notes here are related to server test expectations, in this particular case user lookup is not working, you can run testContacts server test
| ### 1. Contacts API | ||
| - **Error**: User lookup returns `None` | ||
| - **Fix**: Update contact search endpoints | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the search is indeed not working. The url have the format now like https://presence.teams.live.com/v1/pubsub/subscriptions/{x-ms-endpoint-id}. So, I think that header x-ms-endpoint-id is connected with subscriptions and it should be implemented first.
|
|
||
| ### 2. Settings API | ||
| - **Error**: DNS failure for `options.skype.com` | ||
| - **Fix**: Find new settings endpoint URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
myteams.settings works fine, but indeed even ping options.skype.com not works. And I see that it has influence just on callPrivacy and callPrivacyOpt from what myteams.settings returns. But generally, as I understand, this endpoint is not needed yet, because everything is ruled by https://account.microsoft.com/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API can be deprecated if it is not needed
| - **Fix**: Find new settings endpoint URL | ||
|
|
||
| ### 3. Translation API | ||
| - **Error**: `404 from dev.microsofttranslator.com` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the less important one :)
|
|
||
| ### 5. Chat API | ||
| - **Error**: `401/404` from conversation endpoints | ||
| - **Fix**: Check conversation ID format changes No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that everything is fine in .recent() call now. Just one problem is with raised skpy.core.SkypeRateLimitException: ('Rate limit exceeded', <Response [429]>) even with first iteration of .recent(). I think the general limitations of teams are much less now then in skype and we should take into accounting this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please describe steps to reproduce it in detail as well, do you use parallel calls, is registration token acquired and passed to the request, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Nothing unusual, reg token is available, how to test are calls parallel? Just call myteams.chats.recent(). But there are a lot of chats in my personal account. I think, that's why a lot of requests happen and raise this error.
|
|
||
| ## ❌ Need Fixes | ||
|
|
||
| ### 1. Contacts API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure, but contacts.contactIds can contain just old skype accounts, but not new. At least I haven't seen emails at first glance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check testContacts server test
| - **Fix**: Investigate new subscription method | ||
|
|
||
| ### 5. Chat API | ||
| - **Error**: `401/404` from conversation endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my_contact_chat.getMsgs() the same as .sendMsg() - I see the error 401 and the header 'StatusText': 'Invalid registration token header.'. Probably is not everything fine with current auth implementation. This tokens were on outcomming request to https://teams.live.com/api/chatsvc/consumer/v1/users/ME/conversations/8:my_contact_id/messages : {'Authentication': '',
'BehaviorOverride': 'redirectAs404',
'RegistrationToken': '',
'Sec-Fetch-Dest': 'empty',
'Sec-Fetch-Mode': 'cors',
'Sec-Fetch-Site': 'cross-site',
'X-SkypeToken': '***'}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please describe test case more in detail, what was the token state at that stage, when it is reproduced, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I am sorry, looks like that it is github formatting. There are *** in Authentication, Registrationtoken. So, the values are appeared. I see them in debuger too.
| *.py[cod] | ||
|
|
||
| # Virtual environment | ||
| .env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is not python-dotenv here. I think, should be deleted
Use SkypeToken authentication in chat API. Parse Reg Token once seen in server response.