Skip to content

Improve how we handle optional KeyCloak use in server #3143

Open
@portante

Description

@portante

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)

Metadata

Metadata

Assignees

Labels

APIOf and relating to application programming interfaces to services and functionsServerUsersOf and relating to working with users.bug

Type

No type

Projects

Status

To Do

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions