-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Add support for login:password@ in calendar urls #430
base: master
Are you sure you want to change the base?
Conversation
Code.gs
Outdated
@@ -25,6 +25,8 @@ var sourceCalendars = [ // The ics/ical urls that you want to get | |||
// For instance: ["https://p24-calendars.icloud.com/holidays/us_en.ics", "US Holidays"] | |||
// Or with colors following mapping https://developers.google.com/apps-script/reference/calendar/event-color, | |||
// for instance: ["https://p24-calendars.icloud.com/holidays/us_en.ics", "US Holidays", "11"] | |||
// Login and password can be added | |||
// for instance: ["https://login:[email protected]/holidays/us_en.ics", "US Holidays"] |
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.
Interesting format. My first thought was "way too weird", but then I realized this is a standard way to do auth in URI strings (I've just never used it before).
@jonas0b1011001 What are your thoughts on this change? I know it's been requested before #157
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.
The question imo is, what's the benefit of importing a 2365 loc library compared to checking/extracting username:password with a 'simple' regex?
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.
Or, better than using a regex where we probably shouldn't, We could add two more optional settings:
var authUsername = "";
var authPassword = "";
Or even a single var authString = "";
that is just the literal auth header value (eg Bearer ...
)
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.
The credentials need to be url specific though, not sure if we want to add another parameter to the arrays.
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.
True. This might look better once I prototype #311.
@tatankat - can you make the changes discussed here? Since this seems to be an agreed-upon standard, let's keep the URI format. However, let's prefer using a regex (@jonas0b1011001 linked one example above - but you might not need one that complicated. I also like to encourage the usage of named capturing groups when possible) rather than including the URI.js.gs additional library.
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.
There you go. I thought using a library would make it more robust.
I also stumbled on a limitation of this notation needing percent-encoding for certain characters. Additional parameters might still be a good idea.
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.
Looks good to me.
Helpers.gs
Outdated
// it defaults to http if no scheme is given, but just // is not accepted | ||
// - Google Apps Scripts does not seem to accept \] in [^] lists | ||
// - to have the less possible need of percent-encoding, as much characters as possible are accepted | ||
const urlRegex = RegExp("^(https?://)?(?<username>[^:/@]+)(?::(?<password>[^/@]*))?@"); |
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 for converting this here based on feedback. I have one final suggestion: let's break this code block out to a separate function (eg parseAuthenticationFromCalendarURL
or whatever)
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.
You're welcome.
If you would like it in a separate function, please go ahead. You can take the code and put it in the structure you want, no problem.
I have merged the latest changes and in the meanwhile put the whole code block in a separate function |
Implementation of a generic solution for #363 using https://github.com/medialize/URI.js
Also would resolve #157