This should probably be True, and then only mention in the documentation that for local development, you want this False.

Shouldn't this be by_username?

They should, thanks

test_url.scheme == ref_url.scheme?

Note that using flask.session means you should make really sure that session uses https-only cookies in any production context.

We could but that might give problems with proxies and all no?

Not if you use the flask proxy support: that should use x-forwarded-scheme to get https for it's flask.request.host_url.

Perhaps name this oidc = OpenIDConnect instead of FAS = ?

A permanent session? That sounds like a bad idea to me...

I am not sure the gain is worth the potential pain

I thought about this, maybe more openidc to avoid conflict with the module

Or just OIDC

Sure. My point was mostly "not FAS", since it's not tied to FAS (in general..)

Perhaps fall back to "sub" if nickname is None?
Since there is no guarantee in general that OIDC gives you a (stable) nickname, and this would otherwise break horribly with other (mapped) IdP's.

This is just a workaround for a bug in flask where it doesn't run before_request.

(Which is going to break your setting of flask.g.auth as well).

I still consider this more a bug in flask-oidc than flask and even if it's not the OS we target will likely not be updated to a newer flask any time soon

You're checking for flask.session.fas_user, and then setting flask.session.auth?

Well, it's Flask that's not executing the before_request...

"authentify"? :)

And flask-oidc assuming having something in g while it may not be true :)

Maybe fall back to using sub if there's no nickname field?
There is no guarantee in the general case with IdPs mapped otherwise that the nickname field is there or stable.

Perhaps instead just have runserver.py set this to False?
Since runserver itself is meant for development setups.

It is inserting it on every request before_request. The flask documentation says: "Registers a function to run before each request.". Mark the "Each request" part.

I'd say "return hubs.models.... is not None" is more readable.

Is there a ticket upstream I could follow?

I'm more thinking on improving the runserver to allow specifying a configuration file to use for dev (and which would allow setting this configuration key to False while keeping the default correct). Sounds good?

What would be the correct one?

A permanent session? That sounds like a bad idea to me...

It's needed to be able to specify the duration of the session (like we do in
pkgdb, pagure, fedocal, election... :))

So.... this is because you actively clear all before_requests in tests/init.py: APP.before_request_funcs[None] = []

Yep.

I'm fine with this but could this ever happen in our setup?

Not in our setup (unless someone, probably me, messed up a config change), but if someone deploys this code outside of our infrastructure, or if someone copies the auth code from here, they can't depend on it.

Ok, I'll adjust although I doubt hubs ever get deployed elsewhere than our infra and having this assumption may actually bring some odd logic in our code for something that's unlikely to ever happen

did we add more widgets?

Not quite sure tbh, it is odd indeed

Just verified that all 11 widgets belong to the user ralph. This is fine.

00 = {Widget}

01 = {Widget}

02 = {Widget}

03 = {Widget}

04 = {Widget}

05 = {Widget}

06 = {Widget}

07 = {Widget}

08 = {Widget}

09 = {Widget}

10 = {Widget}

LGTM :thumbsup:

Oooh, tried to sneak in another change! I see you! :-)

Thanks for the review, let's merge :)

So... despite the confusion, :thumbsup: :-)