Page MenuHomePhorge

Proxy authorization for irony/syncroton
ClosedPublic

Authored by mollekopf on Oct 28 2021, 9:10 PM.
Tags
None
Referenced Files
F12322794: D2984.id9400.diff
Fri, May 24, 7:35 AM
Unknown Object (File)
Fri, May 24, 5:39 AM
Unknown Object (File)
Sun, May 19, 1:12 AM
Unknown Object (File)
Wed, May 15, 3:49 AM
Unknown Object (File)
Tue, May 14, 8:13 PM
Unknown Object (File)
Wed, May 8, 4:14 AM
Unknown Object (File)
Wed, May 8, 4:09 AM
Unknown Object (File)
Wed, May 8, 3:13 AM
Subscribers

Diff Detail

Repository
rK kolab
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 37870
Build 15265: arc lint + arc unit

Event Timeline

mollekopf created this revision.
mollekopf added a reviewer: Restricted Project.Oct 28 2021, 9:17 PM

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

machniak subscribed.

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
88

Looking at syncroton code all requests are being authenticated.

src/app/Http/Controllers/API/V4/NGINXController.php
37–38

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

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.

docker/proxy/rootfs/etc/nginx/nginx.conf
88

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
37–38

I'll look into that.

Now with domain.tld\username normalization

machniak added inline comments.
src/app/Http/Controllers/API/V4/NGINXController.php
15–17

$e is redundant here. And I'd put @throws after @return. Also, please add @params.

132

I'd make it a single Log::debug() call, like in byebye().

138

Well, we already log the request headers above. So, this debug line looks redundant.

150

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).

189

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 added inline comments.
src/app/Http/Controllers/API/V4/NGINXController.php
138

I think it makes sense to have an explicit message given we stop processing at this point, but the message could be better indeed.

150

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.

mollekopf marked 3 inline comments as done.

Addressed comments

This revision is now accepted and ready to land.Jan 12 2022, 7:55 AM
This revision was automatically updated to reflect the committed changes.