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

fix: Add authorization check for enrollment in lookup verb handler #1505

Merged
merged 7 commits into from
Aug 23, 2023

Conversation

sitaram-kalluri
Copy link
Member

@sitaram-kalluri sitaram-kalluri commented Aug 14, 2023

- What I did

  • When user is authenticated vai the APKAM mechanism, when executing lookup verb, verify if the user has authorization to the key's namespace.
  • Refactor lookup_verb_handler.dart to increase the readability.
  • Update the tests in scan_verb_test.dart. Populated enrollmentId to the inbound connection to mimic the connection is authenticated via the APKAM and authorization check for namespaces enrolled is performed.

- How I did it

  • When user authenticates via the APKAM mechanism, fetch enrollmentId from the connection metadata. Retrieve the enrolled namespaces and verify if user has permission for the key's namespace. If yes, return the data; else throw UnAuthorizedException.
  • Refactor 'Lookup_verb_handler.dart'

- How to verify it

  • Add unit tests in lookup_verb_test.dart to assert the APKAM enrollment changes. Ran all unit tests and functional tests in local and tests pass.
  • Add following scenario in scan verb unit test - A test to verify scan returns all keys when enrollment has *:rw access

- Description for the changelog

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

In addition to my comment on the verb handler implementation, I have one more question - are there tests which cover 'special' records? (1) 'reserved' keys (2) shared symmetric keys (shared_key.bob@alice and @bob:shared_key@alice) etc?

@sitaram-kalluri
Copy link
Member Author

In addition to my comment on the verb handler implementation, I have one more question - are there tests which cover 'special' records? (1) 'reserved' keys (2) shared symmetric keys (shared_key.bob@alice and @bob:shared_key@alice) etc?

Regarding the reserved keys, particularly the key format "@bob:shared_key@alice", when a user authenticates via the APKAM (who has full access), the absence of a namespace associated with the key leads to the failure of the authorization check. As a result, the data retrieval process does not succeed. One approach to deal with this is to have a system level namespace for all the reserved keys.
@gkc: Can you please share your thoughts on this?

@gkc
Copy link
Contributor

gkc commented Aug 22, 2023

In addition to my comment on the verb handler implementation, I have one more question - are there tests which cover 'special' records? (1) 'reserved' keys (2) shared symmetric keys (shared_key.bob@alice and @bob:shared_key@alice) etc?

Regarding the reserved keys, particularly the key format "@bob:shared_key@alice", when a user authenticates via the APKAM (who has full access), the absence of a namespace associated with the key leads to the failure of the authorization check. As a result, the data retrieval process does not succeed. One approach to deal with this is to have a system level namespace for all the reserved keys. @gkc: Can you please share your thoughts on this?

I suggest that if a record id has no namespace then we should say that the client is authorized. This is not an absolute rule - this rule should be applied any broader permission checks (e.g. records which are protected from deletion, immutable checks when we have immutable records, etc)

@sitaram-kalluri
Copy link
Member Author

In addition to my comment on the verb handler implementation, I have one more question - are there tests which cover 'special' records? (1) 'reserved' keys (2) shared symmetric keys (shared_key.bob@alice and @bob:shared_key@alice) etc?

Regarding the reserved keys, particularly the key format "@bob:shared_key@alice", when a user authenticates via the APKAM (who has full access), the absence of a namespace associated with the key leads to the failure of the authorization check. As a result, the data retrieval process does not succeed. One approach to deal with this is to have a system level namespace for all the reserved keys. @gkc: Can you please share your thoughts on this?

I suggest that if a record id has no namespace then we should say that the client is authorized. This is not an absolute rule - this rule should be applied any broader permission checks (e.g. records which are protected from deletion, immutable checks when we have immutable records, etc)

Sure Gary.

…ent is authorized for the keys that do not have namespace
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from apkam_spec_changes to trunk August 23, 2023 07:20
@gkc
Copy link
Contributor

gkc commented Aug 23, 2023

@sitaram-kalluri Before I approve and merge this, please run this server locally and run the at_talk program with 'new' atSigns alice and bob, and verify that everything is working correctly. Please also do any other client program testing that is appropriate.

@sitaram-kalluri
Copy link
Member Author

@sitaram-kalluri Before I approve and merge this, please run this server locally and run the at_talk program with 'new' atSigns alice and bob, and verify that everything is working correctly. Please also do any other client program testing that is appropriate.

Sure Gary.

@sitaram-kalluri
Copy link
Member Author

@sitaram-kalluri Before I approve and merge this, please run this server locally and run the at_talk program with 'new' atSigns alice and bob, and verify that everything is working correctly. Please also do any other client program testing that is appropriate.

Sure Gary.

  • Tested with at_talk and below send and receive messages. Attaching the recording.
  • Functional tests and E2E tests ran succesfully.
at_talk_test.mp4

@gkc gkc merged commit fb5bdd8 into trunk Aug 23, 2023
17 checks passed
@gkc gkc deleted the lookup_verb_changes branch August 23, 2023 18:00
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