Add oauth2 authorization code flow support #345
Conversation
thezultimate
requested review from
anders-kiaer,
asnyv and
HansKallekleiv
as code owners
There was a problem hiding this comment.
This is great @thezultimate. 🎉👍🥳
One thing I've been thinking about: Should we remove the --oauth command line argument? Instead I have a suggestion we can discuss going along something like this:
- Plugins (which are Python classes) can choose to have a
@propertymethoddef oauth2(self):(or some other descriptive name. - In the
jinja2generated app script, if>=1plugin instances have that property method defined AND it returnsTrueduring app startup, we do what you know do when--oauth2is provided.
Being a plugin property method, the plugins will also be able to give True / False based on user configuration input in the YAML file (e.g. the plugin deduces that if user has not specified files on disk, it is expected to use external API etc... the logic here will be plugin specific).
We can take a short video session and discuss? I'm also open for contributing myself in the same PR here in order to move it to 🥅 .
thezultimate
force-pushed
the
oauth2
branch
3 times, most recently
from
4f7ba45 to
cd16e81
Compare
I think I have implemented the changes you suggested in the last commit. I have tested it and it worked as expected. Would it be possible for you to check? Or someone else if you are not available? Thanks. |
There was a problem hiding this comment.
Nice refactoring @thezultimate ! 🎉 Now we are close on getting this into master.
Some comments below.
It would also be nice with a new paragraph in https://github.com/equinor/webviz-config/blob/master/CONTRIBUTING.md explaining this nice new optional feature for plugin authors🚀
anders-kiaer
changed the title
webviz_config/_oauth2.py
Outdated
| WEBVIZ_SESSION_SECRET_KEY environment variable is also needed for signing the | ||
| session cookie. |
There was a problem hiding this comment.
Replaced by autogenerated key, isn't it?
There was a problem hiding this comment.
Ah yes, this should be removed :)
CHANGELOG.md
Outdated
| ### Changed | ||
| - [#345](https://github.com/equinor/webviz-config/pull/345) - Added Oauth2 | ||
| Authorization Code flow support for Azure AD applications. | ||
|
|
There was a problem hiding this comment.
| ### Changed | |
| - [#345](https://github.com/equinor/webviz-config/pull/345) - Added Oauth2 | |
| Authorization Code flow support for Azure AD applications. | |
| ### Added | |
| - [#345](https://github.com/equinor/webviz-config/pull/345) - Added Oauth2 | |
| Authorization Code flow support for Azure AD applications. | |
| ### Changed |
Insert a description of your pull request (PR) here, and check off the boxes below when they are done.
Contributor checklist
CHANGELOG.md, and added it if should be communicated there.