-
Notifications
You must be signed in to change notification settings - Fork 82
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
Enable Data Producers to Set Expiration Periods for Shares: #1083
Comments
Solution 1:Please look at attached diagram for more context on what A1, A2, B1,B2 mean: At what granularity is expiration defined?A1: Expiration is defined at dataset level: producer sets an expiration (for example 3 months) for the dataset. When share is granted, each share on that dataset will by default have 3 months expiration. This feature will be a part of dataset import/creation modal. When should notifications be sent out that expiry is approaching? And how frequently?
Who should be informed that expiry is being reached?B1: Producer - Should we inform DATA OWNERS since they are responsible for the governance of their datasets - more so than requestors of datasets? If access is renewed in timely manner and expiration not reached - Do nothingWhat happens when expiration is reached since access was not renewed in timely manner?C1: Escalate to skip level: Option to escalate to a higher authority in the chain for further review or action. |
Solution 2: Priority setting based systemPlease look at attached diagram for more context on what A1, A2, B1,B2 mean: What is the priority based system?
If Priority 1 - share never expires and access always remains When should notifications be sent out that expiry is approaching? And how frequently?2 weeks before share expires? Who should be informed that expiry is being reached?B1: Producer - Should we inform DATA OWNERS since they are responsible for the governance of their datasets - more so than requestors of datasets? If access is renewed in timely manner and expiration not reached - Do nothingWhat happens when expiration is reached since access was not renewed in timely manner?C1: Escalate to skip level: Option to escalate to a higher authority in the chain for further review or action. Now in this section, we can choose to handle expiration based on priority levels. |
Questions still remaining to be answered:
|
I like solution 1 as it allows to set granular expiration of shares. In this design, maybe we can sent out notifications 1 month before the share expiration and then 1 week before and then 1 day before . Once the share is expired then revoke shares for dataset which have a higher level of confidentiality ( ? ). Another option would be to specify what actions does the dataset owner want to take when the share expires., If the dataset owner chooses to revoke the share then revoke at expiry or if the owner chooses notify then notify or both. I like the UI indicators part. We can add a filter on the share list view to filter these shares which have expired. Once the share is about the expire, I like the option in which the consumer can request an extension on the share and then the producer approves the extension. And I think informing ( email notifications ) both producer and consumer about the share expiry would be prudent, just the way we send email notifications both to the producer and the consumers on a share request. Q. In this solution though, should we also allow the producer to modify the expiration period while approving the share / after approving the share? If we go with option 2 , to determine the priority , maybe could use confidentiality of dataset to determine priority ? As for the questions posed by you in the comments When should notification be sent out? -> 1 month before the share expires, then 1 week , 2 days , 1 day before Few question(s)
|
|
We decided on adv control button in the last design meeting. I am okay with the panel at the bottom too. Curious to know what others think - @dlpzx @noah-paige
For initial design, if extension is rejected, we can notify requestor via email that request has been denied. If its not significant efforts, I will implement a way similar to how a share request is currently denied. Producer will be able to put up a reason for rejection after clicking "Deny" and the reason can show up in the UI in Share Object Comments section.
Yes, I hadnt given much thought to the naming of fields yet. But this is helpful! Will take these suggestions into consideration.
Agreed!
Yes, I thought about this during the design. One way I could think of is, when the monthly ecs task runs, we can keep subtracting the expiry_validity (months) column of a share by 1 - unless a share is extended already. In that case, we might not need a set date at all, since the next time when this column becomes a 0 we can tell that the share has expired. |
@anushka-singh I'd just keep a field when the share was extended last.. when the share is created the date becomes the same as share approval date. |
Hi @anushka-singh , I like the design proposal. Very thorough and detailed. I just have one comment. Enable Auto approval is currently present in data.all on the main UI ( on dataset creation page ) . And I also saw the same thing in the "Advanced Controls" section. I think we can we can just have it placed either inside OR outside on the dataset creation page. Few questions -
|
|
Hi @anushka-singh thanks for the diagrams and mock-ups it was very easy to follow (I drafted my comments before looking at the feedback of others to have an unbiased opinion)
|
Sounds good!
Yes, fair point. We can let them know "it will expire in Xdays" in share request window.
Yes, I was looking for a better place for this too. I can put it in the share object metadata.
We can make it work same way like revoke works. If a user did not extend request, share object gets revoked. Then they will have to delete the share and create new one. Do you have a better way of handling this or is this ok?
Yes, if rejected, items will get revoked at the time of expiration. If I sent a request to extend before expiration cycle and extension is rejected, share will still be valid until expiration date. Does that make sense?
Sure we can do that. |
@anushka-singh will you be picking this up again as part of v2.7 ? |
@anmolsgandhi Let me discuss with the team internally and get back to you. |
@anmolsgandhi We are working on discussing our internal priorities at the moment. We should have an update by next week. |
@TejasRGitHub will build on my work on this for 2.7, while I focus on other tasks. The design is ready and some frontend work has already been done. He will continue to build on that. |
UI views :
3.. Dataset Edit form show the same view
Modifications made to the proposed design mentioned here - #1083 (comment)
Share Request Modal for accessing Non-expiring shares : Share View when the Request is Approved :
"share_expiration" : {
"active" : true,
"run_schedule" : [1, 3, 7]
} Refering the config above, now the share-expiration ECS task will get triggered 1, 3, 7 days before the end of the month |
### Feature or Bugfix - Feature - ### Detail - Details mentioned in the GH issue - #1083 ### Relates - #1083 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? No - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? No - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? No - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? Yes - Have you used the least-privilege principle? How? Yes By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: trajopadhye <tejas.rajopadhye@yahooinc.com>
Close as completed. To be released in 2.7 |
### Feature or Bugfix - Bugfix ### Detail 1. While revoking a share an error message pop-ups as shown below <img width="1267" alt="image" src="https://github.com/user-attachments/assets/bbc85627-fa8e-4a9f-a4af-2b37cc90bb81"> This is not intended. This doesn't change the expiration period but is calling the updateShareExpiration function. ### Relates #1083 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? N/A - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? N/A - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? N/A - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? N/A - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
### Feature or Bugfix - Feature ### Detail Changes to the logic of calculating expiration. Initially current month was considered into the calculation for expiration date. But this resulted in a bug while performing unit tests here #1594 and also this was not correct in the sense that if a user requests a month of expiration at the end of month, that user will get expiration at the end of the same month But with the new logic - #1594, more than half a month is granted extra . **To minimize this extra access period on share, this PR proposes new logic,** 1. If the user is in the last week of the month and if the user requests 1 month of data, allocate the end of the next month. 2. If the user is in NOT in the last week of the months and if the user requests 1 month of data, allocate the end of same month as expiration. ### Relates - #1083 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: trajopadhye <tejas.rajopadhye@yahooinc.com>
Is your feature request related to a problem? Please describe.
Describe the solution you'd like
1. Granular Expiration Settings:
Producers can set expiration periods at:
1.1 - Dataset level: Define an expiration date range (e.g., 30-90 days) when creating a dataset.
1.2 - Share level: Specify different expiration periods for individual shares based on requestor needs (e.g., 30 days for R1, 90 days for R2).
2. Notification System:
3. Handling Expiration:
3.1 - Escalation: Option to escalate to a higher authority in the chain for further review or action.
3.2 - Revocation: Ability to revoke access to the share, potentially disrupting downstream processes dependent on the dataset.
3.3 - Visual Indicators: Display a prominent indicator (e.g., red button) signaling the share's expired status without immediately revoking access, preventing disruptions to existing processes.
4. Configurability:
Describe alternatives you've considered
P.S. Please Don't attach files. Add code snippets directly in the message body instead.
The text was updated successfully, but these errors were encountered: