[XI-6523] Transfer enmeshed changes from Xikolo #1831

nenock · 2025-01-09T14:06:56Z

This PR transfers and adapts the changes done in Xikolo, see https://lab.xikolo.de/xikolo/web/-/merge_requests/4665 .

Changes include:

refactorings of the JS code
adaptations to support nmshd::Connector API version 3.1.0
replacement of the npb_wallet_controller_spec by request specs
several small fixes, see commit message for more details

codecov · 2025-01-09T14:12:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.79%. Comparing base (86c4ee9) to head (7fe70a9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1831      +/-   ##
==========================================
+ Coverage   94.76%   94.79%   +0.02%     
==========================================
  Files         133      133              
  Lines        3346     3359      +13     
==========================================
+ Hits         3171     3184      +13     
  Misses        175      175

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nenock · 2025-01-23T10:43:56Z

I was able to verify the "good" path locally with my local connector still holding authorized data from the enmeshed backbone and using the mock saml. @MrSerth Do you want to do a quick review or are you good with me merging now?

MrSerth

Thank you so much for taking care and transferring the changes back. I really appreciate your time and all the effort you've invested here. 🌷

As a gratitude, I've taken some time to review and test your changes. A few remarks are related to potential defects, a few other minor improvements to the code or documentation. I would hope you won't spent too much time and that my comments are self-explanatory.

app/assets/javascripts/nbp_wallet.js

MrSerth · 2025-01-25T11:51:15Z

app/assets/javascripts/nbp_wallet.js

+  if (templateValidity > 0) {
+    templateValidity -= 1;
+  }
+  if (templateValidity === 0) {
+    window.location.reload();
+  }


Should we perform this check only if finalizing is not true?

Maybe. I tend to still drop the finalizing variable. The worst case would be that the page is reloaded with a new QR code while, at the same time, the relationship is ready to be finalized. After that reload, the relationship is still ready, and the redirect to finalize still happens. 🤷‍♀️

Sure, I am fine dropping the finalizing variable. We just need to be aware that the code is executed globally and remains active when navigating through Turbolinks.

Two considerations regarding your comment:

The worst case would be that the page is reloaded with a new QR code while, at the same time, the relationship is ready to be finalized.

When the relationship is ready when requesting the page, we redirect the user to the finalizing page through Rails:

codeharbor/app/controllers/users/nbp_wallet_controller.rb

Lines 10 to 12 in c5a2837

if Enmeshed::Relationship.pending_for(@provider_uid).present?

redirect_to nbp_wallet_finalize_users_path and return

end

Then, a race condition could occur: First, we could have the JavaScript-initialized redirect to finalize which is still working and second, we could have the reloading of the page (triggered by the end of the QR code validity) performing a redirect to finalize, too. Then, we would have two requests for finalize "in parallel". Only one of them is likely to succeed, and the other will call abort_and_refresh. When the user-visible navigation calls abort_and_refresh, we are redirected once again to the connect page, just to identify that the user is not allowed to access it any longer, redirecting them to the registrations path (which you cannot access as a signed in user), causing another redirect to the root path. I don't want to imagine which error message is given in this case.

The worst case would be that the page is reloaded with a new QR code while, at the same time, the relationship is ready to be finalized.

When changing the page through Turbolinks (i.e., clicking on the logo in the top left corner), the counter is not stopped either. Hence, the reload might also happen at some other page "randomly".

I conclude:

We should stop the time once finishing.

We should stop the time once navigating to another page.

Revised in latest commit.

We should stop the time once navigating to another page.

I verified it by adding some console logs.

We should stop the time once finishing.

To avoid the finalizing variable, I added being on the connect page as another condition to reload the page when the countdown is up.

What do you think about the current version?

Thanks, these changes look fine! I haven't tested the edge cases, but think it is mostly fine. Shall we add the same check for the current page / controller (see other discussion below) to the checkStatus as well (just to be sure)?

if (json.status === 'ready' && <on page>) {

This way (by making use of the short circuit evaluation) we're only checking the current page once the connection is ready. It might not be needed, just as a safeguard?

Sure. But you don't mean to also drop the check in line 39?

If we wouldn't need the check, I am fine. But given all that Turbolinks involvement, I am rather on the safe side. Regarding the check in line 39: I don't remember. Didn't I suggest to use a common check approach wherever needed?

app/assets/javascripts/nbp_wallet.js

MrSerth · 2025-01-25T13:03:32Z

lib/enmeshed/connector.rb

-    private_class_method :conn
+      # @return [Faraday::Connection] The connection to the enmeshed connector.
+      # @raise [ConnectorError] If the connector is not configured as expected.
+      def connection


With this new version, you are no longer memorizing the connection. By creating a new connection object each time, we perform the checks all over again and can't use HTTP keep alive (i.e., as supported by faraday-net_http_persistent).

MrSerth · 2025-01-25T13:23:01Z

lib/enmeshed/object.rb


-    def self.validate!(instance)
-      raise ConnectorError.new("Invalid #{klass} schema") unless schema.valid?(instance)
+      KNOWN_API_ISSUES = ['value at `/auditLog/0/createdBy` does not match format: date-time'].freeze


I can confirm this bug and created an issue: nmshd/feedback#32

My bug has been resolved and a new connector version 6.13.2 has been released. I didn't test yet, but would hope that this new version actually fixes the error and removes the necessity for the KNOWN_API_ISSUES.

MrSerth · 2025-01-25T13:33:20Z

lib/enmeshed/relationship.rb

+    def check_for_required_attributes!(enmeshed_user_attributes)
+      blank_attributes = enmeshed_user_attributes.select {|_, v| v.blank? }.keys
+      missing_attributes = Enmeshed::RelationshipTemplate::REQUIRED_ATTRIBUTES - enmeshed_user_attributes.keys
+
+      if blank_attributes.any? || missing_attributes.any?
+        raise ConnectorError.new("#{(blank_attributes << missing_attributes).flatten.join(', ')} must not be empty")
+      end


I am not sure whether this method (and its effect) is desired in the context of CodeHarbor (it could be in the context of Xikolo). Without this method, an invalid attribute (e.g., space for given and family name) will result in an explanatory flash message:

Benutzer:in konnte nicht erstellt werden: Vorname muss ausgefüllt werden, Nachname muss ausgefüllt werden

However, when this method is used, the error shown to users is simply:

Ein Fehler ist aufgetreten.

Could you please double-check that?

You're right, I double-checked it.
The intention was to abort the workflow as early as possible and raise the error there. For Xikolo, the finalize action works different, that's why I missed this undesired error handling here.

Ideally, I'd like to have different subtypes of ConnectorError to be able to handle the errors more specifically, but this is out of scope, unfortunately. You may want to add a ticket to improve the error handling to the backlog for your successor, though 😉

That's the workaround, I'd suggest:

def accept_and_create_user(relationship) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength # Enmeshed::Relationship checks for the presence of all the requested attributes first and later parses the # provided status group. If it is not a synonym of the valid ones, the attribute is cleared so that a fitting # alert can be passed on to the user here. begin if relationship.userdata[:status_group].blank? abort_and_refresh( relationship, t('common.errors.model_not_created', model: User.model_name.human, errors: t('users.nbp_wallet.unrecognized_role')) ) and return end rescue ConnectorError => e # If the error is due missing attributes, pass on the error message to the user. e.include?('must not be empty') ? abort_and_refresh(relationship, e) : abort_and_refresh(relationship) end .... end

Thanks for taking the time to check the behavior and confirming.

It seems like we may have different expectations about the ConnectorError. From what I read, you would also use a (subclass of) ConnectorError to indicate a domain-specific expectation error in case of invalid or insufficient attributes. That's well possible, of course.

I would, however, not do that. From my perspective, any type of ConnectorError is more linked to a technical problem (connector not reachable, response not parsable) or a severe expectation that failed (like the two pending relationship change requests in the previous version of the API). In that sense, I would not consider the value of user-provided data to cause such an exception. Instead, I see it similar to the regular user registration: A user provides some data about themselves and we use ActiveRecord validations to ensure that the data provided matches our desired format. (Side note: In a multi-service architecture with a dedicated account service, rendering an erroneous response for invalid data and handling that similar to an exception seems plausible to me.) This approach (handling validations through ActiveRecord) is also why we have this transaction when creating the user account:

codeharbor/app/controllers/users/nbp_wallet_controller.rb

Lines 63 to 78 in 5a9d6d3

ApplicationRecord.transaction do

unless user.save

abort_and_refresh(

relationship,

t('common.errors.model_not_created', model: User.model_name.human, errors: user.errors.full_messages.join(', '))

) and raise ActiveRecord::Rollback

end

if relationship.accept!

user.send_confirmation_instructions

redirect_to home_index_path, notice: t('devise.registrations.signed_up_but_unconfirmed')

else

abort_and_refresh(relationship)

raise ActiveRecord::Rollback

end

end

With these lines, we ensure that the regular validations need to pass and if anything goes wrong, we neither accept the relationship nor create the user. Either both pass, or none (in which case we will automatically reject the relationship request).

The current design of the relationship.userdata is to pass a hash of attributes, ideally without any exception (even if the user-provided data is incomplete). Sure, we also raise a ConnectorError right now, but only in those technical cases (which should, ideally, not caused by user interventions).

That having said, I am still not completely convinced of checking the presence of these attributes in lib/enmeshed explicitly, since I feel it duplicates some of the behavior of ActiveRecord validations (and might require us to match an error message with a fixed string).

I get your point. But then, also the parsing of the status group based on synonyms is misplaced in the scope of Enmeshed::Relationship#userdata and should be part of the model validations:

def parse_status_group(affiliation_role) if STATUS_GROUP_SYNONYMS['learner'].any? {|synonym| synonym.downcase.include? affiliation_role } :learner elsif STATUS_GROUP_SYNONYMS['educator'].any? {|synonym| synonym.downcase.include? affiliation_role } :educator end end

I just pushed a commit, that combines all those userdata validations in an additional validation method in the NBPWalletController. I'm also not happy with how those two domains interfere, but it works and is fully tested. With limited capacities in mind, I'd leave it like that and rather not do more refactorings regarding the attribute validation situation. What do you think?

MrSerth · 2025-01-25T13:37:04Z

spec/fixtures/files/enmeshed/valid_relationship_created.json

          "onNewRelationship": {
+            "expiresAt": "2024-11-07T23:36:43.893Z",


Should we use a date far in the future (such as below, see 2077)?

spec/requests/users/nbp_wallet/finalize_spec.rb

MrSerth

I assume you're still working on the code. Nevertheless, here is some intermediate feedback based on your recent changes. Thanks for taking care of all those JavaScript issues already 🦋.

app/assets/javascripts/nbp_wallet.js

MrSerth · 2025-01-30T12:58:42Z

app/assets/javascripts/nbp_wallet.js

+  } catch (error) {
+    console.error(error);
+  }
+  timeoutID = setTimeout(checkStatus, 1000);


From reading the code, I would assume that we schedule another checkStatus check even if the json.status === 'ready' (in which case, we wouldn't need any further checks).

Setting the window.location.pathname performs like an early return (once the redirect happens). Also, the beforeunload and turbolinks:before-render event listeners clear the timeout due to this redirect.
When we don't get a response for the redirect in time, we kind of just try again. IMO, that does not hurt.

Setting the window.location.pathname performs like an early return (once the redirect happens). Also, the beforeunload and turbolinks:before-render event listeners clear the timeout due to this redirect.

That's good.

When we don't get a response for the redirect in time, we kind of just try again. IMO, that does not hurt.

When we don't get a response for the redirect in time, the Rails server might still process the enmeshed changes. The second check should be fine, but I want to avoid that we set the window.location.pathname a second time (which I would consider to be more problematic).

A quick check revealed:

setTimeout(function() {console.log(window.location.pathname); window.location.pathname = "/404"; console.log(window.location.pathname);}, 1000);

From this experiment, I would conclude that the condition (json.status === 'ready' && window.location.pathname === Routes.nbp_wallet_connect_users_path()) would still true. My suggestion: Let's avoid this home-made concurrency problem 😉

So, it's the finalizing variable, again? I might get you wrong here, as I have a brainfog due to a upcoming cold, sorry for that 😕

MrSerth · 2025-01-30T13:10:48Z

app/views/users/nbp_wallet/connect.html.slim

+      = image_tag @relationship_template.qr_code_path,
+        data: {id: 'nbp_wallet_qr_code', 'remaining-validity': @relationship_template.remaining_validity},
+        alt: t('.qr_code_alt_text'),
+        class: 'img-responsive'


Changing the class might have unintended consequences:

img-fluid nbp_wallet_qr_code img-responsive

MrSerth · 2025-01-30T13:15:15Z

app/views/users/nbp_wallet/connect.html.slim

+        data: {id: 'nbp_wallet_qr_code', 'remaining-validity': @relationship_template.remaining_validity},
+        alt: t('.qr_code_alt_text'),
+        class: 'img-responsive'
+    .buttons.btn.btn-primary.w-100.mt-3 data-behavior='reload-on-click'


I cannot find a definition for the class buttons (neither within our code, nor Bootstrap). What should it do?

On that same note (and for my personal learning): Why is a div beneficial over a real button? Because it doesn't "submit" the (non-existing) form?

app/controllers/application_controller.rb

- Increase code robustness Use `setTimeout` instead of `setInterval` to prevent queued up XHR requests returning unordered in case of e.g. an unresponsive server Part of XI-6523

…ibly `RelationshipTemplate#remaining_validity` requires the attribute `expire_at` to be of type `ActiveSupport::TimeWithZone` to be able to perform `minus_without_duration`. Part of XI-6523

- Group class methods with `class << self` - Write out abbreviations - Enmeshed::Connector: Combine initialization and memoization of the Faraday connection into a single method (no expensive calculations here) Part of XI-6523

Memorizing the connection without using Faraday's adapter `net_http_persistent` does not come with any benefits (every request requires setting up a new TCP socket with the default adapter). In contrast, it makes testing the class more difficult, because Rspec does not reload classes (and thus class variables) between tests. Defaultly, a Faraday connection does not time out. Part of XI-6523

Since RSpec 3.5, controller specs are deprecated. The official recommendation of the Rails team and the RSpec core team is to write request specs instead. They involve the router, the middleware stack, and both rack requests and responses. Thus, it's not possible to set the session variables beforehand anymore. Instead, a request spec should call the sign in endpoint before calling the actual endpoint under test, when the session is needed. To avoid the complexity of SSO and SLOs during request tests, this helper introduces the option to set the session variables via a designated endpoint for tests. https://gist.github.com/dteoh/99721c0321ccd18286894a962b5ce584?permalink_comment_id=4188995#gistcomment-4188995

Part of XI-6523

…t/*` Usually, when the current user is not logged in and unauthorized, a redirect to the registration page makes sense. But in case of the `NBPWalletController` actions, the SAML provider and uid are missing and the provided alert message is more meaningful. Part of XI-6523

Drop corresponding controller spec while maintaining full test coverage Part of XI-6523

Main changes: - Relationship: Adapt parsing of the userdata to new schema Values to the requested attributes are now passed in `creationContent/response/items` (former: `changes/request/response/items`). - Connector: Adapt accepting/rejecting a Relationship to new endpoints `/api/v2/Relationships/#{relationship_id}/Changes/#{change_id}/#{action}` was dropped in favor of `/api/v2/Relationships/#{relationship_id}/Accept` (and `Reject`) Part of XI-6523

This decreases the Assignment Branch Condition size of the method.

Part of XI-6523

When appending a return statement to a method call, either with `and` or `&&`, the return won't happen if there is a tailing one-line if statement. Part of XI-6523

…s to `Attribute::Identity` The former implementation of `Attibute.to_json` was very specific to IdentityAttributes. It actually only worked for simple IdentityAttributes and would need adaptations for complex IdentityAttributes. Part of XI-6523

…` model Part of XI-6523

nenock force-pushed the nn/XI-6523_transfer_enmeshed_changes_from_Xikolo branch 6 times, most recently from 4f38640 to c5a2837 Compare January 22, 2025 11:20

nenock marked this pull request as ready for review January 22, 2025 11:29

MrSerth reviewed Jan 25, 2025

View reviewed changes

nenock force-pushed the nn/XI-6523_transfer_enmeshed_changes_from_Xikolo branch from c5a2837 to 3817303 Compare January 30, 2025 12:41

MrSerth reviewed Jan 30, 2025

View reviewed changes

nenock force-pushed the nn/XI-6523_transfer_enmeshed_changes_from_Xikolo branch 2 times, most recently from 16ae249 to 5dad0f5 Compare February 4, 2025 13:37

nenock added 16 commits February 11, 2025 14:08

refactor(nbp): decaffeinate nbp_wallet.coffee

ae51e63

- Increase code robustness Use `setTimeout` instead of `setInterval` to prevent queued up XHR requests returning unordered in case of e.g. an unresponsive server Part of XI-6523

fix(enmeshed): parse expiration time of RelationshipTemplate invers…

1c45c75

…ibly `RelationshipTemplate#remaining_validity` requires the attribute `expire_at` to be of type `ActiveSupport::TimeWithZone` to be able to perform `minus_without_duration`. Part of XI-6523

style(enmeshed): group class methods in the lib

50ae574

- Group class methods with `class << self` - Write out abbreviations - Enmeshed::Connector: Combine initialization and memoization of the Faraday connection into a single method (no expensive calculations here) Part of XI-6523

fix(enmeshed): add initialization of ConnectorError where missing

7e56451

test(enmeshed): add more lib specs

050d56d

Part of XI-6523

test(nbp): add request specs

910d986

Drop corresponding controller spec while maintaining full test coverage Part of XI-6523

docs(enmeshed): explain status group edge case

59b9442

style(enmeshed): tweak Connector::fetch_existing_attribute

756b50e

This decreases the Assignment Branch Condition size of the method.

fix(nbp): Stop QR code countdown when leaving the page

6b1de97

Part of XI-6523

fix(nbp): abort finalize action if no relationship exists

4603bd8

When appending a return statement to a method call, either with `and` or `&&`, the return won't happen if there is a tailing one-line if statement. Part of XI-6523

refactor(enmeshed): Leave the validation of the userdata to the `User…

7fe70a9

…` model Part of XI-6523

nenock force-pushed the nn/XI-6523_transfer_enmeshed_changes_from_Xikolo branch from ec816b1 to 7fe70a9 Compare February 11, 2025 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[XI-6523] Transfer enmeshed changes from Xikolo #1831

[XI-6523] Transfer enmeshed changes from Xikolo #1831

nenock commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

nenock commented Jan 23, 2025 •

edited

Loading

MrSerth left a comment

MrSerth Jan 25, 2025

nenock Jan 27, 2025

MrSerth Jan 27, 2025

nenock Jan 30, 2025

MrSerth Jan 30, 2025

nenock Feb 4, 2025

MrSerth Feb 5, 2025

MrSerth Jan 25, 2025

MrSerth Jan 25, 2025

MrSerth Jan 27, 2025

MrSerth Jan 25, 2025

nenock Feb 4, 2025

MrSerth Feb 5, 2025

nenock Feb 5, 2025

nenock Feb 5, 2025

MrSerth Jan 25, 2025 •

edited

Loading

MrSerth left a comment

MrSerth Jan 30, 2025

nenock Feb 4, 2025 •

edited

Loading

MrSerth Feb 5, 2025

nenock Feb 5, 2025

MrSerth Jan 30, 2025

MrSerth Jan 30, 2025

	if Enmeshed::Relationship.pending_for(@provider_uid).present?
	redirect_to nbp_wallet_finalize_users_path and return
	end

	ApplicationRecord.transaction do
	unless user.save
	abort_and_refresh(
	relationship,
	t('common.errors.model_not_created', model: User.model_name.human, errors: user.errors.full_messages.join(', '))
	) and raise ActiveRecord::Rollback
	end

	if relationship.accept!
	user.send_confirmation_instructions
	redirect_to home_index_path, notice: t('devise.registrations.signed_up_but_unconfirmed')
	else
	abort_and_refresh(relationship)
	raise ActiveRecord::Rollback
	end
	end

		"onNewRelationship": {
		"expiresAt": "2024-11-07T23:36:43.893Z",

[XI-6523] Transfer enmeshed changes from Xikolo #1831

Are you sure you want to change the base?

[XI-6523] Transfer enmeshed changes from Xikolo #1831

Conversation

nenock commented Jan 9, 2025 • edited Loading

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

nenock commented Jan 23, 2025 • edited Loading

MrSerth left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSerth Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

MrSerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nenock Feb 4, 2025 • 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

nenock commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

nenock commented Jan 23, 2025 •

edited

Loading

MrSerth Jan 25, 2025 •

edited

Loading

nenock Feb 4, 2025 •

edited

Loading