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

Blacklist commands #56

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

Conversation

empi89
Copy link
Contributor

@empi89 empi89 commented Dec 28, 2019

  • Introduced a blacklist_commands array in the config to streamline the UI.
  • Introduced config variable DEFAULT_TEMPLATE to specify a path to a template shown as start page

@empi89
Copy link
Contributor Author

empi89 commented Jun 12, 2020

@zorun Could you please check for this PR to be merged?

@zorun
Copy link
Collaborator

zorun commented Jun 14, 2020

Here is a quick review:

  • template feature: the code looks unnecessarily complex and hard to understand. For instance, Flask has render_template(), you don't need to read the template file yourself. Also, I don't understand what the first_command and proxy are about.

  • blacklist feature: inject_commands is used to populate the template, it's not meant as a list of authorized commands. I don't have specific guidance about this, sorry, but this would need more thought/work, the current approach feels too hacky.

In both cases, you should "document" the new settings (e.g. add a commented example in the config file).

Also, please submit the two unrelated features as two different pull requests, it makes them easier to review and merge independently.

@empi89
Copy link
Contributor Author

empi89 commented Jun 14, 2020

Thanks! I have opened another PR for the template feature and described the reasoning of the approach.

For the blacklist feature: Indeed it looks a bit hacky. Would be happy to implement another approach but do not have better ideas. Could you merge this approach (I would open up a new PR for it) and if someone comes up with a better idea in the future it could be changed again?

@gmarsay
Copy link
Contributor

gmarsay commented Jun 15, 2020

Hi @empi89

I've another implementation for this feature, but my fork is too dirty for add here. Mainly because of the home page that does not currently exist and the session that needs to be initialized.

Some extracts from my modification :

lg.cfg :

ENABLED_COMMANDS = [
    "traceroute",
    "summary",
    "detail",
    "prefix",
    "prefix_detail",
    "prefix_bgpmap",
    "where",
    "where_detail",
    "where_bgpmap",
    "adv",
    "adv_bgpmap"
]

lg.py :

def inject_commands():
    available_commands = [
            ("traceroute", "traceroute ..."),
            ("summary", "show protocols"),
            ("detail", "show protocols ... all"),
            ("prefix", "show route for ..."),
            ("prefix_detail", "show route for ... all"),
            ("prefix_bgpmap", "show route for ... (bgpmap)"),
            ("where", "show route where net ~ [ ... ]"),
            ("where_detail", "show route where net ~ [ ... ] all"),
            ("where_bgpmap", "show route where net ~ [ ... ] (bgpmap)"),
            ("adv", "show route ..."),
            ("adv_bgpmap", "show route ... (bgpmap)"),
        ]
    commands = []
    enabled_commands_dict = {}
    enabled_commands = app.config["ENABLED_COMMANDS"]

    for id, text in available_commands:
        if id in enabled_commands:
            commands.append((id, text))
            enabled_commands_dict[id] = text
    
    return dict(commands=commands, commands_dict=enabled_commands_dict)

And in each controller :

def summary(hosts, proto="ipv4"):
    if "summary" not in app.config["ENABLED_COMMANDS"]:
        abort(403)

@empi89 empi89 changed the title Blacklist commands and add custom startpage support Blacklist commands Apr 5, 2022
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