Details

Reviewers

Group Reviewers

Restricted Project

Commits

Diff Detail

Repository

rK kolab

Branch

dev/mollekopf

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 36956
Build 14723: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Oct 28 2021, 9:10 PM

mollekopf created this revision.

Harbormaster completed remote builds in B36953: Diff 8519.Oct 28 2021, 9:10 PM

Cleanup

Harbormaster completed remote builds in B36956: Diff 8525.Oct 28 2021, 9:17 PM

Please note this builds on some other patches for the docker related stuff.

Aren't we authenticate the user two times this way? Once in nginx and once in syncroton/iRony? Should we have an option in syncroton/iRony to disable authentication?

docker/proxy/rootfs/etc/nginx/nginx.conf
112	Looking at syncroton code all requests are being authenticated.
src/app/Http/Controllers/API/V4/NGINXController.php
33	For activesync we have to support user\domain syntax. https://git.kolab.org/diffusion/S/browse/master/lib/kolab_sync.php$123

This revision now requires changes to proceed.Oct 29 2021, 10:06 AM

In D2984#35477, @machniak wrote:

Aren't we authenticate the user two times this way? Once in nginx and once in syncroton/iRony? Should we have an option in syncroton/iRony to disable authentication?

Yes we are authenticating twice. I don't think we should change that (just like we don't for imap).

Ultimately we should plug in our 2fa capable authentication in saslauthd, at which point we no longer require this nginx variant, and in the meantime I don't think we need to introduce further options for what doesn't seem really problematic anyways.

mollekopf added inline comments.Oct 29 2021, 5:34 PM

docker/proxy/rootfs/etc/nginx/nginx.conf
112	I think outlook on android tried to do some unauthenticated stuff, but as you say, syncroton doesn't support that anyways, so I guess that should be fine.
src/app/Http/Controllers/API/V4/NGINXController.php
33	I'll look into that.

Now with domain.tld\username normalization

Harbormaster completed remote builds in B37073: Diff 8615.Nov 15 2021, 10:04 AM

Rebased

Harbormaster completed remote builds in B37723: Diff 9157.Jan 7 2022, 4:50 PM

machniak requested changes to this revision.Jan 10 2022, 9:48 AM

machniak added inline comments.

src/app/Http/Controllers/API/V4/NGINXController.php
13	`$e` is redundant here. And I'd put @throws after @return. Also, please add @params.
109	I'd make it a single Log::debug() call, like in byebye().
115	Well, we already log the request headers above. So, this debug line looks redundant.
127	I'm not sure, but I'm wondering whether for cases like this (and 401 above) we should use byebye() method, of course extended to handle all cases, but it would be some unification on the code level (debug logging).
166	I'd make it a single Log::debug() call, like in byebye().
src/tests/Feature/Controller/NGINXTest.php
229	I'd rather use ned@kolab.org who has 2FA configured already. It will not require to unset the setting for john afterwards.

This revision now requires changes to proceed.Jan 10 2022, 9:48 AM

mollekopf marked 8 inline comments as done.Jan 11 2022, 8:53 PM

mollekopf added inline comments.

src/app/Http/Controllers/API/V4/NGINXController.php
115	I think it makes sense to have an explicit message given we stop processing at this point, but the message could be better indeed.
127	I don't think that would help at the moment. The byebye method only works for the imap/smtp responses which uses headers, and the httpauth module uses the http status code instead. While we could introduce a similar method to combine status code + debug message, it doesn't seem to really improve the situation to me (two lines of code vs 1 line + a function).
src/tests/Feature/Controller/NGINXTest.php
229	Personally I think using the same user is clearer, as this makes explicit that we simply test the same situation with a different setting. I don't particularly mind changing to ned@kolab.org, but note that I can't currently remove resetting the settings, since we also change other settings and I'm resetting in tearDown. So unless it's really worth it to avoid setting the settings one to many times, I'd leave it as is.