Remove redundant has_access definition in superset #4689

timifasubaa · 2018-03-25T11:27:13Z

This is a follow-up PR to #4565. It removes the has_access from superset since it is defined in FAB here.
The only difference is the next parameter in the url_for function. I have tested it and I doubt it will cause any issues.

@john-bodley @mistercrunch

codecov-io · 2018-03-28T00:51:55Z

Codecov Report

Merging #4689 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4689      +/-   ##
==========================================
- Coverage   71.87%   71.85%   -0.03%     
==========================================
  Files         204      204              
  Lines       15323    15308      -15     
  Branches     1177     1177              
==========================================
- Hits        11014    11000      -14     
+ Misses       4306     4305       -1     
  Partials        3        3

Impacted Files	Coverage Δ
superset/connectors/sqla/views.py	`71.56% <100%> (ø)`	⬆️
superset/utils.py	`88.05% <100%> (-0.17%)`	⬇️
superset/connectors/druid/views.py	`68.02% <100%> (ø)`	⬆️
superset/views/core.py	`71.17% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f952ec2...4cd17c1. Read the comment docs.

mistercrunch · 2018-03-29T00:56:30Z

superset/security.py

@@ -92,6 +99,42 @@ def can_access(self, permission_name, view_name, user=None):
            return self.is_item_public(permission_name, view_name)
        return self._has_view_access(user, permission_name, view_name)

+    def has_method_access(self, f):


It might just be me but I don't like the idea of a decorator as a class method. It's probably ok though. I guess decorators are already complex in terms of what's inside them, external context make it more of a stretch.

mistercrunch · 2018-03-29T00:57:04Z

superset/security.py

+            return redirect(
+                url_for(
+                    security_manager.auth_view.__class__.__name__ + '.login',
+                    next=request.full_path))


Looked it up and the only difference from the orignal is this one line

I checked here (http://flask-appbuilder.readthedocs.io/en/latest/_modules/flask_appbuilder/security/manager.html) and all the has_access methods are pretty different from what is in this method.

Ohhh. my bad. Here it is (http://flask-appbuilder.readthedocs.io/en/latest/_modules/flask_appbuilder/security/decorators.html#has_access)

I will test things out using the default one and if that's fine, I will change this PR to removing the method

mistercrunch · 2018-03-29T00:58:40Z

superset/security.py

+            permission_str = f.__name__
+
+        def wraps(self, *args, **kwargs):
+            from superset import security_manager


I guess I'm making an argument against my previous comment, but isn't self == security_manager here?

mistercrunch · 2018-03-29T01:00:39Z

superset/security.py

+            from superset import security_manager
+            permission_str = PERMISSION_PREFIX + f._permission_name
+            if security_manager.has_access(permission_str,
+                                             self.__class__.__name__):


Actually this is fairly confusing as there are two layers of self here. self here points to the SupersetView class, and the one in the frame above it points to superset.security_manager

timifasubaa

I changed this PR to remove the has_access function defined in superset.
I'm now concerned if the next=?? line could potentially cause an error in edge cases I'm not thinking about. But all tests pass and I have accessed some of the endpoints wrapped by has_access successfully.

timifasubaa · 2018-03-29T01:15:19Z

superset/security.py

+            return redirect(
+                url_for(
+                    security_manager.auth_view.__class__.__name__ + '.login',
+                    next=request.full_path))


I checked here (http://flask-appbuilder.readthedocs.io/en/latest/_modules/flask_appbuilder/security/manager.html) and all the has_access methods are pretty different from what is in this method.

timifasubaa · 2018-03-29T01:17:33Z

superset/security.py

+            return redirect(
+                url_for(
+                    security_manager.auth_view.__class__.__name__ + '.login',
+                    next=request.full_path))


Ohhh. my bad. Here it is (http://flask-appbuilder.readthedocs.io/en/latest/_modules/flask_appbuilder/security/decorators.html#has_access)

timifasubaa · 2018-03-29T01:23:07Z

superset/security.py

+            return redirect(
+                url_for(
+                    security_manager.auth_view.__class__.__name__ + '.login',
+                    next=request.full_path))


I will test things out using the default one and if that's fine, I will change this PR to removing the method

fabianmenges

Nice, some cleanup. This should work for us.

john-bodley · 2018-03-30T00:54:08Z

I'm all for code cleanup/refactoring.

* update has_access to has_method_access * move has_access to sm and rename to has_method_access

update has_access to has_method_access

e29421f

timifasubaa force-pushed the move_has_access_to_superset_security_manager branch 4 times, most recently from 5dc0031 to ca3b0d0 Compare March 28, 2018 00:33

timifasubaa changed the title ~~[WiP] Move has access to superset security manager~~ Move has access to superset security manager Mar 28, 2018

timifasubaa force-pushed the move_has_access_to_superset_security_manager branch from ca3b0d0 to 48a95c4 Compare March 28, 2018 22:52

mistercrunch reviewed Mar 29, 2018

View reviewed changes

timifasubaa force-pushed the move_has_access_to_superset_security_manager branch 2 times, most recently from 59fd375 to 5c8b498 Compare March 29, 2018 06:27

move has_access to sm and rename to has_method_access

4cd17c1

timifasubaa force-pushed the move_has_access_to_superset_security_manager branch from 5c8b498 to 4cd17c1 Compare March 29, 2018 18:46

timifasubaa commented Mar 29, 2018

View reviewed changes

timifasubaa changed the title ~~Move has access to superset security manager~~ Remove redundant has_access definition in superset Mar 29, 2018

fabianmenges approved these changes Mar 30, 2018

View reviewed changes

john-bodley merged commit e25535c into apache:master Mar 30, 2018

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018

Remove redundant has_access definition in superset (apache#4689)

fcbc964

* update has_access to has_method_access * move has_access to sm and rename to has_method_access

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Remove redundant has_access definition in superset (apache#4689)

4a3ff25

* update has_access to has_method_access * move has_access to sm and rename to has_method_access

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant has_access definition in superset #4689

Remove redundant has_access definition in superset #4689

timifasubaa commented Mar 25, 2018 •

edited by john-bodley

Loading

codecov-io commented Mar 28, 2018 •

edited

Loading

mistercrunch Mar 29, 2018

mistercrunch Mar 29, 2018

timifasubaa Mar 29, 2018

timifasubaa Mar 29, 2018

timifasubaa Mar 29, 2018

mistercrunch Mar 29, 2018

mistercrunch Mar 29, 2018

timifasubaa left a comment

timifasubaa Mar 29, 2018

timifasubaa Mar 29, 2018

timifasubaa Mar 29, 2018

fabianmenges left a comment

john-bodley commented Mar 30, 2018

Remove redundant has_access definition in superset #4689

Remove redundant has_access definition in superset #4689

Conversation

timifasubaa commented Mar 25, 2018 • edited by john-bodley Loading

codecov-io commented Mar 28, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

timifasubaa 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

fabianmenges left a comment

Choose a reason for hiding this comment

john-bodley commented Mar 30, 2018

timifasubaa commented Mar 25, 2018 •

edited by john-bodley

Loading

codecov-io commented Mar 28, 2018 •

edited

Loading