Refacto OIDC 3 #1405

natacha-beck · 2024-07-04T20:27:41Z

@prioux and @MontrealSergiy I think it is now ready for review.

…facto_oidc

…refacto_oidc_3

prioux · 2024-07-16T15:27:38Z

Can you please include the Globus record in your oidc.yml.rb file please? Also, this file will become part of the official distribution of CBRAIN, so it should contain

a comment block at the top explaining its purpose
all the configs should come with enabled: false, not true like in your commit, otherwise somebody installing a portal would get a non-functionning CBRAIN that crashes with a misconfigured keycloak auth service

prioux · 2024-07-16T15:31:07Z

BrainPortal/config/oidc.yml.erb

+  # Scope used when called the OpenID provider when autheticate
+  scope:                              "openid email profile"
+  # OpenID provider id and secret need to be passed via environment variables
+  client_id:                          <%= ENV["keycloak_client_id"] %>


The examples of ruby code to embed environment variables in the OIDC YAML file should also zap the environment variables, so that they don't get propagated inside the app.

e.g.

client_id: <%= ENV.delete 'keycloak_client_id' %>

When I booted my server, I got this in the initial boot messages:

C> Loading OIDC configuration... C> - ERROR: Cannot load OIDC configuration. C> Missing keys client_secret, client_id in OIDC config: Keycloak C> CBRAIN BrainPortal validation completed, 2024-07-16 11:34:22 -0400

Yet the application continued to boot. That's wrong, if the admin does not configure this important config file correctly, the server should not continue!

prioux · 2024-07-16T15:38:12Z

BrainPortal/lib/oidc_config.rb

+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+class OidcConfig


This class is not documented.

prioux · 2024-07-16T15:39:31Z

BrainPortal/lib/portal_system_checks.rb

+      begin
+        OidcConfig.load_from_file
+      rescue => ex
+        puts "C> \t- ERROR: Cannot load OIDC configuration."


Put a better message here, say something like check the content of "oidc.yml.rb"

Also, make it stop the server boot process.

Also, when enabled is false, it shouldn't complain about anything.

prioux · 2024-07-16T16:02:34Z

BrainPortal/config/oidc.yml.erb

+  # A OpenID provider can be enabled
+  enabled:                            true
+  # Configuration of the OpenID provider
+  authorize_uri:                      "http://localhost:8080/realms/.../protocol/openid-connect/auth"


What are the ... in the path? Ar they things I'm supposed to replace/substitute? What is supposed to go there?

prioux · 2024-07-16T17:09:50Z

BrainPortal/lib/globus_helpers.rb

+
+  def generate_oidc_login_uri(oidc_providers, redirect_url) #:nodoc:
+    @oidc_uris = {}
+    oidc_providers.each { |oidc| @oidc_uris[oidc.name] = oidc_login_uri(oidc, redirect_url)}


When I went to my account page, this line (245) crashed because oidc_providers was nil

The stack trace was:

lib/globus_helpers.rb:245:in `generate_oidc_login_uri' app/controllers/users_controller.rb:98:in `show' app/controllers/application_controller.rb:115:in `activate_user_time_zone'

It is because in the users controller code, method show, you misspelled one of the instance variables:

# Array of enabled OIDC providers configurations @oidc_configss = OidcConfig.enabled # Hash of OIDC uris with the OIDC name as key @oidc_uris = generate_oidc_login_uri(@oidc_configs, globus_url)

(see the @oidc_configss instead of @oidc_configs... )

prioux · 2024-07-16T17:30:49Z

My first step in reviewing this PR is to make sure that all features related to Globus have been preserved.

Right now, on my dev system, I have configured a Globus identity provider in the oidc.yml.erb file, and my server boots, but I can't go any further. The 'my account' page doesn't provide any way to link my account it globus.

Original CBRAIN code screenshot:

This PR's code screenshot:

prioux · 2024-07-16T18:52:53Z

BrainPortal/lib/globus_helpers.rb

-    provider_name = identity['identity_provider_display_name'] || cb_error("Globus: No identity provider name")
-    pref_username = identity['preferred_username'] ||
-                    identity['username']                       || cb_error("Globus: No preferred username")
+    provider_id   = identity[oidc.identity_provider_key]              || cb_error("#{oidc.name}: No identity provider")


Use oidc.identity_info() instead. That is what this method is for. You should try to hide any identity struct accesses by using a method, to make the code cleaner and avoid bugs (like the next comment I will leave about method find_user_with_globus_identity)

prioux · 2024-07-16T18:55:00Z

BrainPortal/lib/globus_helpers.rb

-  # Returns the user object if found; returns a string error message otherwise.
-  def find_user_with_globus_identity(globus_identity)
+  def find_user_with_oidc_identity(oidc, identity)
+    provider_name = identity[oidc.identity_provider_display_name]


Again, use oidc.identity_info() ; the code here crashes because you did not update the method names to say 'NNN_key'. This would not have happend when using identity_info()

prioux · 2024-07-16T19:38:56Z

BrainPortal/app/views/users/show.html.erb

+            <strong>Provider name:</strong> <%= prov_name %><br>
+            <strong>Provider user:</strong> <%= prov_user %><br>
+            <% if @user.id == current_user.id %>
+              <%= link_to("Unlink this #{oidc.name} identity", unlink_oidc_path(oidc.name, :from => "cb"),


Why is there :from => 'cb', it's not used at all

natacha-beck · 2024-07-22T20:40:55Z

Replaced by #1412

natacha-beck and others added 30 commits May 22, 2024 04:24

[WIP] 1st version of keycloak integration

9f9963b

Merge branch 'master' of https://github.com/aces/cbrain

79620db

[ADD] keycloak authentication

b5ce288

[RM] Backup file

36a299b

[ADD] do cleanup

dc13e70

[ADD] do cleanup

8a8c67f

[ADD] do cleanup

5a0455d

[WIP]

753c704

[WIP]

71fbd29

[WIP]

428c624

[WIP]

6eecfb3

[WIP]

8653d1a

[WIP]

41398fd

[WIP]

765d1ff

[WIP]

9127d99

[WIP]

94c8064

[WIP]

d777b12

[WIP]

f9084c1

[WIP]

ed8c6c0

Update nh_sessions_controller.rb

03048a5

WIP

d1ded63

app/controllers/nh_sessions_controller.rb

5015c2a

Merge branch 'refacto_oidc' of github.com:natacha-beck/cbrain into re…

1aa0d36

…facto_oidc

[WIP]

930e238

[WIP]

24b680b

[WIP]

2da6b5e

[WIP]

e617a73

[WIP]

f5a62cc

[WIP]

e031332

[WIP]

b8d874a

natacha-beck and others added 12 commits July 15, 2024 10:48

Merge branch 'refacto_oidc_3' of github.com:natacha-beck/cbrain into …

6988ab7

…refacto_oidc_3

Update globus_helpers.rb

e98890e

Update show.html.erb

3734864

Update show.html.erb

53e383b

Update globus_helpers.rb

bddffbf

Update nh_sessions_controller.rb

b5f55f3

Update nh_mandatory_oidc.html.erb

a8b4542

Merge branch 'refacto_oidc_3' of github.com:natacha-beck/cbrain into …

29bbe79

…refacto_oidc_3

[CLEANUP]

7681c73

[CLEANUP]

09ad36f

[CLEANUP]

fb9a835

[CLEANUP]

af1c7aa

prioux reviewed Jul 16, 2024

View reviewed changes

prioux mentioned this pull request Jul 16, 2024

Refacto OIDC #1398

Closed

prioux reviewed Jul 16, 2024

View reviewed changes

natacha-beck added 4 commits July 17, 2024 12:16

[FIX] after PR review

cba5945

[FIX] enabled values

62d8edd

[FIX] after PR review

9396774

[FIX] after PR review

038a151

prioux force-pushed the master branch from fd9c1fd to 7fd88dc Compare July 21, 2024 17:20

natacha-beck closed this Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refacto OIDC 3 #1405

Refacto OIDC 3 #1405

natacha-beck commented Jul 4, 2024 •

edited

Loading

prioux commented Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024 •

edited

Loading

prioux Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024 •

edited

Loading

prioux Jul 16, 2024

prioux commented Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024

prioux Jul 16, 2024

natacha-beck commented Jul 22, 2024

Refacto OIDC 3 #1405

Refacto OIDC 3 #1405

Conversation

natacha-beck commented Jul 4, 2024 • edited Loading

prioux commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prioux Jul 16, 2024 • edited Loading

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

prioux Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prioux commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natacha-beck commented Jul 22, 2024

natacha-beck commented Jul 4, 2024 •

edited

Loading

prioux Jul 16, 2024 •

edited

Loading

prioux Jul 16, 2024 •

edited

Loading