C2S/SASL #4101

NelsonVides · 2023-08-15T06:46:59Z

An initial refactoring of SASL out of C2S.

I chose accumulators as the SASL state because they also enable tracing trough refs and timing through their timestamps.

codecov · 2023-08-15T06:54:21Z

Codecov Report

Patch coverage: 89.65% and project coverage change: -0.01% ⚠️

Comparison is base (0133bc9) 83.92% compared to head (abb89da) 83.91%.

Additional details and impacted files

@@                Coverage Diff                @@
##           feature/sasl2    #4101      +/-   ##
=================================================
- Coverage          83.92%   83.91%   -0.01%     
=================================================
  Files                551      552       +1     
  Lines              33596    33642      +46     
=================================================
+ Hits               28195    28232      +37     
- Misses              5401     5410       +9

Files Changed	Coverage Δ
src/c2s/mongoose_c2s_stanzas.erl	`96.36% <ø> (ø)`
src/hooks/mongoose_hooks.erl	`94.92% <ø> (ø)`
src/c2s/mongoose_c2s.erl	`87.15% <83.72%> (-1.03%)`	⬇️
src/c2s/mongoose_c2s_sasl.erl	`95.45% <95.45%> (ø)`

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chrzaszcz

It's nice to see how the SASL parts are being extracted. I added some comments.

chrzaszcz · 2023-08-21T13:59:46Z

src/c2s/mongoose_c2s.erl

+
+-spec handle_sasl_abort(data(), mongoose_acc:t(), retries()) -> fsm_res().
+handle_sasl_abort(StateData, SaslAcc, Retries) ->
+    Error = #{server_out => <<"aborted">>, maybe_username => StateData#c2s_data.jid},


Why do we call JID "username"?

chrzaszcz · 2023-08-21T14:00:16Z

src/c2s/mongoose_c2s.erl

+handle_sasl_failure(#c2s_data{host_type = HostType, lserver = LServer} = StateData, SaslAcc,
+                    #{server_out := ServerOut, maybe_username := Username}, Retries) ->
+    ?LOG_INFO(#{what => auth_failed, text => <<"Failed SASL authentication">>,
+                jid => Username, c2s_state => StateData}),


So is it a JID or user name? I am confused.

chrzaszcz · 2023-08-21T14:10:08Z

src/c2s/mongoose_c2s_sasl.erl

+handle_sasl_failure(C2SData, Error, Username, _, SaslAcc) ->
+    LServer = mongoose_c2s:get_lserver(C2SData),
+    NewJid = jid:make_bare(Username, LServer),
+    {failure, SaslAcc, #{server_out => Error, maybe_username => NewJid}}.


Really strange - the code keeps jumping between JID and username - like here between lines 96 and 100.

chrzaszcz · 2023-08-21T14:17:57Z

src/c2s/mongoose_c2s_sasl.erl

+        mongoose_c2s:data(), term(), maybe_username(), maybe_username(), mongoose_acc:t()) -> result().
+handle_sasl_failure(_C2SData, Error, undefined, undefined, SaslAcc) ->
+    {failure, SaslAcc, #{server_out => Error, maybe_username => undefined}};
+handle_sasl_failure(_C2SData, Error, undefined, #jid{luser = Username}, SaslAcc) ->


This is for sure against the spec (double username). Is it reachable?

Indeed it is the only line of code not covered by tests in this module, now it will have 100% coverage, good catch! 😛

chrzaszcz

I added minor comments.

chrzaszcz · 2023-08-24T10:03:03Z

src/c2s/mongoose_c2s_sasl.erl

+    handle_sasl_failure(C2SData, Error, Username, undefined, SaslAcc);
+handle_sasl_step(C2SData, {error, Error}, SaslAcc) ->
+    Jid = mongoose_c2s:get_jid(C2SData),
+    handle_sasl_failure(C2SData, Error, undefined, Jid, SaslAcc).


Same as before, passing Jid while the type is maybe_username().

chrzaszcz · 2023-08-24T10:07:53Z

big_tests/tests/mim_c2s_SUITE.erl

@@ -119,6 +121,16 @@ message_sent_to_malformed_jid_results_in_error(Config) ->
        escalus_assert:is_chat_message(<<"Hi!">>, escalus_client:wait_for_stanza(Bob))
    end).

+invalid_resource_fails_to_log(Config) ->


Wasn't this already done in a merged PR? Maybe rebase?

I think I did and yet dunno why this shows up here, just rebased again.

mongoose-im · 2023-08-24T10:27:19Z

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / abb89da
Reports root/ big
OK: 369 / Failed: 0 / User-skipped: 38 / Auto-skipped: 0

small_tests_24 / small_tests / abb89da
Reports root / small

small_tests_25 / small_tests / abb89da
Reports root / small

small_tests_25_arm64 / small_tests / abb89da
Reports root / small

ldap_mnesia_24 / ldap_mnesia / abb89da
Reports root/ big
OK: 2275 / Failed: 0 / User-skipped: 831 / Auto-skipped: 0

dynamic_domains_mysql_redis_25 / mysql_redis / abb89da
Reports root/ big
OK: 4202 / Failed: 0 / User-skipped: 116 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / abb89da
Reports root/ big
OK: 4234 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0

ldap_mnesia_25 / ldap_mnesia / abb89da
Reports root/ big
OK: 2275 / Failed: 0 / User-skipped: 831 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / abb89da
Reports root/ big
OK: 4234 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0

internal_mnesia_25 / internal_mnesia / abb89da
Reports root/ big
OK: 2421 / Failed: 0 / User-skipped: 685 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / abb89da
Reports root/ big
OK: 4231 / Failed: 0 / User-skipped: 87 / Auto-skipped: 0

mysql_redis_25 / mysql_redis / abb89da
Reports root/ big
OK: 4603 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / abb89da
Reports root/ big
OK: 4617 / Failed: 1 / User-skipped: 91 / Auto-skipped: 5

bosh_SUITE:essential_https:accept_higher_hold_value

{error,
  {{assertEqual,
     [{module,bosh_SUITE},
      {line,260},
      {expression,"get_bosh_sessions ( )"},
      {expected,[]},
      {value,
        [{bosh_session,<<"5609d62ed9b946b9d86c3c2d98b7bc0a87484cbc">>,
           <8854.10235.0>}]}]},
   [{bosh_SUITE,accept_higher_hold_value,1,
      [{file,"/home/circleci/project/big_tests/tests/bosh_SUITE.erl"},
       {line,260}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1292}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1224}]}]}}

Report log

pgsql_cets_25 / pgsql_cets / abb89da
Reports root/ big
OK: 4593 / Failed: 0 / User-skipped: 121 / Auto-skipped: 0

pgsql_mnesia_25 / pgsql_mnesia / abb89da
Reports root/ big
OK: 4623 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / abb89da
Reports root/ big
OK: 4620 / Failed: 0 / User-skipped: 94 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / abb89da
Reports root/ big
OK: 4623 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0

chrzaszcz

Looks good 👌

This comment was marked as outdated.

Sign in to view

NelsonVides force-pushed the c2s/sasl branch from cf71561 to 7299e6d Compare August 21, 2023 07:20

NelsonVides mentioned this pull request Aug 21, 2023

Xeps/extensible sasl #4102

Merged

This comment was marked as outdated.

Sign in to view

NelsonVides marked this pull request as ready for review August 21, 2023 07:57

chrzaszcz reviewed Aug 21, 2023

View reviewed changes

NelsonVides force-pushed the c2s/sasl branch from 7299e6d to 421457f Compare August 23, 2023 17:54

This comment was marked as outdated.

Sign in to view

chrzaszcz reviewed Aug 24, 2023

View reviewed changes

NelsonVides added 3 commits August 24, 2023 12:18

Ensure right types for sasl stanzas

e61db73

Extract SASL from C2S

bc8695c

Apply review: consistently use username and not jid

abb89da

NelsonVides force-pushed the c2s/sasl branch from 421457f to abb89da Compare August 24, 2023 10:19

NelsonVides requested a review from chrzaszcz August 24, 2023 11:09

chrzaszcz approved these changes Aug 24, 2023

View reviewed changes

chrzaszcz merged commit 389560e into feature/sasl2 Aug 24, 2023
4 checks passed

chrzaszcz deleted the c2s/sasl branch August 24, 2023 11:40

chrzaszcz added this to the 6.2.0 milestone Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C2S/SASL #4101

C2S/SASL #4101

NelsonVides commented Aug 15, 2023

This comment was marked as outdated.

codecov bot commented Aug 15, 2023 •

edited

This comment was marked as outdated.

chrzaszcz left a comment

chrzaszcz Aug 21, 2023

chrzaszcz Aug 21, 2023

chrzaszcz Aug 21, 2023

chrzaszcz Aug 21, 2023

NelsonVides Aug 23, 2023

This comment was marked as outdated.

chrzaszcz left a comment

chrzaszcz Aug 24, 2023

chrzaszcz Aug 24, 2023

NelsonVides Aug 24, 2023

mongoose-im commented Aug 24, 2023 •

edited

chrzaszcz left a comment

C2S/SASL #4101

C2S/SASL #4101

Conversation

NelsonVides commented Aug 15, 2023

This comment was marked as outdated.

codecov bot commented Aug 15, 2023 • edited

Codecov Report

This comment was marked as outdated.

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mongoose-im commented Aug 24, 2023 • edited

chrzaszcz left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2023 •

edited

mongoose-im commented Aug 24, 2023 •

edited