Skip to content

[Security] Add an interface for security passports which support attributes #42181

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

Closed
wants to merge 1 commit into from

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jul 18, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

Right now, attributes for security passports can't be relied upon as they don't exist in the interface, so to use them you effectively have to type check the concrete Passport class. This proposes an interface exposing the attributes API from the Passport class which can make the feature more reliable without expecting concrete classes.

@mbabker mbabker requested review from chalasr and wouterj as code owners July 18, 2021 23:56
@mbabker mbabker force-pushed the attribute-aware-passports branch from 5a7bdd4 to 3edd5e3 Compare July 19, 2021 00:04
@carsonbot carsonbot changed the title Add an interface for security passports which support attributes [Security] Add an interface for security passports which support attributes Jul 19, 2021
@derrabus derrabus added this to the 5.4 milestone Jul 19, 2021
@mbabker mbabker force-pushed the attribute-aware-passports branch from 3edd5e3 to a8aa89a Compare July 19, 2021 00:10
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

I like the proposal, but please don't use my vote as the final one to merge this PR. To me, at least @chalasr or @wouterj should react with a 👍 to get this merged, thank you.

@wouterj
Copy link
Member

wouterj commented Jul 19, 2021

Thanks for submitting a patch!

I have one reservation here, which doesn't have anything to do with the work you've done: Originally, there was a discussion about whether we should add an interface for passports (which is nothing more than a value object). I managed to convince people by saying "I want to refactor the UserInterface". Meanwhile, @chalasr has done an amazing job in refactoring the UserInterface in a backwards compatible way.

In other words: I think we maybe should deprecate the PassportInterface and remove it in 6.0. That would make adding a new passport interface invalid. I would like to see how other core team members now think about this (cc @nicolas-grekas and @weaverryan as you both were part of the original discussion about this).

@chalasr
Copy link
Member

chalasr commented Jul 19, 2021

I would keep PassportInterface.
I think we can expect quite a lot of implementations in userland apps and community bundles, there are already some so having an abstraction makes sense to me, and I prefer an interface over a base class.

About the proposed extension, I tend to think we'd better add those methods to PassportInterface (in a BC way, via @method at first).

(sorry for the close/reopen, messed up with my keyboard)

@chalasr chalasr closed this Jul 19, 2021
@chalasr chalasr reopened this Jul 19, 2021
@mbabker
Copy link
Contributor Author

mbabker commented Jul 19, 2021

About the proposed extension, I tend to think we'd better add those methods to PassportInterface (in a BC way, via @method at first).

The more I think about this suggestion, the more I start to think it actually makes the most sense.

At the root of this (and why I think the methods do need to be part of the interface, somehow), I know that the 5.3 compatible authenticators in LexikJWTAuthenticationBundle and JWTTokenRefreshBundle are using the attribute API and right now neither implementation is really 100% type-safe (both will end up causing fatals if someone were to subclass and authenticate with a passport that doesn't have attributes, like the passport linked from SchebTwoFactorBundle).

So, maybe the better approach is to add the attribute methods to the main PassportInterface and move the actual implementation from the root Passport class into the PassportTrait. This way, any custom passports (like the one from SchebTwoFactorBundle) using the trait only has to worry about the attribute methods potentially not existing while they're supporting Symfony 5.3, as the upgrade to 5.4 would add them automatically.

@wouterj
Copy link
Member

wouterj commented Jul 19, 2021

The one from scheb/2fa actually predates passport attributes (Christian did lots of work back in 5.1 to integrate the new system, while attributes were added "only" in 5.2). The needs of the special passport are now replaced by using attributes and I'll open an issue in that repository about deprecating the custom passport.

I really don't see a need to customize the passport. It has 2 very generic methods to store data: either the data is important for authentication - stored in a badge; or the data is just transported - key-value store using attributes.

@chalasr
Copy link
Member

chalasr commented Jul 19, 2021

Makes sense :) Custom badges are a better extension point and removes the need for custom passports. Let's burninate it!

@chalasr
Copy link
Member

chalasr commented Jul 19, 2021

Closing as discussed, thanks for raising @mbabker!

@chalasr chalasr closed this Jul 19, 2021
@mbabker mbabker deleted the attribute-aware-passports branch July 19, 2021 19:48
fabpot added a commit that referenced this pull request Aug 6, 2021
This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Deprecate `PassportInterface`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

As explained in #42181, the right extension point is badges, not passports.

Also renames `AuthenticatorInterface::createAuthenticatedToken()` to `createToken()` because of the signature change and the recent abandon of the `authenticated` state for tokens.

Commits
-------

a446030 [Security] Deprecate `PassportInterface`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants