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

Inet Service Model Updates #3561

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Inet Service Model Updates #3561

wants to merge 28 commits into from

Conversation

invisig0th
Copy link
Contributor

No description provided.

@ancailliau
Copy link

Feedback on inet:service:message

  • from should be an account, or a string, and then from:account the account.
  • Field deleted is missing.
  • I would miss place or telem, could be modeled as light edge though. Same with the client.
  • We store the raw message as a file and reference with a light edge, so not a field data limited to a JSON format.

Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.62%. Comparing base (2ce1ba9) to head (161e3f5).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3561      +/-   ##
==========================================
- Coverage   97.72%   97.62%   -0.11%     
==========================================
  Files         237      237              
  Lines       50619    50619              
==========================================
- Hits        49467    49416      -51     
- Misses       1152     1203      +51     
Flag Coverage Δ
linux 97.62% <100.00%> (-0.01%) ⬇️
linux_replay ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@invisig0th invisig0th changed the title WIP: Inet Service Model Updates Inet Service Model Updates May 22, 2024
mbvertex
mbvertex previously approved these changes May 22, 2024
synapse/models/inet.py Outdated Show resolved Hide resolved
synapse/models/infotech.py Outdated Show resolved Hide resolved
Comment on lines 3524 to 3525
'doc': 'The name of the group on this platform.'}),
('profile', ('ps:contact', {}), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'doc': 'The name of the group on this platform.'}),
('profile', ('ps:contact', {}), {
'doc': 'The name of the group on this platform.'}),
('profile', ('ps:contact', {}), {

synapse/models/inet.py Outdated Show resolved Hide resolved
synapse/models/inet.py Outdated Show resolved Hide resolved
synapse/models/infotech.py Outdated Show resolved Hide resolved
synapse/models/infotech.py Outdated Show resolved Hide resolved
Co-authored-by: Cisphyx <cisphyx@vertex.link>
invisig0th and others added 3 commits May 22, 2024 17:51
Co-authored-by: Cisphyx <cisphyx@vertex.link>
Co-authored-by: Cisphyx <cisphyx@vertex.link>
Co-authored-by: Cisphyx <cisphyx@vertex.link>
Copy link
Contributor

@therealsilence therealsilence left a comment

Choose a reason for hiding this comment

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

Mostly minor/clarifications, a few issues.

synapse/models/risk.py Outdated Show resolved Hide resolved
synapse/models/inet.py Show resolved Hide resolved
synapse/models/inet.py Show resolved Hide resolved
synapse/models/inet.py Show resolved Hide resolved
synapse/models/inet.py Outdated Show resolved Hide resolved
synapse/models/inet.py Show resolved Hide resolved
'doc': 'The period where the session was valid.'}),
)),

('inet:service:login', {}, (
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure several of the props declared on inet:service:object (used by inet:service:action) are applicable here, or if they are they represent a specialized use case (creator, owner, created, removed?). Those seem more applicable to object-type access vs. authentication. I'm ok with having edge case props on a form but will caveat that everyday users may find it confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, agreed. Lets talk about it in the regular weekly meet :)

synapse/models/inet.py Show resolved Hide resolved
synapse/models/inet.py Show resolved Hide resolved

('inet:service:access', {}, (

('resource', ('inet:service:resource', {}), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this include a reference to a inet:service:rule, inet:service:permission, or to indicate the type of access attempted? Leave as TODO for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would the referenced rule essentially mean "this is the rule which allowed/denied the access" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrrm. Was actually thinking of "type of access requested". If access fails, an error message (if available) might let me know what someone tried to do ("thesilence attempted to delete visi's macro"). But in the case of "access succeeded" (or the absence of an error message) , the form doesn't seem to capture what the requestor did / tried to do.

"Type of access requested" is not necessarily 1:1 with "rule that allowed or denied access". 🤔 Do we think it's a close enough proxy? Am I down a rabbit hole?

invisig0th and others added 11 commits May 28, 2024 09:52
Co-authored-by: therealsilence <therealsilence@users.noreply.github.com>
Co-authored-by: therealsilence <therealsilence@users.noreply.github.com>
Co-authored-by: Cisphyx <cisphyx@vertex.link>
Co-authored-by: therealsilence <therealsilence@users.noreply.github.com>
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.

None yet

7 participants