-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
regenerate activity URL when importing Workflows #15291
Conversation
Why the build fail in Ubuntu? |
no idea @hishamco , can you rerun the build? |
Sure I will re-run this again |
Already did, failed again. |
CORS issue (check the messages). Maybe some node version problem in the scripts, there are warnings in the build. |
The issue related to the JSON conversion, @lampersky please check the logs https://github.com/OrchardCMS/OrchardCore/actions/runs/7843466469/job/21417779248?pr=15291 |
I had not thought about this, these functional test recipes are only run on Linux, and the coming soon one imports a workflow for the subscription form. So it makes sense now. Here is the error message:
|
|
||
private string ReGenerateHttpRequestEventUrl(WorkflowType workflow, ActivityRecord activity) | ||
{ | ||
var tokenLifeSpan = activity.Properties["TokenLifeSpan"]?.ToObject<int>() ?? 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it actually try to generate the token property if there is no or invalid tokenlifespan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance if a request has no token, this code here would generate a 0-lifespan one so the request would fail, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, it's better to go with infinite token lifespan than no working activity at all. But it is up to you. What is your preference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
than no working activity at all
I don't understand, maybe I am missing some info. I thought that you could have request workflow that are not protected by a token. Is that correct? In that case you would still generate a token even if it's not necessary with your first suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that you could have request workflow that are not protected by a token. Is that correct?
no, every request to any HttpRequestEvent is protected by a token, a token is just an ecrypted json (it contains workflowId and ActivityId), this token has some lifespan, even if it isn't provided we are using some predefined value (100 years = ~36500 days)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't remember the details.
@sebastienros To maintain backward compatibility, I will treat activities without a provided timespan as never expiring. |
@lampersky good. However might still be better to not add a token if there were none before, assuming that's something that can happen. Shouldn't be hard in code either, just a condition before generating the link instead of defaulting to infinite lifetime. |
src/OrchardCore.Modules/OrchardCore.Workflows/Recipes/WorkflowTypeStep.cs
Outdated
Show resolved
Hide resolved
@sebastienros I've just pushed suggested changes. The activity URL is only refreshed when token life time is provided. |
fixes #15087