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

Convert JWT amr to list if string #85

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

danipran
Copy link
Contributor

As discussed here: #73

This converts amr to a list if it's a string.

Apparently, Tunnistamo sends amr as a string, not a list. This screws things up if drf-oidc-auth's version is >=1.0.0. However, drf-oidc-auth >=1.0.0 is required for Django 4, so something needs to be done. I went with @nikomakela's suggestion.

I suppose it's not a perfect solution, but it should do the job for the time being.

@danipran danipran requested a review from akikoskinen March 21, 2023 14:20
@danipran
Copy link
Contributor Author

danipran commented Mar 21, 2023

Note that I am not 100% sure if this is still an issue. If this has been fixed in Tunnistamo already, then this PR is probably redundant and should be closed. I go with the assumption that this is still a "feature" in Tunnistamo.

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #85 (c735af7) into master (2493957) will increase coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head c735af7 differs from pull request most recent head 6574c54. Consider uploading reports for the commit 6574c54 to get more accurate results

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   51.80%   51.94%   +0.14%     
==========================================
  Files          30       30              
  Lines        1000     1003       +3     
==========================================
+ Hits          518      521       +3     
  Misses        482      482              
Impacted Files Coverage Δ
helusers/_oidc_auth_impl.py 80.28% <100.00%> (+0.86%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

helusers/_oidc_auth_impl.py Outdated Show resolved Hide resolved
helusers/_oidc_auth_impl.py Outdated Show resolved Hide resolved
helusers/tests/test_oidc_api_token_authentication.py Outdated Show resolved Hide resolved
helusers/tests/test_oidc_api_token_authentication.py Outdated Show resolved Hide resolved
@danipran danipran force-pushed the convert-amr-to-list branch from 84e0706 to 848a621 Compare March 22, 2023 10:17
@danipran danipran requested a review from akikoskinen March 22, 2023 10:28
@danipran danipran force-pushed the convert-amr-to-list branch from fbb6644 to c735af7 Compare March 27, 2023 06:23
Copy link
Contributor

@akikoskinen akikoskinen left a comment

Choose a reason for hiding this comment

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

The end result looks good now. There are some unnecessary intermediate commits that don't need to be preserved. Perhaps squashing the three commits into one does the job.

@danipran
Copy link
Contributor Author

Oh yeah, I apparently failed at amending commits 🤡

Apparently, this screws things up if drf-oidc-auth is below 1.0.0.
drf-oidc-auth >=1.0.0 is required for Django 4, however.
Bit of a workaround, but should work?
Source: City-of-Helsinki#73 (comment)
@danipran danipran force-pushed the convert-amr-to-list branch from c735af7 to 6574c54 Compare March 30, 2023 06:23
@danipran danipran requested a review from akikoskinen March 30, 2023 06:24
@danipran danipran merged commit 95a7421 into City-of-Helsinki:master Mar 30, 2023
@danipran danipran deleted the convert-amr-to-list branch March 30, 2023 07:20
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.

2 participants