Skip to content
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

introduce a whitelist of uids exempted from ip check #60

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lmamane
Copy link

@lmamane lmamane commented Feb 9, 2021

No description provided.

Lionel Elie Mamane added 2 commits February 9, 2021 19:41
''.split(',') returns [''], not []

Signed-off-by: Lionel Elie Mamane <[email protected]>
@lmamane
Copy link
Author

lmamane commented Feb 9, 2021

I had to bump up the nc compatibility version, else the automated test fails with "app is incompatible with this version".

I haven't (yet) added test coverage of my extension... Mostly, I haven't understood the testing framework yet. Any help on this is welcome.

@lmamane
Copy link
Author

lmamane commented Feb 9, 2021

It did a first stab at extending the testing code coverage on the "test_code_coverage" branch in my fork: https://github.com/lmamane/limit_login_to_ip/tree/test_code_coverage

The issue is I don't know how to simulate a web login in the test. The current test framework just loads the login page without actually trying to login, and tests the result of that. This cannot be useful when there are whitelisted users, because the login page must load successfully, and the login rejected only after we know for which user login is attempted.

@rakekniven rakekniven removed their request for review February 11, 2021 14:03
@jospoortvliet
Copy link
Member

@LukasReschke seems a useful contribution, even though I can't really judge the technical side, could you pls?

@nextcloud nextcloud deleted a comment from Ibrokhim0103 Mar 6, 2024
@LukasReschke LukasReschke removed their request for review March 6, 2024 19:44
@LukasReschke
Copy link
Member

@LukasReschke seems a useful contribution, even though I can't really judge the technical side, could you pls?

Not sure I am still the best to review this @jospoortvliet. But likely it won’t be mergeable anymore.

Thanks GitHub for the timely notification.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants