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.

Sign up for GitHub

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

Jump to bottom

OIDC Login #201

Merged
merged 11 commits into from
Jan 6, 2023
Merged

OIDC Login #201

merged 11 commits into from
Jan 6, 2023

Conversation

dasGoogle
Copy link
Collaborator

@dasGoogle dasGoogle commented Jan 3, 2023 •
edited
Loading

Fixes #59

Description (what might a PO or reviewer want to know?)

  • Log in using the OIDC Login button on the login page
  • You can keep your current account if the email address matches

PR checklist

  • dev-branch has been merged into local branch to resolve conflicts
  • tests and linter have passed AFTER local merge
  • localization is supported (Guide)
  • another dev reviewed and approved
  • if feature-Branch: Teams PO has approved (show via e.g. screenshots/screencapture/live demo)

dasGoogle and others added 2 commits January 3, 2023 15:16
@dasGoogle @Sohn123
Add first version of OIDC login
5e24df5 
Co-authored-by: Alexander Sohn <[email protected]>
@dasGoogle
Fix rubocop issues
b1e9186 
Co-authored-by: Alexander Sohn <[email protected]>
@dasGoogle dasGoogle added the BP-HGHK BP Giese/Karl label Jan 3, 2023
@dasGoogle dasGoogle requested a review from lukasrad02 January 3, 2023 14:20
@dasGoogle
Copy link
Collaborator Author

Peek.2023-01-05.16-21.webm

@benn02

@dasGoogle dasGoogle requested a review from benn02 January 5, 2023 15:22
@dasGoogle dasGoogle marked this pull request as ready for review January 5, 2023 15:22
benn02
benn02 approved these changes Jan 5, 2023
View reviewed changes
Copy link
Collaborator

@benn02 benn02 left a comment

Choose a reason for hiding this comment

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

Very nice

@dasGoogle
Copy link
Collaborator Author

@rgwohlbold When deploying OIDC, we need to add OIDC client credentials in the corresponding ENV variables. See here. Can you take care of this?

@rgwohlbold
Copy link
Collaborator

@rgwohlbold When deploying OIDC, we need to add OIDC client credentials in the corresponding ENV variables. See here. Can you take care of this?

I created two OIDC clients and added you as an owner @dasGoogle. I added the config variables to both deployments, we should check after the merge that it is configured correctly.

@dasGoogle
Copy link
Collaborator Author

@rgwohlbold When deploying OIDC, we need to add OIDC client credentials in the corresponding ENV variables. See here. Can you take care of this?

I created two OIDC clients and added you as an owner @dasGoogle. I added the config variables to both deployments, we should check after the merge that it is configured correctly.

Thanks!

lukasrad02
lukasrad02 reviewed Jan 6, 2023
View reviewed changes
Copy link
Collaborator

@lukasrad02 lukasrad02 left a comment

Choose a reason for hiding this comment

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

Besides the localization of the button, it looks pretty good to me. Nice work!

app/views/devise/shared/_links.html.erb Outdated
<%= button_to(omniauth_authorize_path(resource_name, provider),
class: 'btn btn-primary', id: "#{provider}-signin" ,
method: :post,
data: {disable_with: "<i class='fa fa-spinner fa-spin'></i> Logging in...", turbo: "false"}) do %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand this correctly, the "logging in" text will be shown on the "Sign in with OIDC" button after clicking it, but I've been never able to see it, even with throttled requests to the OIDC server.

But if it should be visible in some cases, it would be nice if it was localized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I simply removed this text now, I think the icon suffices. I honestly also don't know when this should be shown.

@dasGoogle dasGoogle merged commit 7791efe into dev Jan 6, 2023
@dasGoogle
Copy link
Collaborator Author

@rgwohlbold When deploying OIDC, we need to add OIDC client credentials in the corresponding ENV variables. See here. Can you take care of this?

I created two OIDC clients and added you as an owner @dasGoogle. I added the config variables to both deployments, we should check after the merge that it is configured correctly.

It seems that the base URL you configured in APP_BASE_URL is http rather than https which makes the authentication fail. Could you maybe fix this in the deployments, @rgwohlbold?

@rgwohlbold
Copy link
Collaborator

rgwohlbold commented Jan 6, 2023 •
edited
Loading

Sorry, should be fixed now

antonneubauer pushed a commit that referenced this pull request Jan 6, 2023
@dasGoogle @Sohn123 @antonneubauer
OIDC Login (#201)
d786d34 
* Add first version of OIDC login

Co-authored-by: Alexander Sohn <[email protected]>

* Fix rubocop issues
Co-authored-by: Alexander Sohn <[email protected]>

* Revert DB Schema changes

Co-authored-by: Alexander Sohn <[email protected]>

* Revert DB Schema changes - like, eally

Co-authored-by: Alexander Sohn <[email protected]>

* Add tests

Co-authored-by: Alexander Sohn <[email protected]>

* Fix linter issues
Co-authored-by: Alexander Sohn <[email protected]>

* Remove schema changes and remove CSRF on certain route

* Update schema to current version on main

* Remove unlocalized text

Co-authored-by: Alexander Sohn <[email protected]>
antonneubauer added a commit that referenced this pull request Jan 6, 2023
@antonneubauer
Revert "OIDC Login (#201)"
5c7033c 
This reverts commit d786d34.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benn02 benn02 benn02 approved these changes

@lukasrad02 lukasrad02 lukasrad02 approved these changes

Assignees

@dasGoogle dasGoogle

@Sohn123 Sohn123

@Nils1729 Nils1729

Labels
BP-HGHK BP Giese/Karl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OIDC
6 participants
@dasGoogle @rgwohlbold @lukasrad02 @benn02 @Sohn123 @Nils1729