Some small tweaks and "fixes" to halauth by richteer · Pull Request #115 · Halibot/halibot

richteer · 2018-10-01T21:46:46Z

~~Pretty straightforward PR, nothing invasive this time.~~

~~1898759 returns a boolean based on if .grant or .revoke actually did something to the internal permissions list -- largely as an indicator to the caller if they should bother calling .write~~
fe3cfc2 changes the behavior of the @hasPermission decorator slightly, to no longer require the Message object it parses ri and identity from as a positional argument. Best to look at the code/commit message for more info.

Haha jk wasn't straightforward at all. See comments for what happened.

coveralls · 2018-10-01T21:49:38Z

Coverage decreased (-0.2%) to 92.813% when pulling f69d3bc on perm-fixes into fb54463 on master.

richteer · 2018-10-01T22:12:24Z

Ah coveralls, you bone in my brisket. I'll post an update to this branch with an update to tests that checks that error case that should never be hit, before merge. Since the other PR that uses this branch is also complaining, might as well wait before this gets merged to fix both.

Nevermind, fixed it probably.

sjrct · 2019-02-06T05:07:57Z

 	def grantPermission(self, ri, identity, perm):
 		if not self.enabled:
-			return
+			return False


We should be raising exceptions instead of the return successful paradigm.

Oh, yeah I suppose this would be the case for exceptions here, wouldn't it. Is there an existing exception class I should use here, or should I start inventing my own?

Be creative!

I'm having second thoughts on this, but for a different reason. Should we allow permissions to be edited when they are not being enforced? Without, there may be a chicken-and-egg problem where you enable permission enforcement, but no one is permitted to modify the permissions table.

Proposal: split enabled and enforced. Setting enabled = True simply enables access to the permissions table internally. Setting enforced = True implies enabled = True, and is checked in hasPermission calls.

Therefore, calls to grantPermission when not enabled should be an Exception because you cannot grant a permission when auth is not enabled at all. True or False still apply for if the permission was added, or already existed.

Note, enabled = True and enforced = False is like selinux "permissive" mode. We should still log failed auth checks. Side note, thinking about adding a "seperate" auth log that logs checks, rejections, etc. May not actually be a separate physical log, but something easy to grep for.

sjrct · 2019-02-06T05:09:03Z

+		def wrapper(self, *args, **kwargs):
+			msg = None
+			for i in list(args) + list(kwargs.values()):
+				if i.__class__ == Message:


Not very pythonic, what about subclasses of Message?

Also, what should we do in the cases with multiple message arguments? I don't think just taking the first one that appears is the most transparent behavior. Maybe an argument to the decorator to specify which to use?

Agh, that should be a subclass of Message check not a equality check. I blame hacktoberfest-rush @richteer on that laziness.

As for multiple message arguments: The previous convention stated that the message argument should be first, and I agree that should be the case now as well. That said, that means this wrapper doesn't get any easier to use if you still have to uphold the convention, so convenience is lost I suppose.

Alternative suggestion: Have two different wrappers:

One that requires a Message object as the first parameter

One that requires exactly one Message object in *args

...now that I write that, it feels like it just adds needless complexity.

Maybe for simplicity we just have one wrapper with the general use case, and when people need to do fancy things, like pass multiple messages around, they just have to call hasPermission themself?

Probably the better choice. I'm leaning towards requiring the message to be in **kwargs, as msg, since that is what CommandModule uses as convention.

I'm fine with that. More clear than first positional.

richteer · 2019-03-05T15:08:25Z

Update: Instead of crawling **kwargs for a fixed named variable, instead the decorator can now be passed optional parameters to specify which argument should be used as the message object.

New decorator args are argnum=-1 and key="msg". The first specifies the index of a positional argument to use. The key parameter specifies what key to use in **kwargs, if it exists. Positional arguments take precedence over key, if both are supplied. The default key is "msg", which should be mostly compatible with existing CommandModule commands.

sjrct · 2019-03-05T18:15:32Z

-				if i.__class__ == Message:
-					msg = i
-					break
+			if argnum >= 0 and argnum < len(args):


I think you should check for None here. A negative argnum makes sense, for example if you want to refer to the last position argument with -1. Also, None is more semantically significant of a value to not be used.

...yep, that makes a lot more sense. I've been writing too much C lately. Default argnum should be None, and I will explicitly check against None to see if it has been set.

sjrct · 2019-03-05T18:16:53Z

+				msg = kwargs.get(key)
+
+			if not msg:
 				self.log.error("Probable module bug! -- hasPermission decorator called on a function that doesn't have a Message argument!")


Not necessarily true: it could have successfully retrieved a message from the kwargs that was set to a false value. Either change the log or make the check if not key in kwargs.

What about if not (hasattr(msg, "origin") and hasattr(msg, "identity")) ? That's all we are really looking to extract from the message object anyway, less chance for false positives.

What about just catching AttributeErrors for those when we try to use them?

I guess that surfaces less to the module. Erg. Yeah, we should probably just check for those attrs.

Actually, I just added a MalformedMsgException in #131 , maybe we should utilize that here too? If attr check fails raise that complaining about the bad attr.

Thinking I might add that definition to base halibot, and expand it to properly complain about the specific problem attrs.

Seems reasonable. Should we throw that where we use those attrs then? It would keep that which enforces constraints and the reason for them together.

We probably should, but not as a .__getattr__() action. There's always a possibility that we are handed a non-Message object, so we should probably check those attrs where it is critical to do so. That is to say, .raw_send() should, and auth checks should.

Something I just thought of here, should we handle the case of key not in kwargs.keys() separately from kwargs[key] = None/Badobject?

sjrct · 2019-03-05T18:17:54Z

I like this approach much better though.

richteer · 2019-03-06T00:10:45Z

Alright, complete U-turn on this. I had some fun rebasing this to a more recent branch, since all the regex stuff for RI matching was merged before this. I nuked the "return True/False" stuff entirely. Since we should probably be returning exceptions anyway, figured it'd probably be best to completely change that behavior anyway.

So, what is in this branch now? Really only the decorator change as discussed in the threads above. That was largely the only chunk worth salvaging for now. I archived the old branch to perm-fixes-old, so we can retrieve the changes I from there later if we must.

Minor update: squashed patches down into one, since there's really only one rework now.

sjrct · 2019-03-06T17:45:52Z

+			if argnum != None and argnum < len(args):
+				msg = args[argnum]
+			else:
+				msg = kwargs.get(key)


Maybe this should be kwargs[key]? If they provide the wrong key, it is probably better to point that out directly than to get a MalformedMsgException.

Yeah, I think that resolves my question in the other thread. We should propagate the KeyError upwards.

richteer · 2019-03-06T18:00:50Z

Update: changed kwargs.get(key) to kwargs[key], as we should propagate a KeyError if the module dev specified the wrong key= in **kwargs to use

… as expected

@hasPermission

The `@hasPermission` decorator was intended to be simple to use for module developers. As it turns out however, it is completely incompatible with modules that subclass CommandModule. Considering this is also a class intended to make life easier, something had to be fixed. Thus, the decorator now supports two optional parameters when applying to a function: "argnum" and "key". Since we need to extract the source origin and identity at runtime, but different modules may have different calling conventions, this was the easiest and most flexible method to instruct the decorator where to look for its information. argnum can be supplied to specify which positional argument contains a Message object, indexing by 0. Remember, class methods do not count self in *args!. The default value is None, so unless supplied, this parameter is ignored. key can be supplied to specify a key for retrieveing the Message object from **kwargs. The default value is "msg", which is the default convention for classes that inherit from CommandModule. If argnum is specified (i.e. not None), it takes priority over key. Example: class MyModule(HalModule): # Uses first non-self positional argument @hasPermission("FOO", argnum=0) def myfunc(self, msg): pass # key= is not necessary here, since "msg" is the default # make sure msg= is supplied! @hasPermission("BAR") def mybar(self, msg=None): pass

richteer requested a review from sjrct October 1, 2018 21:46

richteer mentioned this pull request Oct 1, 2018

Implement a PermissionManager core module #116

Open

richteer added this to the v0.3.0 milestone Oct 1, 2018

sjrct requested changes Feb 6, 2019

View reviewed changes

sjrct requested changes Mar 5, 2019

View reviewed changes

richteer mentioned this pull request Mar 5, 2019

Add MalformedMsgException to message.py for general error use #136

Merged

richteer force-pushed the perm-fixes branch from 512fc88 to ced406b Compare March 6, 2019 00:05

richteer requested a review from sjrct March 6, 2019 00:08

richteer force-pushed the perm-fixes branch from ced406b to a909398 Compare March 6, 2019 00:21

sjrct approved these changes Mar 6, 2019

View reviewed changes

richteer force-pushed the perm-fixes branch from a909398 to 3721dc4 Compare March 6, 2019 17:59

richteer added 2 commits March 8, 2019 14:15

message: add exception to throw when message has not been initialized…

b76ed82

… as expected

richteer force-pushed the perm-fixes branch from 3721dc4 to c162a54 Compare March 8, 2019 20:15

CI: update test_auth for changes to halauth

f69d3bc

richteer force-pushed the perm-fixes branch from c162a54 to f69d3bc Compare March 8, 2019 21:08

richteer merged commit a7bdf35 into master Mar 11, 2019

Uh oh!

Conversation

richteer commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richteer commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richteer Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richteer commented Mar 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richteer Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richteer Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrct commented Mar 5, 2019

Uh oh!

richteer commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richteer commented Mar 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richteer commented Oct 1, 2018 •

edited

Loading

coveralls commented Oct 1, 2018 •

edited

Loading

richteer commented Oct 1, 2018 •

edited

Loading

richteer Feb 6, 2019 •

edited

Loading

richteer Mar 5, 2019 •

edited

Loading

richteer Mar 5, 2019 •

edited

Loading

richteer commented Mar 6, 2019 •

edited

Loading