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

fix: update get_username function #365

Merged
merged 4 commits into from
Apr 29, 2024
Merged

Conversation

botanical
Copy link
Member

@botanical botanical commented Apr 29, 2024

Issue

NASA-IMPACT/veda-data-airflow#134
#347

What?

  • Update get_username to fall back on sub if username doesn't exist in token
  • Added some more logging statements to make debugging easier in the future

Why?

  • This fix is to enable a successful workflows API run since the workflow API passes a token to backend API and it's currently erroring
  • Also, sub is a more definitive identifier because it represents a unique identifier compared to username

Testing?

  • Relevant testing details

@smohiudd smohiudd self-requested a review April 29, 2024 16:54
@botanical botanical requested a review from anayeaye April 29, 2024 17:14
return token["username"]
def get_username(token: Annotated[Dict[Any, Any], Depends(validated_token)]) -> str:
logger.info(f"\nToken {token}")
result = token["username"] if "username" in token else token.get("sub", None)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mixed about having a default when we might want to fail but because this depends on validation I think it is OK? We can decide after it works or not

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dbb62fb Perhaps we can raise the error instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f640fdf change reverted since we don't want to be blocked and could forget about this change later on

@botanical botanical merged commit 28afca4 into develop Apr 29, 2024
4 checks passed
@botanical botanical deleted the jt/update-get-username branch April 29, 2024 18:52
botanical added a commit that referenced this pull request May 6, 2024
### Breaking
- #356

#### Breaking changes notes
Breaking: `VEDA_COGNITO_DOMAIN` configuration now required along with
one time administrator step to update existing user pool client allowed
callback urls with the ingest-api's URL

### Added
- #342
- #330
- #323

### Changed/Updated
- #355
- #340

### Fixed

1. - #367
2. - #365
3. - #361
4. - #360
5. - #358
8. - #345
9. - #344
12. - #339
13. - #338
14. - #337
15. - #335
16. - #334
17. - #331
20. - #329
21. - #327
22. - #326
23. - #325
24. - #324
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