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

Improve how we handle optional KeyCloak use in server #3143

Open
portante opened this issue Jan 4, 2023 · 0 comments
Open

Improve how we handle optional KeyCloak use in server #3143

portante opened this issue Jan 4, 2023 · 0 comments
Assignees
Labels
API Of and relating to application programming interfaces to services and functions bug Server Users Of and relating to working with users.
Milestone

Comments

@portante
Copy link
Member

portante commented Jan 4, 2023

Below is a code-review comment from PR #3081 that should be addressed going forward, but also addressed along side changes in PR #3114.


If the ops person deploying the Server configuration botches one of those four items in the "authentication" section, then the server comes up silently ignoring the OIDC...right?

That seems...unfriendly... That is, if the "authentication" section exists and there is at least one item in it, then it seems like the Server should report an error if all four are not available -- it should only continue without OIDC if there is no "authentication" section or if it exists but is empty.

E.g.,

        try:
            secret = self.server_config.get("authentication", "secret")
            client = self.server_config.get("authentication", "client")
            realm = self.server_config.get("authentication", "realm")
            issuer = self.server_config.get("authentication", "server_url")
        except NoSectionError:
            pass      # No "authentication" configuration section, skip OIDC
        except NoOptionError:
            if self.server_config.options("authentication"):
                pass  # No options in "authentication" configuration section, skip OIDC
            else:
                self.logger.exception("Bad OIDC configuration")
                abort(HTTPStatus.INTERNAL_SERVER_ERROR, message="INTERNAL ERROR")

Of course...it would be better if this all happened during Server initialization rather than when a client requests the endpoints....

[BTW, did you know that there is a configparser.ConfigParser.getint() function?? Also, there's a configparser.ConfigParser.items() function which lets you iterate over the options and their values in a given section....]

Originally posted by @webbnh in #3081 (comment)


I believe that we could do something like this instead:

            endpoints["authentication"] = self.server_config["authentication"]

I don't know whether we would want to do that, but...if we trusted (or pre-verified) our configuration, then I think that syntax is available...and, it's nice and simple....

[See suggestion below.]

Originally posted by @webbnh in #3081 (comment)


As I mentioned above, I believe we could replace this with

        expected_results = {
            "authentication": server_config["authentication"],

if we wanted to.

Originally posted by @webbnh in #3081 (comment)


Alternatively,

        req_opts = {"secret", "client", "realm", "server_url"}
        if set(self.server_config.options("authentication")) >= req_opts:
            endpoints["authentication"] = self.server_config["authentication"]

Originally posted by @webbnh in #3081 (comment)

@portante portante self-assigned this Jan 4, 2023
@portante portante added this to the v0.72 milestone Jan 4, 2023
@portante portante added bug Server API Of and relating to application programming interfaces to services and functions Users Of and relating to working with users. labels Jan 4, 2023
@portante portante modified the milestones: v0.72, v0.73 Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Of and relating to application programming interfaces to services and functions bug Server Users Of and relating to working with users.
Projects
Status: To Do
Development

No branches or pull requests

1 participant