Feature/25 add jwt config #35

lchen-2101 · 2023-09-22T14:13:10Z

closes #25

github-actions · 2023-09-22T14:18:42Z

Coverage report

The coverage rate went from 83.91% to 84.93% ⬆️
The branch rate is 85%.

97.72% of new lines are covered.

Diff Coverage details (click to unfold)

src/config.py

100% of new lines are covered (97.29% of the complete file).

src/entities/engine/engine.py

100% of new lines are covered (100% of the complete file).

src/entities/models/dto.py

100% of new lines are covered (100% of the complete file).

src/main.py

100% of new lines are covered (93.1% of the complete file).

src/oauth2/oauth2_admin.py

50% of new lines are covered (33.33% of the complete file).
Missing lines: 41

hkeeler · 2023-09-28T00:17:18Z

src/.env.local

+INST_CONN=postgresql+asyncpg://${INST_DB_USER}:${INST_DB_PWD}@${INST_DB_HOST}/${INST_DB_NAME}
+JWT_OPTS=verify_at_hash:False,verify_aud:False,verify_iss:False


I feel like these should be parameters we can tune individually, like:

JWT_OPTS_VERIFY_AT_HASH

JWT_OPTS_VERIFY_AUD

JWT_OPTS_ISS

agreed, just thought of getting env vars by prefix, so will be implementing that approach.

hkeeler · 2023-09-28T01:15:08Z

src/oauth2/oauth2_admin.py

+        pairs = opts_string.split(",")
+        for pair in pairs:
+            [key, value] = pair.split(":", 1)
+            jwt_opts[key] = ast.literal_eval(value)


Since we already have Pydantic in place, we might as well use their much friendlier boolean parsing.

Suggested change

jwt_opts[key] = ast.literal_eval(value)

pydantic.parse_obj_as(bool, envvar_name)

Also, whether we use pydantic or the ast route, we should wrap this in a try/except and raise a friendly error.

will try/except, I'm thinking app should fail to start in this instance considering the configuration is vital to everything else.

also, there is one option that's not bool; I'm thinking sticking with ast, or do we want to make a distinction on that?

will try/except, I'm thinking app should fail to start in this instance considering the configuration is vital to everything else.

Agreed. It should be a hard fail.

also, there is one option that's not bool; I'm thinking sticking with ast, or do we want to make a distinction on that?

I like Pydantic as it parses all the values that you'd assume are legit for a boolean: True,true,Yes,no,ON,Off,0,1...etc. The AST is for parsing the Python language itself. You basically have to make it exactly True or False, otherwise 💥. Also, you have a chance of parsing values into real Python values. For example, if you passed 0 or 1, you'd get an int, and then the app would likely fail down the line, perhaps in an unexpected way.

Also, I'm guessing there'll be more configs added to this project. FastAPI has a Pydantic-based Settings feature that seems like it'd handle our configs pretty cleanly.

https://fastapi.tiangolo.com/advanced/settings/#pydantic-settings

https://docs.pydantic.dev/latest/concepts/pydantic_settings/

I didn't want to add that to this PR since it's a bigger change than we'd originally discussed, but it seems like a nice baby step in that direction.

Oh yeah, and what's the option that's not a bool?

leeway is the option name, I remember vaguely with it being about timing.

hkeeler · 2023-09-28T04:12:54Z

src/oauth2/oauth2_admin.py

+def get_jwt_opts(opts_string: str) -> Dict[str, bool | int]:
+    """
+    Parses out the opts_string into JWT options dictionary.
+
+    Args:
+        opts_string (str): comma separated key value pairs in the form of "key1:True,key2:False"
+            all options are boolean, with exception of 'leeway' being int
+            valid options can be found here:
+            https://github.com/mpdavis/python-jose/blob/4b0701b46a8d00988afcc5168c2b3a1fd60d15d8/jose/jwt.py#L81
+
+    Returns:
+        dict: dictionary of options supported by jwt, mentioned in link above
+    """
+    jwt_opts = {}
+    if opts_string:
+        pairs = opts_string.split(",")
+        for pair in pairs:
+            [key, value] = pair.split(":", 1)
+            jwt_opts[key] = ast.literal_eval(value)
+    return jwt_opts
+
+
+JWT_OPTS = get_jwt_opts(os.getenv("JWT_OPTS", ""))


...and if we made them separate envvars, we could make this something like:

from pydantic import parse_obj_as from pydantic.error_wrappers import ValidationError import os

# Enable all jwt_opts settings by default JWT_OPTS = { "verify_at_hash": True, "verify_aud": True, "verify_iss": True, } # Override the defaults with envvars prefixed with JWT_OPTS_ for jwt_opt_key in JWT_OPTS: env_key = f"JWT_OPTS_{jwt_opt_key.upper()}" env_val = os.getenv(env_key, None) if env_val: try: JWT_OPTS[jwt_opt_key] = parse_obj_as(bool, env_val) except ValidationError: raise ValueError(f'jwt_opts setting "{env_key}" value of "{env_val}" could not be parsed into a boolean')

I was thinking of using JWT_ prefix parsing to get all possible configs from env vars, so we can dynamically add additional configs as needed.

I started off that way, filtering the list of envvars, but that didn't handle default values if an envvar wasn't set, or what'd happen if a envvar was added that had that prefix, but the suffix was invalid. Will it get ignored? Will it blow up? Having and explicit dict for the supported values, each with a default, and then looking up just those from the env seemed safer.

I'm not opposed to the other option. I just didn't know how jws_opts would handle those situations, and I didn't feel like doing the testing, so I went with safe.

gotcha, invalid suffices gets ignored; I think adding a docstring should serve the purpose of what the valid options are; also gives the flexibility that if any additional configs arise, no code changes are needed.

lchen-2101 · 2023-09-28T14:46:23Z

will also take this opportunity to refactor how we deal with env vars by creating constants in env.py instead of have the files that needs env vars to directly call os.getnv / os.environ...

hkeeler · 2023-09-28T15:56:57Z

will also take this opportunity to refactor how we deal with env vars by creating constants in env.py instead of have the files that needs env vars to directly call os.getnv / os.environ...

I think before we go too far down the road of creating our own thing, it's worth looking into FastAPI's Pydantic Settings. It seems to be the official-ish way to add settings to your project, and has stuff like envvar prefix handling and other sugar all baked in.

lchen-2101 · 2023-09-28T15:58:04Z

will also take this opportunity to refactor how we deal with env vars by creating constants in env.py instead of have the files that needs env vars to directly call os.getnv / os.environ...

I think before we go too far down the road of creating our own thing, it's worth looking into FastAPI's Pydantic Settings. It seems to be the official-ish way to add settings to your project, and has stuff like envvar prefix handling and other sugar all baked in.

oh nice, yeap, Settings is exactly what I had in mind.

hkeeler · 2023-09-28T16:09:44Z

Cool. If you want to go that route for this PR, 👍. I think it's the right way to go long term. I was initially hesitant to add it here since we hadn't planned for that work, but these type of settings seem like the right fit for it, so why go do a one-off custom thing we'll probably drop in the future anyway!? If it starts to look like it's going to stretch on, though, let's circle back to something simpler for now.

lchen-2101 · 2023-09-28T16:15:35Z

Cool. If you want to go that route for this PR, 👍. I think it's the right way to go long term. I was initially hesitant to add it here since we hadn't planned for that work, but these type of settings seem like the right fit for it, so why go do a one-off custom thing we'll probably drop in the future anyway!? If it starts to look like it's going to stretch on, though, let's circle back to something simpler for now.

Yeah, I think I'll change the ticket description, just adding jwt options is a really small change anyway, think it makes sense for this scope creep, I've been meaning to refactor the env var handling for a while now, might as well do it now.

src/entities/models/dto.py

lchen-2101 · 2023-10-11T14:38:08Z

src/entities/models/dto.py

@@ -15,7 +15,7 @@ class FinancialInsitutionDomainDto(FinancialInsitutionDomainBase):
    lei: str

    class Config:
-        orm_mode = True
+        from_attributes = True


orm_mode was deprecated and renamed to from_attributes: https://docs.pydantic.dev/2.4/migration/#changes-to-config

lchen-2101 · 2023-10-11T14:45:06Z

src/config.py

+
+    def __init__(self, **data):
+        super().__init__(**data)
+        self.set_jwt_opts()


using pydantic settings with prefixes still required defining the keys, and the extra param for the config wouldn't pull in the undefined configs if a prefix is specified; so jwt opts still needed some custom transformation.

hkeeler

Overall, looks good. The way jwt_opts gets initialized is a little more complicated than I thought it'd be, but I think that's OK. Thanks for adding the docstring on that one.

I added a few suggestions, mostly around using more of Pydantic's built-in types. Some of those seem pretty handy.

src/entities/models/dto.py

src/config.py

Co-authored-by: Hans Keeler <[email protected]>

lchen-2101 added 2 commits September 22, 2023 10:06

feat: make jwt options configurable

895c73c

feat: ruff check line length

a568d2c

lchen-2101 requested review from hkeeler, guffee23, aharjati and nargis-sultani September 22, 2023 14:14

feat: jwt opts string check

3931fe5

feat: add more doc on options, add test

8424fce

lchen-2101 mentioned this pull request Sep 22, 2023

feat: add jwt config cfpb/sbl-project#36

Merged

hkeeler reviewed Sep 28, 2023

View reviewed changes

lchen-2101 added 7 commits October 3, 2023 15:13

Merge branch 'main' into feature/25_add_jwt_config

3f6d250

Merge branch 'main' into feature/25_add_jwt_config

b129ad5

refactor: change env vars to use pydantic settings

318d19e

refactor: lint fix

aa7a7ac

docs: add jwt options docstring

1e1b6f8

feat: added tests

52e04f8

fix: lint fix

4fe02b9

lchen-2101 commented Oct 11, 2023

View reviewed changes

src/entities/models/dto.py Outdated Show resolved Hide resolved

lchen-2101 commented Oct 11, 2023

View reviewed changes

fix: typo

1e98b4a

lchen-2101 commented Oct 11, 2023

View reviewed changes

hkeeler reviewed Oct 18, 2023

View reviewed changes

lchen-2101 and others added 2 commits October 18, 2023 10:48

Update src/entities/models/dto.py

c4e0cd8

Co-authored-by: Hans Keeler <[email protected]>

feat: add pytest-env, and use pydantic settings types

1f278d2

fix: lock file updated

892f7bf

hkeeler approved these changes Oct 26, 2023

View reviewed changes

lchen-2101 merged commit 65d5d5c into main Oct 26, 2023

lchen-2101 deleted the feature/25_add_jwt_config branch October 26, 2023 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/25 add jwt config #35

Feature/25 add jwt config #35

lchen-2101 commented Sep 22, 2023

github-actions bot commented Sep 22, 2023 •

edited

Loading

src/config.py

src/entities/engine/engine.py

src/entities/models/dto.py

src/main.py

src/oauth2/oauth2_admin.py

hkeeler Sep 28, 2023

lchen-2101 Sep 28, 2023

hkeeler Sep 28, 2023

lchen-2101 Sep 28, 2023

hkeeler Sep 28, 2023

hkeeler Sep 28, 2023

lchen-2101 Sep 28, 2023

hkeeler Sep 28, 2023

lchen-2101 Sep 28, 2023

hkeeler Sep 28, 2023

lchen-2101 Sep 28, 2023

lchen-2101 commented Sep 28, 2023

hkeeler commented Sep 28, 2023

lchen-2101 commented Sep 28, 2023

hkeeler commented Sep 28, 2023

lchen-2101 commented Sep 28, 2023

lchen-2101 Oct 11, 2023

lchen-2101 Oct 11, 2023

hkeeler left a comment

		INST_CONN=postgresql+asyncpg://${INST_DB_USER}:${INST_DB_PWD}@${INST_DB_HOST}/${INST_DB_NAME}
		JWT_OPTS=verify_at_hash:False,verify_aud:False,verify_iss:False

	jwt_opts[key] = ast.literal_eval(value)
	pydantic.parse_obj_as(bool, envvar_name)

Feature/25 add jwt config #35

Feature/25 add jwt config #35

Conversation

lchen-2101 commented Sep 22, 2023

github-actions bot commented Sep 22, 2023 • edited Loading

Coverage report

src/config.py

src/entities/engine/engine.py

src/entities/models/dto.py

src/main.py

src/oauth2/oauth2_admin.py

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchen-2101 commented Sep 28, 2023

hkeeler commented Sep 28, 2023

lchen-2101 commented Sep 28, 2023

hkeeler commented Sep 28, 2023

lchen-2101 commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkeeler left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 22, 2023 •

edited

Loading