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

feat: logout with ueberauth_oidcc #950

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Conversation

paulswartz
Copy link
Member

@paulswartz paulswartz commented Nov 3, 2023

Summary of changes

Support logging out based on the Ueberauth.Strategy.Oidcc strategy, and API access via the Keycloak API. Based on top of #952 .

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

@paulswartz paulswartz marked this pull request as draft November 3, 2023 17:43
@paulswartz paulswartz changed the title wip: logout with ueberauth_oidc feat: logout with ueberauth_oidc Nov 3, 2023
@paulswartz paulswartz force-pushed the ps-keycloak-entraid branch 2 times, most recently from f5bfba2 to e3fcae3 Compare November 13, 2023 23:55
@paulswartz paulswartz requested review from a team and nlwstein and removed request for a team December 1, 2023 20:27
@paulswartz paulswartz marked this pull request as ready for review December 1, 2023 20:27
@mbta mbta deleted a comment from github-actions bot Dec 1, 2023
@mbta mbta deleted a comment from github-actions bot Dec 1, 2023
@mbta mbta deleted a comment from github-actions bot Dec 1, 2023
@mbta mbta deleted a comment from github-actions bot Dec 1, 2023
Copy link
Contributor

@nlwstein nlwstein left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me, just a couple non-blocking q's 😄

test/arrow_web/try_api_token_auth/keycloak_test.exs Outdated Show resolved Hide resolved
ArrowWeb.AuthManager,
auth_token.username,
%{roles: roles},
ttl: {0, :second}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this provide a session that never expires?

Copy link
Member Author

Choose a reason for hiding this comment

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

The opposite; it expires immediately so that if it does happen to make it outside of the API (which it shouldn't) it won't be usable.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be a session that expires immediately, but I will double-check that's how Guardian treats it (or if there's another way to implement this).

The issue I ran into (and I think this is true of the existing implementation) is that the API returned a session cookie that you can use instead of the API key, and I didn't think that was the normal approach for an API. We disable that now, but also set a 0-second TTL to ensure that if the token did leak out, it wouldn't be usable.

Copy link

github-actions bot commented Dec 5, 2023

Coverage of commit 372c279

Summary coverage rate:
  lines......: 88.8% (672 of 757 lines)
  functions..: 59.2% (515 of 870 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow_web/controllers/auth_controller.ex                       |73.1%     26|85.7%     7|    -      0
  lib/arrow_web/router.ex                                            |95.0%     20|43.8%    73|    -      0
  lib/arrow_web/try_api_token_auth.ex                                |92.9%     14| 100%     3|    -      0
  lib/arrow_web/try_api_token_auth/keycloak.ex                       |90.6%     32| 100%     5|    -      0
  test/support/fake_oidcc.ex                                         | 100%      1| 100%     1|    -      0

Download coverage report

@paulswartz paulswartz merged commit 9cbafd0 into master Dec 5, 2023
10 checks passed
@paulswartz paulswartz deleted the ps-keycloak-entraid branch December 5, 2023 17:37
@paulswartz paulswartz restored the ps-keycloak-entraid branch December 5, 2023 17:37
@paulswartz paulswartz deleted the ps-keycloak-entraid branch December 5, 2023 17:37
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