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

token based report #6216

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

token based report #6216

wants to merge 10 commits into from

Conversation

pjain1
Copy link
Member

@pjain1 pjain1 commented Dec 5, 2024

Changes:

  • This PR changes all the report links to always have a magic token.
  • Magic tokens are created for each run of the report with expiry of 60 days for both internal and external users.
  • This enables the links to never expire and everytime its clicked it just works (if user is removed or added etc. in all scenarios) unless the token expires.

UI changes needed:

  • Report Creation API - Since even external users can now open the report in explore, the API now expects either the metrics view and explore name or canvas name on which report is created.
  • Open link - handle token and use it show the explore (similar to a public URL). If a user is logged in, it should display a banner to say "this is a preview link" or something like that (may need product input here). The report's token may have different permissions/attributes than the current user (e.g. if the report was created by an admin, but the recipient is not an admin), so it cannot simply show the dashboard as the current user.
  • Download link - use token to enable download, it already happens for external user now so probably only removing internal download page (if exists).
  • Unsubscribe link - It will need to grab the email or slack_user query param from the url and call the unsubscribe API with this param.

Important Note: This does not support locking time ranges as of now (locking dimension filter works), as they would be evaluated during each report run at runtime. The queries that are sent by explore have a separate time range apart from the filter, magic token only supports row filter. If mgc token has time range then it will need to be reconciled with the actual time range sent.

Contributes to https://github.com/orgs/rilldata/projects/38/views/8?pane=issue&itemId=85181742&issue=rilldata%7Crill-private-issues%7C855

@pjain1 pjain1 marked this pull request as draft December 5, 2024 11:22
@pjain1 pjain1 requested a review from begelundmuller December 9, 2024 15:42
@pjain1 pjain1 marked this pull request as ready for review December 9, 2024 15:43
Copy link
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

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

Also, do we have a job that clears expired magic auth tokens periodically? If not, can you add one?

admin/database/database.go Outdated Show resolved Hide resolved
admin/server/reports.go Outdated Show resolved Hide resolved
admin/server/reports.go Outdated Show resolved Hide resolved
admin/server/reports.go Outdated Show resolved Hide resolved
admin/server/reports.go Outdated Show resolved Hide resolved
admin/server/reports.go Outdated Show resolved Hide resolved
Comment on lines 1928 to 1932
string edit_url = 3;
string unsubscribe_url = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

How will the report owner/admin open the report if they want to edit it? (E.g. change the frequency or the title)

Copy link
Member Author

@pjain1 pjain1 Dec 16, 2024

Choose a reason for hiding this comment

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

They will need to visit the reports page on UI.

We would need to detect if user is a project or org admin or creator of the report and add edit link. Currently we just add edit link for all Rill users but it has an issue if the user is not part of project (or not admin) otherwise it will be broken experience for them. For this reason I just removed the edit link and thought admin can visit the reports page, let me know if we want to add these checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though not great, this is alright. But would it be possible to keep the edit link for at least the owner user (I'm wondering if that would just require a simple check against the owner user ID)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added edit link for owner, also added unsubscribe as a separate link since owner can be removed from project or org but should have option to unsubscribe.

@pjain1
Copy link
Member Author

pjain1 commented Dec 16, 2024

Also, do we have a job that clears expired magic auth tokens periodically? If not, can you add one?

There is already a deleteExpiredAuthTokens job that will take care of it since we set an expiry.

@@ -283,7 +265,23 @@ func (s *Server) UnsubscribeReport(ctx context.Context, req *adminv1.Unsubscribe
return nil, status.Errorf(codes.InvalidArgument, "failed to find report token: %s", err.Error())
}

userEmail = reportTkn.RecipientEmail
if reportTkn.RecipientEmail == "" {
Copy link
Member Author

Choose a reason for hiding this comment

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

If a user is added as both slack and email recipient using same email then unsubscribing from any one unsubscribes from both places, what behaviour we want here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it easy to break it out by notifier type? If not, I think it's fine to unsubscribe both places because this will probably never happen in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@begelundmuller
Copy link
Contributor

begelundmuller commented Dec 27, 2024

@pjain1 I responded to the two open questions on this PR. Can you let me know when you address them (or if you decide they are not worth it) and fix the merge conflict?

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