Skip to content
This repository has been archived by the owner on Sep 17, 2021. It is now read-only.

Notification Settings panel not working #998

Closed
3 tasks done
johnjeffers opened this issue Mar 12, 2018 · 17 comments
Closed
3 tasks done

Notification Settings panel not working #998

johnjeffers opened this issue Mar 12, 2018 · 17 comments

Comments

@johnjeffers
Copy link

Please make sure that you have checked the boxes:

  • Review the Quickstart guide
  • Search for both open and closed issues regarding the problem you are experiencing
  • For permissions issues (Access Denied and credential related errors), please refer to the requisite docs before submitting an issue:
    AWS, GCP, OpenStack, GitHub

Description of issue:

Notification Settings panel is not allowing me to enable notifications on my account.

I select the Notify box on my account, set the Change Emails dropdown to With Issues, and click Save.

screen shot 2018-03-12 at 11 20 49 am

After clicking Save, I get the following error:

screen shot 2018-03-12 at 11 21 02 am

@mikegrima mikegrima added the bug label Mar 19, 2018
@mikegrima
Copy link
Contributor

I was able to replicate. Need to dig in more to see why.

@mikegrima
Copy link
Contributor

The UI is not sending a list but rather a string of the first account ID in the list. The Security Monkey API is expecting a list of IDs.

@mikegrima
Copy link
Contributor

I was spending some time looking into this, and there is some oddities going on in Dart.

Since we are actively trying to get rid of Dart (it's awful), we're going to put this on the backburner or fix it via the replacement.

@johnjeffers
Copy link
Author

Is there a workaround in the meantime to enable notifications? Sorry if this is a dumb question. I'm new to Security Monkey.

@devlin84
Copy link

devlin84 commented Apr 10, 2018

Hello, Is there any update regarding this? Maybe a workaround?

@mikegrima
Copy link
Contributor

Unfortunately, the bug lies somewhere in an underlying Dart package that is extremely hard to debug.

The best work around is for there to be a monkey command that does this. It's not ideal, but it would unblock you. You would have to implement this, however.

@devlin84
Copy link

I don't mind implementing it if you could give me a little more direction. Not much of a developer.

@arnitolog
Copy link

I don't think that this is a UI bug. I tried to do the same with curl and POST and got the same result.
Here is my curl without cookies and tokens:
curl -X POST -d "{'accounts': [1,2,3], 'daily_audit_email': true, 'change_report_setting': 'ISSUES'}" -H "Accept: application/json" -H "cookie: session=.....; XSRF-COOKIE=....." -H "X-CSRFToken: ....." -k --insecure 'https://localhost/api/1/settings'

And here is the result:
"message": {"accounts": "Must provide accounts"}}

I found a workaround in case you have only one account. You just need to change a type from list to str in the file /views/user_settings.py, line 151
self.reqparse.add_argument('accounts', required=True, type=str, help='Must provide accounts', location='json')

But this is definitely a dirty hack. And it will not work in case you want to receive notifications for more than one account.

@devlin84
Copy link

@arnitolog

That's a great find but, sadly I have like 9 accounts I need notifications for and I am unable to make a Monkey command to enable the notifications.

@mikegrima
Copy link
Contributor

mikegrima commented Apr 20, 2018

@arnitolog : I am going to take another look.

When I was debugging this the last time, what I saw was Dart sending over an incorrect JSON (as seen in the Chrome networking console).

I will double-check with the curl you posted above.

@mikegrima
Copy link
Contributor

mikegrima commented Apr 20, 2018

It looks like there are some issues with flask_restful reqparse (among other things).

I'm going to replace it with Marshmallow.

@arnitolog
Copy link

arnitolog commented Apr 20, 2018

yeah, I've also thought that it is something in the flask_restful. But didn't have enough evidence to mention that. Anyway, thanks Mike for your help.

@mikegrima
Copy link
Contributor

Just Submitted #1042 with a possible fix.

That should fix the API at the very least.

@arnitolog
Copy link

thanks. I will test it a little bit later

@mikegrima
Copy link
Contributor

Good news: Code wise, it now appears to be working (I don't know what issues were with Dart before... but now it's working properly) .

Once I get TravisCI to cooperate, I will merge it in.

mikegrima added a commit that referenced this issue Apr 20, 2018
Possible fix for notifications bug in #998
@mikegrima
Copy link
Contributor

mikegrima commented Apr 20, 2018

This should now be fixed in #1042 . Please make sure that you move celeryconfig.py to security_monkey/celeryconfig.py. I'm going to deploy it and see if it works in my environment.

@mikegrima
Copy link
Contributor

Confirmed that it's now fixed.

mstair added a commit to mstair/security_monkey that referenced this issue Apr 24, 2018
mikegrima added a commit that referenced this issue Apr 24, 2018
Updating docker celeryconfig.py path for #998
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants