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

When either data_profiler_filter or superuser_filter aren't defined,… #930

Merged
merged 3 commits into from
Feb 12, 2016

Conversation

mtagle
Copy link
Contributor

@mtagle mtagle commented Jan 29, 2016

… don't crash, and set user to have data_profiler or superuser access.

At present, if data_profiler_filter or superuser_filter aren't defined, dataprofiler and superuser aren't set in LDAPUser's init, resulting an an AttributeError when accessors are called.

@mistercrunch
Copy link
Member

@bolkedebruin

@criccomini
Copy link
Contributor

@bolkedebruin btw, without this patch, LDAP on master won't work unless dataprofiler/superuser filters are set (i.e. master is broken for default use case right now).

@bolkedebruin
Copy link
Contributor

Ok. Will have a look at it towards the end of this week

@criccomini
Copy link
Contributor

Gentle nudge here. :)

@bolkedebruin
Copy link
Contributor

@criccomini In general the patch looks solid! I would like to see the tests also cover the use of data_profiler and superuser. This would require to add it to the ldap provisioning for a specific user.

After that please squash your commits in some meaningful ones.

@mtagle
Copy link
Contributor Author

mtagle commented Feb 8, 2016

@bolkedebruin can you elaborate a bit on what you mean by "tests covering the use of data_profiler and superuser"?

@criccomini
Copy link
Contributor

@mtagle I think he means that he wants a test that adds several users to the LDAP test. One would be a user with just basic access (no data profiler/super user), one with just data profiler, and one with just super user (and potentially one with both). Then verify that the data_profiler() and superuser() methods return True/False as expected.

@bolkedebruin
Copy link
Contributor

Correct. The provisioning can be done by the files no need to encapsulate it in the test itself.

…don't crash, and set user to have data_profiler or superuser access.
@mtagle mtagle force-pushed the no_ldap_filter_fix branch 4 times, most recently from b2b64cf to 0c5b390 Compare February 9, 2016 23:23
1. make sure that if no superuser or dataprofiler filter is set, all logged in users get superuser or dataprofiler privileges.
2. make sure that, if filters are set, users get acess when the filters allow it.

change ldif loading so that it always loads in a reasonable order.
@mtagle
Copy link
Contributor Author

mtagle commented Feb 9, 2016

@bolkedebruin could I get a further review/merge?

@mtagle
Copy link
Contributor Author

mtagle commented Feb 10, 2016

ping @mistercrunch

@criccomini
Copy link
Contributor

@mistercrunch I think this is ready to be committed.

@mistercrunch
Copy link
Member

👍

mistercrunch added a commit that referenced this pull request Feb 12, 2016
When either data_profiler_filter or superuser_filter aren't defined,…
@mistercrunch mistercrunch merged commit bdbe121 into apache:master Feb 12, 2016
mobuchowski pushed a commit to mobuchowski/airflow that referenced this pull request Jan 4, 2022
Signed-off-by: henneberger <git@danielhenneberger.com>

Co-authored-by: Willy Lulciuc <willy@datakin.com>
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.

4 participants