-
Notifications
You must be signed in to change notification settings - Fork 427
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
Refactor inband registration (and cancelation) #559
Conversation
357c96c
to
848cccf
Compare
#xmlel{name = <<"password">>} | ||
| QuerySubels]}]} | ||
%% Clients must register before being able to authenticate. | ||
process_unauthenticated_iq(Sender, Receiver, #iq{type = set} = Stanza, IPAddr) -> |
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'd stick to one standard - either use From/To or Sender/Receiver, not both alternately. The same story with IQ/Stanza. For me it was slightly misleading when I was comparing process_unauthenticated_iq
to process_iq
.
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.
Okay, changed both: commit 1d811e7.
One note from me. Maybe it's good time to move all test related to registration to a separate SUITE? |
@michalwski Yeah, sure, I'll make the changes Paweł commented on. Then I'll move them over to their own suite. |
2c9b62a
to
3c2df10
Compare
error_response(IQ, ?ERR_FORBIDDEN) | ||
end. | ||
|
||
attempt_cancelation(ClientJID, #jid{lserver = ServerDomain}, #iq{id = ID, sub_el = Child} = IQ) -> |
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.
cancelation -> cancellation
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, sorry, the dictionary adds that US also cancelation :) My mistake.
6d9df55
to
161d38b
Compare
process_iq_get(From, To, IQ, IPAddr). | ||
|
||
process_iq(From, To, #iq{type = set} = IQ) -> | ||
process_iq_set(From, To, IQ, jlib:jid_tolower(From)); |
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 take a look at jid
module and change all the deprecated functions from jlib
.
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.
Commit 2f1765b fixes these.
How about |
Another test would be for the |
Move necessary agruments out of the "extras".
That is: `Sender'/`Receiver' ---> `From'/`To', and `Stanza' ---> `IQ'.
Also rename the procedure that helps to switch on the children of the "query" element and use more informative names for the atoms which are returned.
When someone has authenticated, i.e. they are already registered, Mongoose should respond with a `registered' IQ query element.
Matches will succeed when changing password: we were comparing the sender of a stanza to a JID that hadn't been formatted according to XMPP guidelines.
NOTE: This test fails until a fix to Mongoose is made.
Since the password change is an IQ request, even though XEP 0077 does not describe this, we test for an error response.
The procedure in question had an empty string (i.e. "") as a pattern in its head, but it's meant to be a binary (i.e. <<"">>), so add this clause.
The calls made inside `*_per_testcase(null_password, _)' simply shouldn't be necessary to test for an error response when a client tries to register with an null password.
We were matching only on an empty string when we really need to match on an empty binary.
Test for a <resource-constraint/> error-response after making inband registration requests in quick succession NOT just that we receive an error-response. The second paragraph of section 3.1.1 of XEP 0077 may suggest using a <not-acceptable/>, see comment in test for more detail.
Mongoose should restrict the number of registrations, regardles of a client's recent success or failure, within a time period.
This makes it more clear that the timers are prerequisites for both tests.
An "error" stanza will in fact produce a "failed_to_register", not a "bad_response".
This concerns changes to the `{access, register, global}' variable, used for inband registration.
2f1765b
to
e9d5dea
Compare
Assert timeouts have expired before much of the registration process begins: this'll make sure that registration requests are constrained within a given time period, regardless of a client's recent registration success or failure. It discriminates clients using their IP address.
We use the Erlang binary internally.
e9d5dea
to
d754769
Compare
Refactor inband registration (and cancelation)
This began as a task to increase coverage, then became about making Mongoose conform to XEP 0077, but to do this we need to refactor.